Bug 1482019 - part 5: Make HTMLEditor::GetSelectedNode() never return non-element node and change its name to GetSelectedElement() r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 10 Aug 2018 18:01:42 +0900
changeset 486974 57e568a7f9334e4b0610199a3fec7fce79e786bb
parent 486973 66e9a30a6b1a192e6afdb9bae386243cec9a2127
child 486975 161817e6d127e4a1fdc0ba9f041c709e09096dcb
child 487036 5a77e82d273ef9dec5752614f0bb4fd35bca45d2
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1482019
milestone63.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 1482019 - part 5: Make HTMLEditor::GetSelectedNode() never return non-element node and change its name to GetSelectedElement() r=m_kato If HTMLEditor::GetSelectedNode() is called with nullptr for aTagName, the first block may return non-element node. In such case, we should return nullptr without error for now (since I have no idea which element node is a good node to return). Then, we can rename it to GetSelectedElement() and can replace existing GetSelectedElement() with the new one.
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
@@ -2637,28 +2637,28 @@ HTMLEditor::GetSelectedElement(const nsA
 
   RefPtr<Selection> selection = GetSelection();
   if (NS_WARN_IF(!selection)) {
     return NS_ERROR_FAILURE;
   }
 
   ErrorResult error;
   RefPtr<nsAtom> tagName = GetLowerCaseNameAtom(aTagName);
-  RefPtr<nsINode> selectedNode = GetSelectedNode(*selection, tagName, error);
+  RefPtr<nsINode> selectedNode = GetSelectedElement(*selection, tagName, error);
   if (NS_WARN_IF(error.Failed())) {
     return error.StealNSResult();
   }
   selectedNode.forget(aReturn);
   return NS_OK;
 }
 
-already_AddRefed<nsINode>
-HTMLEditor::GetSelectedNode(Selection& aSelection,
-                            const nsAtom* aTagName,
-                            ErrorResult& aRv)
+already_AddRefed<Element>
+HTMLEditor::GetSelectedElement(Selection& aSelection,
+                               const nsAtom* aTagName,
+                               ErrorResult& aRv)
 {
   MOZ_ASSERT(!aRv.Failed());
 
   bool isLinkTag = aTagName && IsLinkTag(*aTagName);
   bool isNamedAnchorTag = aTagName && IsNamedAnchorTag(*aTagName);
 
   RefPtr<nsRange> firstRange = aSelection.GetRangeAt(0);
   if (NS_WARN_IF(!firstRange)) {
@@ -2670,24 +2670,32 @@ HTMLEditor::GetSelectedNode(Selection& a
   nsIContent* startNode = firstRange->GetChildAtStartOffset();
 
   nsCOMPtr<nsINode> endContainer = firstRange->GetEndContainer();
   nsIContent* endNode = firstRange->GetChildAtEndOffset();
 
   // Optimization for a single selected element
   if (startContainer && startContainer == endContainer &&
       startNode && endNode && startNode->GetNextSibling() == endNode) {
-    nsCOMPtr<nsINode> selectedNode = startNode;
-    if (selectedNode) {
-      // Test for appropriate node type requested
-      if (!aTagName || aTagName == selectedNode->NodeInfo()->NameAtom() ||
-          (isLinkTag && HTMLEditUtils::IsLink(selectedNode)) ||
-          (isNamedAnchorTag && HTMLEditUtils::IsNamedAnchor(selectedNode))) {
-        return selectedNode.forget();
+    if (!aTagName) {
+      if (NS_WARN_IF(!startNode->IsElement())) {
+        // XXX Keep not returning error in this case, but perhaps, we should
+        //     look for element node.
+        return nullptr;
       }
+      RefPtr<Element> selectedElement = startNode->AsElement();
+      return selectedElement.forget();
+    }
+    // Test for appropriate node type requested
+    if (aTagName == startNode->NodeInfo()->NameAtom() ||
+        (isLinkTag && HTMLEditUtils::IsLink(startNode)) ||
+        (isNamedAnchorTag && HTMLEditUtils::IsNamedAnchor(startNode))) {
+      MOZ_ASSERT(startNode->IsElement());
+      RefPtr<Element> selectedElement = startNode->AsElement();
+      return selectedElement.forget();
     }
   }
 
   RefPtr<Element> selectedElement;
   if (isLinkTag) {
     // Link tag is a special case - we return the anchor node
     //  found for any selection that is totally within a link,
     //  included a collapsed selection (just a caret in a link)
@@ -2792,25 +2800,16 @@ HTMLEditor::GetSelectedNode(Selection& a
       }
     }
     iter->Next();
   }
   return selectedElement.forget();
 }
 
 already_AddRefed<Element>
-HTMLEditor::GetSelectedElement(const nsAString& aTagName)
-{
-  nsCOMPtr<nsISupports> domElement;
-  GetSelectedElement(aTagName, getter_AddRefs(domElement));
-  nsCOMPtr<Element> element = do_QueryInterface(domElement);
-  return element.forget();
-}
-
-already_AddRefed<Element>
 HTMLEditor::CreateElementWithDefaults(const nsAString& aTagName)
 {
   MOZ_ASSERT(!aTagName.IsEmpty());
 
   nsAutoString tagName(aTagName);
   ToLowerCase(tagName);
   nsAutoString realTagName;
 
@@ -4735,17 +4734,19 @@ HTMLEditor::GetSelectionContainer()
 
       nsCOMPtr<nsINode> startContainer = range->GetStartContainer();
       int32_t startOffset = range->StartOffset();
       nsCOMPtr<nsINode> endContainer = range->GetEndContainer();
       int32_t endOffset = range->EndOffset();
 
       if (startContainer == endContainer && startOffset + 1 == endOffset) {
         MOZ_ASSERT(!focusNode, "How did it get set already?");
-        focusNode = GetSelectedElement(EmptyString());
+        IgnoredErrorResult error;
+        focusNode = GetSelectedElement(*selection, nullptr, error);
+        NS_WARNING_ASSERTION(!error.Failed(), "Failed to get selected element");
       }
       if (!focusNode) {
         focusNode = range->GetCommonAncestor();
       }
     } else {
       for (int32_t i = 0; i < rangeCount; i++) {
         RefPtr<nsRange> range = selection->GetRangeAt(i);
 
--- a/editor/libeditor/HTMLEditor.h
+++ b/editor/libeditor/HTMLEditor.h
@@ -791,17 +791,17 @@ protected: // Shouldn't be used by frien
   /**
    * CollapseSelectionAfter() collapses Selection after aElement.
    * If aElement is an orphan node or not in editing host, returns error.
    */
   nsresult CollapseSelectionAfter(Selection& aSelection,
                                   Element& aElement);
 
   /**
-   * GetSelectedNode() returns an element node which is in first range of
+   * GetSelectedElement() returns an element node which is in first range of
    * aSelection.  The rule is a little bit complicated and the rules do not
    * make sense except in a few cases.  If you want to use this newly,
    * you should create new method instead.  This needs to be here for
    * comm-central.
    * The rules are:
    *   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.)
@@ -810,34 +810,32 @@ protected: // Shouldn't be used by frien
    *      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 does *not* match with the argument,
    *          *returns* the element.
    *     4-2. When first element node matches with the argument, returns
    *          *next* element node.
-   * XXX This may return non-element node in some cases.  Perhaps, it's a
-   *     bug.
    *
    * @param aSelection          The Selection.
    * @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.
    * @param aRv                 Returns error code.
    * @return                    An element in first range of aSelection.
    */
-  already_AddRefed<nsINode>
-  GetSelectedNode(Selection& aSelection,
-                  const nsAtom* aTagName,
-                  ErrorResult& aRv);
+  already_AddRefed<Element>
+  GetSelectedElement(Selection& aSelection,
+                     const nsAtom* aTagName,
+                     ErrorResult& aRv);
 
   /**
    * PasteInternal() pasts text with replacing selected content.
    * This tries to dispatch ePaste event first.  If its defaultPrevent() is
    * called, this does nothing but returns NS_OK.
    *
    * @param aClipboardType  nsIClipboard::kGlobalClipboard or
    *                        nsIClipboard::kSelectionClipboard.
@@ -1346,22 +1344,16 @@ protected: // Shouldn't be used by frien
    *                    return value and can use the
    *                    AutoSelectionSetterAfterTableEdit stack-based object to
    *                    insure we reset the caret in a table-editing method.
    */
   void SetSelectionAfterTableEdit(Element* aTable,
                                   int32_t aRow, int32_t aCol,
                                   int32_t aDirection, bool aSelected);
 
-  /**
-   * A more C++-friendly version of nsIHTMLEditor::GetSelectedElement
-   * that just returns null on errors.
-   */
-  already_AddRefed<dom::Element> GetSelectedElement(const nsAString& aTagName);
-
   void RemoveListenerAndDeleteRef(const nsAString& aEvent,
                                   nsIDOMEventListener* aListener,
                                   bool aUseCapture,
                                   ManualNACPtr aElement,
                                   nsIPresShell* aShell);
   void DeleteRefToAnonymousNode(ManualNACPtr aContent,
                                 nsIPresShell* aShell);
 
--- a/editor/libeditor/tests/test_nsIHTMLEditor_getSelectedElement.html
+++ b/editor/libeditor/tests/test_nsIHTMLEditor_getSelectedElement.html
@@ -252,21 +252,19 @@ SimpleTest.waitForFocus(function() {
 
   // <p>{p1}<b>b1</b><i>...
   range = document.createRange();
   range.setStart(editor.firstChild, 0);
   range.setEnd(editor.firstChild, 1);
   selection.removeAllRanges();
   selection.addRange(range);
 
-  // XXX This is a bug obviously since nsIHTMLEditor.idl defines that
-  //     getSelectedElement() won't return non-element node.
-  todo_is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
-          null,
-          "#1-11 nsIHTMLEditor::getSelectedElement(\"\") should return null when only a text node is selected");
+  is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("")),
+     null,
+     "#1-11 nsIHTMLEditor::getSelectedElement(\"\") should return null when only a text node is selected");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("b")),
      null,
      "#1-11 nsIHTMLEditor::getSelectedElement(\"b\") should return null when only a text node is selected");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("i")),
      null,
      "#1-11 nsIHTMLEditor::getSelectedElement(\"i\") should return null when only a text node is selected");
   is(SpecialPowers.unwrap(getHTMLEditor().getSelectedElement("href")),
      null,