Bug 1383242 - Backout "Correctly calculate start offset for non-text nodes" (bug 1351170). r=backout, a=lizzard
authorJim Chen <nchen@mozilla.com>
Tue, 08 Aug 2017 19:17:00 -0400
changeset 423466 21d266694e28e1913288a8f8c0891dc34eeef186
parent 423465 5903c9372f345b99030d6467888979a2e5f9583f
child 423467 de3698e225ce40eb85dfd2b4b75027932b1dcc61
push id1517
push userjlorenzo@mozilla.com
push dateThu, 14 Sep 2017 16:50:54 +0000
treeherdermozilla-release@3b41fd564418 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbackout, lizzard
bugs1383242, 1351170
milestone56.0
Bug 1383242 - Backout "Correctly calculate start offset for non-text nodes" (bug 1351170). r=backout, a=lizzard Back out part 1 of bug 1351170 from Beta because it introduced this regression, and the proper fix for this regression is too risky to uplift to Beta. The other parts of bug 1351170 are not involved in the regression and do not need to be backed out.
dom/base/nsContentIterator.cpp
dom/events/ContentEventHandler.cpp
widget/tests/window_composition_text_querycontent.xul
--- a/dom/base/nsContentIterator.cpp
+++ b/dom/base/nsContentIterator.cpp
@@ -10,17 +10,16 @@
 #include "nsIContentIterator.h"
 #include "nsRange.h"
 #include "nsIContent.h"
 #include "nsCOMPtr.h"
 #include "nsTArray.h"
 #include "nsContentUtils.h"
 #include "nsINode.h"
 #include "nsCycleCollectionParticipant.h"
-#include "nsIParserService.h"
 
 using mozilla::DebugOnly;
 
 // couple of utility static functs
 
 ///////////////////////////////////////////////////////////////////////////
 // NodeToParentOffset: returns the node's parent and offset.
 //
@@ -367,27 +366,20 @@ nsContentIterator::Init(nsIDOMRange* aDO
     // No children (possibly a <br> or text node), or index is after last child.
 
     if (mPre) {
       // XXX: In the future, if start offset is after the last
       //      character in the cdata node, should we set mFirst to
       //      the next sibling?
 
       // Normally we would skip the start node because the start node is outside
-      // of the range in pre mode. However, if startIndx == 0, and the node is a
-      // non-container node (e.g. <br>), we don't skip the node in this case in
-      // order to address bug 1215798.
-      bool startIsContainer = true;
-      if (startNode->IsHTMLElement()) {
-        if (nsIParserService* ps = nsContentUtils::GetParserService()) {
-          nsIAtom* name = startNode->NodeInfo()->NameAtom();
-          ps->IsContainer(ps->HTMLAtomTagToId(name), startIsContainer);
-        }
-      }
-      if (!startIsData && (startIsContainer || startIndx)) {
+      // of the range in pre mode. However, if startIndx == 0, it means the node
+      // has no children, and the node may be <br> or something. We don't skip
+      // the node in this case in order to address bug 1215798.
+      if (!startIsData && startIndx) {
         mFirst = GetNextSibling(startNode);
         NS_WARNING_ASSERTION(mFirst, "GetNextSibling returned null");
 
         // Does mFirst node really intersect the range?  The range could be
         // 'degenerate', i.e., not collapsed but still contain no content.
         if (mFirst &&
             NS_WARN_IF(!NodeIsInTraversalRange(mFirst, mPre, startNode,
                                                startIndx, endNode, endIndx))) {
@@ -430,32 +422,24 @@ nsContentIterator::Init(nsIDOMRange* aDO
   bool endIsData = endNode->IsNodeOfType(nsINode::eDATA_NODE);
 
   if (endIsData || !endNode->HasChildren() || endIndx == 0) {
     if (mPre) {
       if (NS_WARN_IF(!endNode->IsContent())) {
         // Not much else to do here...
         mLast = nullptr;
       } else {
-        // If the end node is a non-container element and the end offset is 0,
+        // If the end node is an empty element and the end offset is 0,
         // the last element should be the previous node (i.e., shouldn't
         // include the end node in the range).
-        bool endIsContainer = true;
-        if (endNode->IsHTMLElement()) {
-          if (nsIParserService* ps = nsContentUtils::GetParserService()) {
-            nsIAtom* name = endNode->NodeInfo()->NameAtom();
-            ps->IsContainer(ps->HTMLAtomTagToId(name), endIsContainer);
-          }
-        }
-        if (!endIsData && !endIsContainer && !endIndx) {
+        if (!endIsData && !endNode->HasChildren() && !endIndx) {
           mLast = PrevNode(endNode);
           NS_WARNING_ASSERTION(mLast, "PrevNode returned null");
-          if (mLast && mLast != mFirst &&
-              NS_WARN_IF(!NodeIsInTraversalRange(mLast, mPre,
-                                                 mFirst, 0,
+          if (NS_WARN_IF(!NodeIsInTraversalRange(mLast, mPre,
+                                                 startNode, startIndx,
                                                  endNode, endIndx))) {
             mLast = nullptr;
           }
         } else {
           mLast = endNode->AsContent();
         }
       }
     } else {
--- a/dom/events/ContentEventHandler.cpp
+++ b/dom/events/ContentEventHandler.cpp
@@ -14,17 +14,16 @@
 #include "nsCaret.h"
 #include "nsCOMPtr.h"
 #include "nsContentUtils.h"
 #include "nsCopySupport.h"
 #include "nsFocusManager.h"
 #include "nsFontMetrics.h"
 #include "nsFrameSelection.h"
 #include "nsIContentIterator.h"
-#include "nsIParserService.h"
 #include "nsIPresShell.h"
 #include "nsISelection.h"
 #include "nsIFrame.h"
 #include "nsIObjectFrame.h"
 #include "nsLayoutUtils.h"
 #include "nsPresContext.h"
 #include "nsQueryObject.h"
 #include "nsRange.h"
@@ -2835,43 +2834,19 @@ ContentEventHandler::GetFlatTextLengthIn
 }
 
 nsresult
 ContentEventHandler::GetStartOffset(nsRange* aRange,
                                     uint32_t* aOffset,
                                     LineBreakType aLineBreakType)
 {
   MOZ_ASSERT(aRange);
-  // To match the "no skip start" hack in nsContentIterator::Init, when range
-  // offset is 0 and the range node is not a container, we have to assume the
-  // range _includes_ the node, which means the start offset should _not_
-  // include the node.
-  //
-  // For example, for this content: <br>abc, and range (<br>, 0)-("abc", 1), the
-  // range includes the linebreak from <br>, so the start offset should _not_
-  // include <br>, and the start offset should be 0.
-  //
-  // However, for this content: <p/>abc, and range (<p>, 0)-("abc", 1), the
-  // range does _not_ include the linebreak from <p> because <p> is a container,
-  // so the start offset _should_ include <p>, and the start offset should be 1.
-
-  nsINode* startNode = aRange->GetStartContainer();
-  bool startIsContainer = true;
-  if (startNode->IsHTMLElement()) {
-    if (nsIParserService* ps = nsContentUtils::GetParserService()) {
-      nsIAtom* name = startNode->NodeInfo()->NameAtom();
-      ps->IsContainer(ps->HTMLAtomTagToId(name), startIsContainer);
-    }
-  }
-  const NodePosition& startPos =
-    startIsContainer
-    ? NodePosition(startNode, aRange->StartOffset())
-    : NodePositionBefore(startNode, aRange->StartOffset());
   return GetFlatTextLengthInRange(
-           NodePosition(mRootContent, 0), startPos,
+           NodePosition(mRootContent, 0),
+           NodePosition(aRange->GetStartContainer(), aRange->StartOffset()),
            mRootContent, aOffset, aLineBreakType);
 }
 
 nsresult
 ContentEventHandler::AdjustCollapsedRangeMaybeIntoTextNode(nsRange* aRange)
 {
   MOZ_ASSERT(aRange);
   MOZ_ASSERT(aRange->Collapsed());
--- a/widget/tests/window_composition_text_querycontent.xul
+++ b/widget/tests/window_composition_text_querycontent.xul
@@ -4281,38 +4281,16 @@ function runQueryTextContentEventTest()
 
   // #16
   contenteditable.innerHTML = "a<blink>b</blink>c";
 
   result = synthesizeQueryTextContent(0, 3);
   is(result.text, "abc", "runQueryTextContentEventTest #16 (0, 3), \"" + contenteditable.innerHTML + "\"");
 }
 
-function runQuerySelectionEventTest()
-{
-  contenteditable.focus();
-
-  var selection = windowOfContenteditable.getSelection();
-
-  // #1
-  contenteditable.innerHTML = "<br/>a";
-  selection.setBaseAndExtent(contenteditable.firstChild, 0, contenteditable.lastChild, 1);
-  checkSelection(0, kLF + "a", "runQuerySelectionEventTest #1, \"" + contenteditable.innerHTML + "\"");
-
-  // #2
-  contenteditable.innerHTML = "<p></p><p>abc</p>";
-  selection.setBaseAndExtent(contenteditable.firstChild, 0, contenteditable.lastChild.firstChild, 1);
-  checkSelection(kLFLen, kLF + "a", "runQuerySelectionEventTest #2, \"" + contenteditable.innerHTML + "\"");
-
-  // #3
-  contenteditable.innerHTML = "<p>abc</p><p>def</p>";
-  selection.setBaseAndExtent(contenteditable.firstChild, 0, contenteditable.lastChild.firstChild, 1);
-  checkSelection(kLFLen, "abc" + kLF + "d", "runQuerySelectionEventTest #3, \"" + contenteditable.innerHTML + "\"");
-}
-
 function runQueryIMESelectionTest()
 {
   textarea.focus();
   textarea.value = "before  after";
   var startoffset = textarea.selectionStart = textarea.selectionEnd = "before ".length;
 
   if (!checkIMESelection("RawClause", false, 0, "", "runQueryIMESelectionTest: before starting composition") ||
       !checkIMESelection("SelectedRawClause", false, 0, "", "runQueryIMESelectionTest: before starting composition") ||
@@ -8137,17 +8115,16 @@ function* testBody()
   runCompositionCommitTest();
   runCompositionTest();
   runCompositionEventTest();
   runQueryTextRectInContentEditableTest();
   runCharAtPointTest(textarea, "textarea in the document");
   runCharAtPointAtOutsideTest();
   runSetSelectionEventTest();
   runQueryTextContentEventTest();
-  runQuerySelectionEventTest();
   runQueryIMESelectionTest();
   runQueryContentEventRelativeToInsertionPoint();
   yield* runIMEContentObserverTest();
   runCSSTransformTest();
   runBug722639Test();
   runBug1375825Test();
   runForceCommitTest();
   runNestedSettingValue();