Bug 1557996 - Make `HTMLEditor::GetSelectedElement()` not treat an element as selected when it's followed by a <br> element r=m_kato a=jcristau
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 11 Jun 2019 07:59:07 +0000
changeset 536924 9d6ef6164ec1cdc79d1420def6f40ad28d5bd7eb
parent 536923 0c663b927b277a56270923e22fb22911b8f5ada5
child 536925 7222059b69403e52b9b35e045ad0d9aa3afbd581
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, jcristau
bugs1557996
milestone68.0
Bug 1557996 - Make `HTMLEditor::GetSelectedElement()` not treat an element as selected when it's followed by a <br> element r=m_kato a=jcristau Currently, `HTMLEditor::GetSelectedElement()` is not used in mozilla-central and mainly used for handling double clicks in the editor with its complicated path. In most cases, users don't want double clicks to cause showing property dialog in mail composer. Therefore, we must be able to stricter in the complicated path. This patch adds new check whether the selected range ends immediately before a `<br>` element. If it's end at a `<br>` element, we shouldn't treat found element as selected. Note that when `<a href="...">` element is double-clicked, the element itself is selected like `<img>` element. So, we don't need to worry about the case which is that users probably want to update a link with double-clicking since such case is handled by the first optimized path in the method. Differential Revision: https://phabricator.services.mozilla.com/D34335
editor/libeditor/HTMLEditor.cpp
editor/libeditor/tests/test_nsIHTMLEditor_getSelectedElement.html
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -2750,19 +2750,19 @@ already_AddRefed<Element> HTMLEditor::Ge
   for (nsINode* lastNodeInRange = nullptr; !postOrderIter.IsDone();
        postOrderIter.Next()) {
     if (lastElementInRange) {
       // When any node follows an element node, not only one element is
       // selected so that return nullptr.
       return nullptr;
     }
 
-    // This loop ignored any non-element nodes before first element node.
-    // Its purpose must be that this method allow to this case as selecting
-    // an element:
+    // This loop ignors any non-element nodes before first element node.
+    // Its purpose must be that this method treats this case as selecting
+    // the <b> element:
     // - <p>abc <b>d[ef</b>}</p>
     // because children of an element node is listed up before the element.
     // However, this case must not be expected by the initial developer:
     // - <p>a[bc <b>def</b>}</p>
     // When we meet non-parent and non-next-sibling node of previous node,
     // it means that the range across element boundary (open tag in HTML
     // source).  So, in this case, we should not say only the following
     // element is selected.
@@ -2775,16 +2775,26 @@ already_AddRefed<Element> HTMLEditor::Ge
 
     lastNodeInRange = currentNode;
 
     lastElementInRange = Element::FromNodeOrNull(lastNodeInRange);
     if (!lastElementInRange) {
       continue;
     }
 
+    // And also, if it's followed by a <br> element, we shouldn't treat the
+    // the element is selected like this case:
+    // - <p><b>[def</b>}<br></p>
+    // Note that we don't need special handling for <a href> because double
+    // clicking it selects the element and we use the first path to handle it.
+    if (lastElementInRange->GetNextSibling() &&
+        lastElementInRange->GetNextSibling()->IsHTMLElement(nsGkAtoms::br)) {
+      return nullptr;
+    }
+
     if (!aTagName) {
       continue;
     }
 
     if (isLinkTag && HTMLEditUtils::IsLink(lastElementInRange)) {
       continue;
     }
 
--- a/editor/libeditor/tests/test_nsIHTMLEditor_getSelectedElement.html
+++ b/editor/libeditor/tests/test_nsIHTMLEditor_getSelectedElement.html
@@ -28,23 +28,25 @@ SimpleTest.waitForFocus(async function()
   // as far as possible.  Therefore, this API users don't expect that this
   // returns usual inline elements like <b> element.
 
   // So, we need to check user's operation works fine.
   for (let eatSpaceToNextWord of [true, false]) {
     await SpecialPowers.pushPrefEnv({"set": [["layout.word_select.eat_space_to_next_word", eatSpaceToNextWord]]});
 
     editor.innerHTML = "<p>This <b>is</b> an <i>example </i>text.<br></p>" +
-                       "<p>and an image <img src=\"green.png\"> is here.</p>";
+                       "<p>and an image <img src=\"green.png\"> is here.</p>" +
+                       "<p>An anchor with href attr <a href=\"about:blank\">is</a> here.</p>";
     editor.focus();
     editor.scrollTop; // flush layout.
 
     let b = editor.firstChild.firstChild.nextSibling;
     let i = b.nextSibling.nextSibling;
     let img = editor.firstChild.nextSibling.firstChild.nextSibling;
+    let href = editor.firstChild.nextSibling.nextSibling.firstChild.nextSibling;
 
     // double clicking usual inline element shouldn't cause "selecting" the element.
     synthesizeMouseAtCenter(b, {clickCount: 1});
     synthesizeMouseAtCenter(b, {clickCount: 2});
     is(selection.getRangeAt(0).startContainer, b.previousSibling,
        "#0-1 Double-clicking in <b> element should set start of selection to end of previous text node (eat_space_to_next_word: {$eatSpaceToNextWord})");
     is(selection.getRangeAt(0).startOffset, b.previousSibling.length,
        "#0-1 Double-clicking in <b> element should set start of selection to end of previous text node (eat_space_to_next_word: {$eatSpaceToNextWord})");
@@ -77,42 +79,75 @@ SimpleTest.waitForFocus(async function()
 
     is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
        null,
        "#0-2 nsIHTMLEditor::getSelectedElement(\"\") should return null after double-clicking in <b> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
 
     // Both clicking and double-clicking on <img> element should "select" it.
     synthesizeMouseAtCenter(img, {clickCount: 1});
     is(selection.getRangeAt(0).startContainer, img.parentElement,
-       "#0-3 Clicking in <b> element should set start of selection to the <img> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
+       "#0-3 Clicking in <img> element should set start of selection to the <img> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
     is(selection.getRangeAt(0).startOffset, 1,
-       "#0-3 Clicking in <b> element should set start of selection to the <img> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
+       "#0-3 Clicking in <img> element should set start of selection to the <img> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
     is(selection.getRangeAt(0).endContainer, img.parentElement,
-       "#0-3 Clicking in <b> element should set end of selection to start of next text node (eat_space_to_next_word: {$eatSpaceToNextWord})");
+       "#0-3 Clicking in <img> element should set end of selection to start of next text node (eat_space_to_next_word: {$eatSpaceToNextWord})");
     is(selection.getRangeAt(0).endOffset, 2,
-       "#0-3 Clicking in <b> element should set end of selection to start of next text node (eat_space_to_next_word: {$eatSpaceToNextWord})");
+       "#0-3 Clicking in <img> element should set end of selection to start of next text node (eat_space_to_next_word: {$eatSpaceToNextWord})");
 
     is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
        img,
        "#0-3 nsIHTMLEditor::getSelectedElement(\"\") should return the <img> element after clicking in it (eat_space_to_next_word: {$eatSpaceToNextWord})");
 
     synthesizeMouseAtCenter(img, {clickCount: 1});
     synthesizeMouseAtCenter(img, {clickCount: 2});
     is(selection.getRangeAt(0).startContainer, img.parentElement,
-       "#0-4 Double-clicking in <b> element should set start of selection to the <img> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
+       "#0-4 Double-clicking in <img> element should set start of selection to the <img> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
     is(selection.getRangeAt(0).startOffset, 1,
-       "#0-4 Double-clicking in <b> element should set start of selection to the <img> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
+       "#0-4 Double-clicking in <img> element should set start of selection to the <img> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
     is(selection.getRangeAt(0).endContainer, img.parentElement,
-       "#0-4 Double-clicking in <b> element should set end of selection to start of next text node (eat_space_to_next_word: {$eatSpaceToNextWord})");
+       "#0-4 Double-clicking in <img> element should set end of selection to start of next text node (eat_space_to_next_word: {$eatSpaceToNextWord})");
     is(selection.getRangeAt(0).endOffset, 2,
-       "#0-4 Double-clicking in <b> element should set end of selection to start of next text node (eat_space_to_next_word: {$eatSpaceToNextWord})");
+       "#0-4 Double-clicking in <img> element should set end of selection to start of next text node (eat_space_to_next_word: {$eatSpaceToNextWord})");
 
     is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
        img,
        "#0-4 nsIHTMLEditor::getSelectedElement(\"\") should return the <img> element after double-clicking in it (eat_space_to_next_word: {$eatSpaceToNextWord})");
+
+    // Puts caret into the <a href> element.
+    synthesizeMouseAtCenter(href, {clickCount: 1});
+    is(selection.getRangeAt(0).startContainer, href.firstChild,
+       "#0-5 Clicking in <a href> element should set start of selection to the text node in it (eat_space_to_next_word: {$eatSpaceToNextWord})");
+    is(selection.isCollapsed, true,
+       "#0-5 Clicking in <a href> element should cause collapsing Selection (eat_space_to_next_word: {$eatSpaceToNextWord})");
+
+    is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
+       null,
+       "#0-5 nsIHTMLEditor::getSelectedElement(\"\") should return null after clicking in the <a href> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
+    is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("href")),
+       href,
+       "#0-5 nsIHTMLEditor::getSelectedElement(\"href\") should return the <a href> element after clicking in it (eat_space_to_next_word: {$eatSpaceToNextWord})");
+
+    // Selects the <a href> element with a double-click.
+    synthesizeMouseAtCenter(href, {clickCount: 1});
+    synthesizeMouseAtCenter(href, {clickCount: 2});
+    is(selection.getRangeAt(0).startContainer, href.parentElement,
+       "#0-6 Double-clicking in <a href> element should set start of selection to the element (eat_space_to_next_word: {$eatSpaceToNextWord})");
+    is(selection.getRangeAt(0).startOffset, 1,
+       "#0-6 Double-clicking in <a href> element should set start of selection to the element (eat_space_to_next_word: {$eatSpaceToNextWord})");
+    is(selection.getRangeAt(0).endContainer, href.parentElement,
+       "#0-6 Double-clicking in <a href> element should set end of selection to start of next <br> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
+    is(selection.getRangeAt(0).endOffset, 2,
+       "#0-6 Double-clicking in <a href> element should set end of selection to start of next <br> element (eat_space_to_next_word: {$eatSpaceToNextWord})");
+
+    is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
+       href,
+       "#0-6 nsIHTMLEditor::getSelectedElement(\"\") should return the <a href> element after double-clicking in it (eat_space_to_next_word: {$eatSpaceToNextWord})");
+    is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("href")),
+       href,
+       "#0-6 nsIHTMLEditor::getSelectedElement(\"href\") should return the <a href> element after double-clicking in it (eat_space_to_next_word: {$eatSpaceToNextWord})");
   }
 
   editor.innerHTML = "<p>p1<b>b1</b><i>i1</i></p>";
   editor.focus();
 
   // <p>[]p1...
   let range = document.createRange();
   range.setStart(editor.firstChild.firstChild, 0);
@@ -696,16 +731,59 @@ SimpleTest.waitForFocus(async function()
      "#7-1 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection starts from a text node in <b> element and ends at start of following text node");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("href")),
      null,
      "#7-1 nsIHTMLEditor::getSelectedElement(\"href\") should return null when Selection starts from a text node in <b> element and ends at start of following text node");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("anchor")),
      null,
      "#7-1 nsIHTMLEditor::getSelectedElement(\"anchor\") should return null when Selection starts from a text node in <b> element and ends at start of following text node");
 
+  editor.innerHTML = "<p>p1<b>b1</b><br></p>";
+  editor.focus();
+
+  // <p>p1[<b>b1</b>}<br></p>
+  // This is usual case of double-clicking in the <b> element.
+  range = document.createRange();
+  range.setStart(editor.firstChild.firstChild, 2);
+  range.setEnd(editor.firstChild, 2);
+  selection.removeAllRanges();
+  selection.addRange(range);
+
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
+     null,
+     "#8-1 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection starts from previous text node of <b> element and ends before the following <br> element");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("href")),
+     null,
+     "#8-1 nsIHTMLEditor::getSelectedElement(\"href\") should return null when Selection starts from previous text node of <b> element and ends before the following <br> element");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("anchor")),
+     null,
+     "#8-1 nsIHTMLEditor::getSelectedElement(\"anchor\") should return null when Selection starts from previous text node of <b> element and ends before the following <br> element");
+
+
+  editor.innerHTML = "<p><b>b1</b><br></p>";
+  editor.focus();
+
+  // <p><b>[b1</b>}<br></p>
+  // This is usual case of double-clicking in the <b> element.
+  range = document.createRange();
+  range.setStart(editor.firstChild.firstChild.firstChild, 0);
+  range.setEnd(editor.firstChild, 1);
+  selection.removeAllRanges();
+  selection.addRange(range);
+
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
+     null,
+     "#9-1 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection starts from a text node in <b> element and ends before the following <br> element");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("href")),
+     null,
+     "#9-1 nsIHTMLEditor::getSelectedElement(\"href\") should return null when Selection starts from a text node in <b> element and ends before the following <br> element");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("anchor")),
+     null,
+     "#9-1 nsIHTMLEditor::getSelectedElement(\"anchor\") should return null when Selection starts from a text node in <b> element and ends before the following <br> element");
+
   SimpleTest.finish();
 });
 
 function getHTMLEditor() {
   var Ci = SpecialPowers.Ci;
   var editingSession = SpecialPowers.wrap(window).docShell.editingSession;
   return editingSession.getEditorForWindow(window).QueryInterface(Ci.nsIHTMLEditor);
 }