Bug 1319660 - 2. Use previous node instead of sibling when adjusting last node; r=masayuki r=smaug
authorJim Chen <nchen@mozilla.com>
Wed, 04 Jan 2017 14:46:10 -0500
changeset 327977 93353b53a706c7dc9b559d6224fd7f1f4ca50ccb
parent 327976 9a7c2edd54b8dd6c3d6471560ac1b096b2955536
child 327978 d506d3c193c90dc85d57519f8d822c3919b12322
push id31160
push userphilringnalda@gmail.com
push dateThu, 05 Jan 2017 02:33:44 +0000
treeherdermozilla-central@f13abb8ba9f3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmasayuki, smaug
bugs1319660
milestone53.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 1319660 - 2. Use previous node instead of sibling when adjusting last node; r=masayuki r=smaug nsContentIterator in pre mode adjusts its last node if the node is a childless node like <br>. However, right now it's using GetPrevSibling, which can lead to error in some edge cases such as: <p></p><div><br></div> In this case, if the last node is <br> with offset 0, GetPrevSibling will return <p> because <p> is <br>'s parent's previous sibling, and the last node will be set to <p>. However, the correct last node in this case is <div>, because <br> with offset 0 refers to the position to the left of <br>, which is <div> with offset 0. In this case, PrevNode returns the correct <div> value, so we should set the last node to the result of PrevNode. For the first node, for a childless node in pre mode, GetNextSibling and NextNode are the same, so there is no bug in this case. Nevertheless, this patch changes the call to NextNode to be consistent with calling PrevNode for the last node.
dom/base/nsContentIterator.cpp
--- a/dom/base/nsContentIterator.cpp
+++ b/dom/base/nsContentIterator.cpp
@@ -369,18 +369,18 @@ nsContentIterator::Init(nsIDOMRange* aDO
       // XXX: In the future, if start offset is after the last
       //      character in the cdata node, should we set mFirst to
       //      the next sibling?
 
       // If the node has no child, the child may be <br> or something.
       // So, we shouldn't skip the empty node if the start offset is 0.
       // In other words, if the offset is 1, the node should be ignored.
       if (!startIsData && startIndx) {
-        mFirst = GetNextSibling(startNode);
-        NS_WARNING_ASSERTION(mFirst, "GetNextSibling returned null");
+        mFirst = NextNode(startNode);
+        NS_WARNING_ASSERTION(mFirst, "NextNode 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))) {
           mFirst = nullptr;
         }
@@ -425,18 +425,18 @@ nsContentIterator::Init(nsIDOMRange* aDO
       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,
         // the last element should be the previous node (i.e., shouldn't
         // include the end node in the range).
         if (!endIsData && !endNode->HasChildren() && !endIndx) {
-          mLast = GetPrevSibling(endNode);
-          NS_WARNING_ASSERTION(mLast, "GetPrevSibling returned null");
+          mLast = PrevNode(endNode);
+          NS_WARNING_ASSERTION(mLast, "PrevNode returned null");
           if (NS_WARN_IF(!NodeIsInTraversalRange(mLast, mPre,
                                                  startNode, startIndx,
                                                  endNode, endIndx))) {
             mLast = nullptr;
           }
         } else {
           mLast = endNode->AsContent();
         }