Bug 1582010 - Make `IMEContentObserver` always recompute selection range after sending text change notification r=m_kato a=lizzard
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 19 Sep 2019 08:54:04 +0000
changeset 555298 a552d3955e14a2e2c98f9c56aed0d0a1438e5bf3
parent 555297 d7a88a4fb29501dd22616ad0d39c4e6fd6b9f097
child 555299 3ca86ab994bff98de8a5cc93c8d75b48ce7bcb03
push id2165
push userffxbld-merge
push dateMon, 14 Oct 2019 16:30:58 +0000
treeherdermozilla-release@0eae18af659f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, lizzard
bugs1582010
milestone70.0
Bug 1582010 - Make `IMEContentObserver` always recompute selection range after sending text change notification r=m_kato a=lizzard `IMEContentObserver` sends selection change notification only when it receives a DOM `Selection` change notification received. However, selection range in plaintext offset may be changed when changing previous content of caret is changed. In this case, currently we notify IME of only text change and that causes IME keep caching selection offsets in previous content. This causes IME queries character rect out of bounds. Therefore, even if `IMEContentObserver` hasn't received DOM `Selection` change notification, it should recompute selection range and if it's changed, it should notify IME of selection change too. Differential Revision: https://phabricator.services.mozilla.com/D46251
dom/events/IMEContentObserver.cpp
widget/tests/window_composition_text_querycontent.xul
--- a/dom/events/IMEContentObserver.cpp
+++ b/dom/events/IMEContentObserver.cpp
@@ -1679,24 +1679,29 @@ IMEContentObserver::IMENotificationSende
 
   if (observer->mNeedsToNotifyIMEOfTextChange) {
     observer->mNeedsToNotifyIMEOfTextChange = false;
     SendTextChange();
   }
 
   // If a text change notification causes another text change again, we should
   // notify IME of that before sending a selection change notification.
+  // Otherwise, even if the observer hasn't received selection change, let's
+  // try to send selection change notification to IME because selection
+  // start offset may be changed if the previous contents of selection start
+  // are changed.  For example, when previous `<p>` element of another `<p>`
+  // element which contains caret is removed by a DOM mutation, selection
+  // change event won't be fired, but selection start offset should be
+  // decreased by the length of removed `<p>` element.
   if (!observer->mNeedsToNotifyIMEOfTextChange) {
     // Be aware, PuppetWidget depends on the order of this. A selection change
     // notification should not be sent before a text change notification because
     // PuppetWidget shouldn't query new text content every selection change.
-    if (observer->mNeedsToNotifyIMEOfSelectionChange) {
-      observer->mNeedsToNotifyIMEOfSelectionChange = false;
-      SendSelectionChange();
-    }
+    observer->mNeedsToNotifyIMEOfSelectionChange = false;
+    SendSelectionChange();
   }
 
   // If a text change notification causes another text change again or a
   // selection change notification causes either a text change or another
   // selection change, we should notify IME of those before sending a position
   // change notification.
   if (!observer->mNeedsToNotifyIMEOfTextChange &&
       !observer->mNeedsToNotifyIMEOfSelectionChange) {
--- a/widget/tests/window_composition_text_querycontent.xul
+++ b/widget/tests/window_composition_text_querycontent.xul
@@ -8810,25 +8810,56 @@ async function runIMEContentObserverTest
     await waitNotifications;
     ensureToRemovePrecedingPositionChangeNotification();
     is(aElement.innerHTML, "<div>1<div>2<div>39</div></div></div>", description + " should remove '</div>4</div>5<div>6<div>7</div>8</div>'");
     // It causes removing '</div>4</div>5<div>6<div>7</div>8</div>' and inserting '<div>36<div>7</div>8</div>'.
     checkTextChangeNotification(notifications[0], description, { offset: getNativeText("\n1\n2\n3").length + offsetAtStart, removedLength: getNativeText("45\n6\n789").length, addedLength: getNativeText("9").length });
     checkSelectionChangeNotification(notifications[1], description, { offset: getNativeText("\n1\n2\n3").length + offsetAtStart, text: "" });
     checkPositionChangeNotification(notifications[2], description);
     dumpUnexpectedNotifications(description, 3);
+
+    // "<p>abc</p><p><br></p><p>{<br>}</p>" and removing second paragraph with DOM API
+    aElement.innerHTML = "<p>abc</p><p><br></p><p><br></p>";
+    sel.collapse(aElement.firstChild.nextSibling.nextSibling, 0);
+    await flushNotifications();
+    description = aDescription + "deleting previous paragraph with DOM API";
+    waitNotifications = promiseReceiveNotifications();
+    synthesizeKey("KEY_Unidentified", { code: "" }, win, callback);  // For setting the callback to recode notifications
+    aElement.firstChild.nextSibling.remove();
+    await waitNotifications;
+    ensureToRemovePrecedingPositionChangeNotification();
+    is(aElement.innerHTML, "<p>abc</p><p><br></p>", description + " the second paragraph should've been removed");
+    checkTextChangeNotification(notifications[0], description, { offset: getNativeText("\nabc").length + offsetAtStart, removedLength: getNativeText("\n\n").length, addedLength: 0 });
+    checkSelectionChangeNotification(notifications[1], description, { offset: getNativeText("\nabc\n").length + offsetAtStart, text: "" });
+    checkPositionChangeNotification(notifications[2], description);
+    dumpUnexpectedNotifications(description, 3);
+
+    // "<p>abc</p><p>{<br>}</p><p><br></p>" and removing last paragraph with DOM API
+    aElement.innerHTML = "<p>abc</p><p><br></p><p><br></p>";
+    sel.collapse(aElement.firstChild.nextSibling, 0);
+    await flushNotifications();
+    description = aDescription + "deleting next paragraph with DOM API";
+    waitNotifications = promiseReceiveNotifications();
+    synthesizeKey("KEY_Unidentified", { code: "" }, win, callback);  // For setting the callback to recode notifications
+    aElement.firstChild.nextSibling.nextSibling.remove();
+    await waitNotifications;
+    ensureToRemovePrecedingPositionChangeNotification();
+    is(aElement.innerHTML, "<p>abc</p><p><br></p>", description + " the last paragraph should've been removed");
+    checkTextChangeNotification(notifications[0], description, { offset: getNativeText("\nabc\n\n").length + offsetAtStart, removedLength: getNativeText("\n\n").length, addedLength: 0 });
+    checkPositionChangeNotification(notifications[1], description);
+    dumpUnexpectedNotifications(description, 2);
   }
 
   await testWithPlaintextEditor("runIMEContentObserverTest with input element: ", input, false);
   await testWithPlaintextEditor("runIMEContentObserverTest with textarea element: ", textarea, true);
   await testWithHTMLEditor("runIMEContentObserverTest with contenteditable (defaultParagraphSeparator is br): ", contenteditable, "br");
   await testWithHTMLEditor("runIMEContentObserverTest with contenteditable (defaultParagraphSeparator is p): ", contenteditable, "p");
   await testWithHTMLEditor("runIMEContentObserverTest with contenteditable (defaultParagraphSeparator is div): ", contenteditable, "div");
   // XXX Due to the difference of HTML editor behavior between designMode and contenteditable,
-  //     testWithHTMLEditor() gets some unexpected behavior.  However, IMEContentObserveri is
+  //     testWithHTMLEditor() gets some unexpected behavior.  However, IMEContentObserver is
   //     not depend on editor's detail.  So, we should investigate this issue later.  It's not
   //     so important for now.
   // await testWithHTMLEditor("runIMEContentObserverTest in designMode (defaultParagraphSeparator is br): ", iframe2.contentDocument.body, "br");
   // await testWithHTMLEditor("runIMEContentObserverTest in designMode (defaultParagraphSeparator is p): ", iframe2.contentDocument.body, "p");
   // await testWithHTMLEditor("runIMEContentObserverTest in designMode (defaultParagraphSeparator is div): ", iframe2.contentDocument.body, "div");
 }
 
 async function runPasswordMaskDelayTest() {