Bug 1501663 - part 2: HTMLEditor::GetSelectedElement() should return nullptr if 2 or more elements are in selected range r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 05 Nov 2018 04:57:02 +0000
changeset 444410 d0cbbc6d237140067877049c1c9c103667b453dc
parent 444409 eb94ff3abeca2cbf252a5fab5e8c1c3a997b7486
child 444411 a7c1286bce7f2f5274980250becf197039860794
push id109582
push usercsabou@mozilla.com
push dateMon, 05 Nov 2018 16:21:20 +0000
treeherdermozilla-inbound@4115e3cf7ad0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1501663, 1432944, 1482019
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1501663 - part 2: HTMLEditor::GetSelectedElement() should return nullptr if 2 or more elements are in selected range r=m_kato This is also regression of bug 1432944, but hidden by the optimization of bug 1482019. In bug 1482019, we removing the code clearing |found| flag when the loop meets second element in selection range. https://searchfox.org/mozilla-central/diff/0dd29e18302c5e6b07b88aac7164889d752acda7/editor/libeditor/HTMLEditor.cpp#2751-2753 because the flag won't be referred. However, before the fix of bug 1432944, it's referred and the cleared flag makes the method return nullptr. Therefore, this patch makes it return nullptr when 2 or more elements are in selected range. Differential Revision: https://phabricator.services.mozilla.com/D10698
editor/libeditor/HTMLEditor.cpp
editor/libeditor/HTMLEditor.h
editor/libeditor/tests/test_nsIHTMLEditor_getSelectedElement.html
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -3009,57 +3009,45 @@ HTMLEditor::GetSelectedElement(const nsA
 
   if (SelectionRefPtr()->IsCollapsed()) {
     return selectedElement.forget();
   }
 
   nsCOMPtr<nsIContentIterator> iter = NS_NewContentIterator();
 
   bool found = !!selectedElement;
-  const nsAtom* tagNameLookingFor = aTagName;
   iter->Init(firstRange);
   // loop through the content iterator for each content node
   while (!iter->IsDone()) {
-    // Update selectedElement with new node.  If it's not an element node,
-    // clear it.
     // XXX This is really odd since this means that the result depends on
     //     what is the last node.  If the last node is an element node,
     //     it may be returned even if it does not match with aTagName.
     //     On the other hand, if last node is not an element, i.e., we have
     //     not found proper element node, we return nullptr as this method
     //     name explains.
     selectedElement = Element::FromNodeOrNull(iter->GetCurrentNode());
     if (selectedElement) {
-      // If we already found a node, then we have another element,
-      // thus there's not just one element selected.
-      // XXX Really odd.  The new element node may be different name element.
-      //     So, this means that we return any next element node if we find
-      //     proper element as first element in the range.
       if (found) {
-        break;
+        // At least 2 elements are in the range so that return nullptr.
+        return nullptr;
       }
 
-      if (!tagNameLookingFor) {
-        // Get name of first selected element
-        // XXX Looks like that this is necessary only for making the following
-        //     handler work as expected...  Why don't you check this below??
-        tagNameLookingFor = selectedElement->NodeInfo()->NameAtom();
+      if (!aTagName) {
+        found = true;
       }
-
       // The "A" tag is a pain,
       //  used for both link(href is set) and "Named Anchor"
-      if ((isLinkTag &&
-           HTMLEditUtils::IsLink(selectedElement)) ||
-          (isNamedAnchorTag &&
-           HTMLEditUtils::IsNamedAnchor(selectedElement))) {
+      else if ((isLinkTag &&
+                HTMLEditUtils::IsLink(selectedElement)) ||
+               (isNamedAnchorTag &&
+                HTMLEditUtils::IsNamedAnchor(selectedElement))) {
         found = true;
       }
       // All other tag names are handled here.
-      else if (tagNameLookingFor ==
-                 selectedElement->NodeInfo()->NameAtom()) {
+      else if (aTagName == selectedElement->NodeInfo()->NameAtom()) {
         found = true;
       }
 
       if (!found) {
         return nullptr;
       }
     }
     iter->Next();
--- a/editor/libeditor/HTMLEditor.h
+++ b/editor/libeditor/HTMLEditor.h
@@ -1077,19 +1077,19 @@ protected: // Shouldn't be used by frien
    *   1. If Selection selects an element node, i.e., both containers are
    *      same node and start offset and end offset is start offset + 1.
    *      (XXX However, if last child is selected, this path is not used.)
    *   2. If the argument is "href", look for anchor elements whose href
    *      attribute is not empty from container of anchor/focus of Selection
    *      to <body> element.  Then, both result are same one, returns the node.
    *      (i.e., this allows collapsed selection.)
    *   3. If the Selection is collapsed, returns null.
-   *   4. Otherwise, listing up all nodes with content iterator (post-order).
-   *     4-1. When first element node matches with the argument, returns
-   *          *next* element node.
+   *   4. Otherwise, listing up all nodes with content iterator (post-order)
+   *      and only when the last node is first element node, returns the
+   *      element node.
    *
    * @param aTagName            The atom of tag name in lower case.
    *                            If nullptr, look for any element node.
    *                            If nsGkAtoms::href, look for an <a> element
    *                            which has non-empty href attribute.
    *                            If nsGkAtoms::anchor or atomized "namedanchor",
    *                            look for an <a> element which has non-empty
    *                            name attribute.
--- a/editor/libeditor/tests/test_nsIHTMLEditor_getSelectedElement.html
+++ b/editor/libeditor/tests/test_nsIHTMLEditor_getSelectedElement.html
@@ -231,22 +231,22 @@ SimpleTest.waitForFocus(async function()
 
   // <p>[p1<b>b1</b><i>i1</i>}...
   range = document.createRange();
   range.setStart(editor.firstChild.firstChild, 0);
   range.setEnd(editor.firstChild, 3);
   selection.removeAllRanges();
   selection.addRange(range);
 
-  todo_is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
-          null,
-          "#1-8 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection includes 2 elements and starts from previous text node");
-  todo_is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("b")),
-          null,
-          "#1-8 nsIHTMLEditor::getSelectedElement(\"b\") should return null when Selection includes 2 elements and starts from previous text node");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
+     null,
+     "#1-8 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection includes 2 elements and starts from previous text node");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("b")),
+     null,
+     "#1-8 nsIHTMLEditor::getSelectedElement(\"b\") should return null when Selection includes 2 elements and starts from previous text node");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("i")),
      null,
      "#1-8 nsIHTMLEditor::getSelectedElement(\"i\") should return null when Selection includes 2 elements and starts from previous text node");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("href")),
      null,
      "#1-8 nsIHTMLEditor::getSelectedElement(\"href\") should null when Selection includes 2 elements and starts from previous text node");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("anchor")),
      null,
@@ -340,39 +340,39 @@ SimpleTest.waitForFocus(async function()
 
   // <p>p1<b>b[1</b><b>b2</b><b>b]3</b>...
   range = document.createRange();
   range.setStart(editor.firstChild.firstChild.nextSibling.firstChild, 1);
   range.setEnd(editor.firstChild.firstChild.nextSibling.nextSibling.nextSibling.firstChild, 1);
   selection.removeAllRanges();
   selection.addRange(range);
 
-  todo_is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
-          null,
-          "#2-1 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection is across 3 <b> elements");
-  todo_is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("b")),
-          null,
-          "#2-1 nsIHTMLEditor::getSelectedElement(\"b\") should return null when Selection is across 3 <b> elements");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
+     null,
+     "#2-1 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection is across 3 <b> elements");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("b")),
+     null,
+     "#2-1 nsIHTMLEditor::getSelectedElement(\"b\") should return null when Selection is across 3 <b> elements");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("i")),
      null,
      "#2-1 nsIHTMLEditor::getSelectedElement(\"i\") should return null when Selection is across 3 <b> elements");
 
   // <p>p[1<b>b1</b><b>b2</b><b>b]3</b>...
   range = document.createRange();
   range.setStart(editor.firstChild.firstChild, 1);
   range.setEnd(editor.firstChild.firstChild.nextSibling.nextSibling.nextSibling.firstChild, 1);
   selection.removeAllRanges();
   selection.addRange(range);
 
-  todo_is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
-          null,
-          "#2-2 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection is across 3 <b> elements and previous text node");
-  todo_is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("b")),
-          null,
-          "#2-2 nsIHTMLEditor::getSelectedElement(\"b\") should return null when Selection is across 3 <b> elements and previous text node");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
+     null,
+     "#2-2 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection is across 3 <b> elements and previous text node");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("b")),
+     null,
+     "#2-2 nsIHTMLEditor::getSelectedElement(\"b\") should return null when Selection is across 3 <b> elements and previous text node");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("i")),
      null,
      "#2-2 nsIHTMLEditor::getSelectedElement(\"i\") should return null when Selection is across 3 <b> elements and previous text node");
 
   editor.innerHTML = "<p>p1<b>b1<b>b2<b>b3</b></b></b>p2</p>";
   editor.focus();
 
   // <p>p1<b>b[1<b>b1-2<b>b]1-3</b>...
@@ -411,22 +411,22 @@ SimpleTest.waitForFocus(async function()
 
   // <p>p1<b>b1<b>b1-2<b>b[1-3</b></b></b>p]2...
   range = document.createRange();
   range.setStart(editor.firstChild.firstChild.nextSibling.firstChild.nextSibling.firstChild.nextSibling.firstChild, 1);
   range.setEnd(editor.firstChild.firstChild.nextSibling.nextSibling, 1);
   selection.removeAllRanges();
   selection.addRange(range);
 
-  todo_is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
-          null,
-          "#3-3 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection is across 3 <b> elements which are nested and following text node");
-  todo_is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("b")),
-          null,
-          "#3-3 nsIHTMLEditor::getSelectedElement(\"b\") should return null when Selection is across 3 <b> elements which are nested and following text node");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
+     null,
+     "#3-3 nsIHTMLEditor::getSelectedElement(\"\") should return null when Selection is across 3 <b> elements which are nested and following text node");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("b")),
+     null,
+     "#3-3 nsIHTMLEditor::getSelectedElement(\"b\") should return null when Selection is across 3 <b> elements which are nested and following text node");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("i")),
      null,
      "#3-3 nsIHTMLEditor::getSelectedElement(\"i\") should return null when Selection is across 3 <b> elements which are nested and following text node");
 
   editor.innerHTML = "<p><b>b1</b><a href=\"about:blank\">a1</a><a href=\"about:blank\">a2</a><a name=\"foo\">a3</a><b>b2</b></p>";
   editor.focus();
 
   // <p><b>b1</b>{<a href="...">a1</a>}<a href="...">a2...