Bug 1351170 - 1. Correctly calculate start offset for non-text nodes; r=masayuki draft
authorJim Chen <nchen@mozilla.com>
Wed, 19 Jul 2017 14:29:59 -0400
changeset 611462 7d86b6cf04581f1cd71fa85f8c8586541b3a84e9
parent 611461 f15bf893f71fb75f4c8c4b3a17994c5531408368
child 611463 da0cc3073e4b8ad23c6f6eab42da5aa8b269cae9
push id69220
push userbmo:nchen@mozilla.com
push dateWed, 19 Jul 2017 18:31:25 +0000
reviewersmasayuki
bugs1351170
milestone56.0a1
Bug 1351170 - 1. Correctly calculate start offset for non-text nodes; r=masayuki When the start node is a non-container node (i.e. <br>), and the start offset is 0, we should not include a newline character for the node. For example, for this range, > <br/>hello > \___/ the start node/offset is (<br/>, 0) and end node/offset is ("hello", 1). The calculated range offset should be 0, and the range length should be 2: 1 for the <br/> newline character plus 1 for "h". The patch also ensures this behavior for pre-mode nsContentIterator, for both start and end node adjustments. For start nodes, we include any non-container nodes with offset 0 in the range. For end node, we exclude any non-container nodes with offset 0 from the range. MozReview-Commit-ID: Lt2tCLbapq7
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,16 +10,17 @@
 #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.
 //
@@ -366,20 +367,27 @@ 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, 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) {
+      // 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)) {
         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))) {
@@ -422,24 +430,32 @@ 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 an empty element and the end offset is 0,
+        // If the end node is a non-container 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).
-        if (!endIsData && !endNode->HasChildren() && !endIndx) {
+        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) {
           mLast = PrevNode(endNode);
           NS_WARNING_ASSERTION(mLast, "PrevNode returned null");
-          if (NS_WARN_IF(!NodeIsInTraversalRange(mLast, mPre,
-                                                 startNode, startIndx,
+          if (mLast && mLast != mFirst &&
+              NS_WARN_IF(!NodeIsInTraversalRange(mLast, mPre,
+                                                 mFirst, 0,
                                                  endNode, endIndx))) {
             mLast = nullptr;
           }
         } else {
           mLast = endNode->AsContent();
         }
       }
     } else {
--- a/dom/events/ContentEventHandler.cpp
+++ b/dom/events/ContentEventHandler.cpp
@@ -14,16 +14,17 @@
 #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"
@@ -2834,19 +2835,43 @@ 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),
-           NodePosition(aRange->GetStartContainer(), aRange->StartOffset()),
+           NodePosition(mRootContent, 0), startPos,
            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,16 +4281,38 @@ 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") ||
@@ -8115,16 +8137,17 @@ 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();