Closed Bug 1215798 Opened 4 years ago Closed 4 years ago

nsContentIterator::Init(nsIDOMRange*) ignores start node of the range if it's empty node like <br> when mPre == true

Categories

(Core :: DOM: Core & HTML, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(1 file, 1 obsolete file)

Although, I don't know who is an expert of nsContentIterator, I need to change nsContentIterator::Init(nsIDOMRange*).

If nsContentIterator is initialized as pre-order, it ignores the start node of the range if the node has no children. Therefore, with following range:

[<br>f]oo

i.e., { startNode: <br>, startOffset: 0, endNode: "foo", endOffset: 1 }, the iterator initialized with this range will start from endNode. I.e., the startNode, <br>, is never enumerated.
OS: Unspecified → All
Hardware: Unspecified → All
I'm not sure if you're the best person to review a patch of nsContentIterator, though. See comment 0 for the detail.
Attachment #8675422 - Flags: review?(bugs)
I tested the patch with creating a patch for bug 1213589. In the bug, I need to change NS_NewContentInterator() to NS_NewPreContentIterator() in ContentEventHandler for inserting line breakers at opening a tag.
Comment on attachment 8675422 [details] [diff] [review]
nsContentIterator::Init(nsIDOMRange*) should not skip empty start node when mPre is true

># HG changeset patch
># User Masayuki Nakano <masayuki@d-toybox.com>
># Parent  f1919e93e7044ecdbb94f52b657826ab556c44c6
>Bug 1215798 nsContentIterator::Init(nsIDOMRange*) should not skip empty start node when mPre is true r=
>
>diff --git a/dom/base/nsContentIterator.cpp b/dom/base/nsContentIterator.cpp
>--- a/dom/base/nsContentIterator.cpp
>+++ b/dom/base/nsContentIterator.cpp
>@@ -43,19 +43,19 @@ static bool
> NodeIsInTraversalRange(nsINode* aNode, bool aIsPreMode,
>                        nsINode* aStartNode, int32_t aStartOffset,
>                        nsINode* aEndNode, int32_t aEndOffset)
> {
>   if (!aStartNode || !aEndNode || !aNode) {
>     return false;
>   }
> 
>-  // If a chardata node contains an end point of the traversal range, it is
>+  // If a leaf node contains an end point of the traversal range, it is
>   // always in the traversal range.
>-  if (aNode->IsNodeOfType(nsINode::eDATA_NODE) &&
>+  if ((aNode->IsNodeOfType(nsINode::eDATA_NODE) || !aNode->HasChildren()) &&
>       (aNode == aStartNode || aNode == aEndNode)) {
Could we assert here that if aNode isn't a data node and doesn't have children and is eatch start or end node, 
the relevant offset is 0.



>       if (!startIsData) {
>-        mFirst = GetNextSibling(startNode);
>+        // 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 (!startIndx) {
>+          mFirst = startNode->AsContent();
Ah, this AsContent() is here because we use it elsewhere too. Silly code. Doesn't seem to handle Document.

So why we need  mFirst = startNode->AsContent(); twice?
Couldn't you just add || !startIndx to if (!startIsData) {?
Attachment #8675422 - Flags: review?(bugs) → review-
Looks like 0 and 1 are possible offset even if the node is empty element.
Attachment #8675422 - Attachment is obsolete: true
Attachment #8676065 - Flags: review?(bugs)
> Looks like 0 and 1 are possible offset even if the node is empty element.

Ah, maybe, I misunderstood. nsRange::SetStart() and nsRange::SetEnd() blocks lager offset than nsINode::Length(). And the callers set variables which are retrieved from nsRange instance. So, I should remove the check for 1. Please ignore this point at reviewing the patch.
Comment on attachment 8676065 [details] [diff] [review]
nsContentIterator::Init(nsIDOMRange*) should not skip empty start node when mPre is true

>+  if (aNode == aStartNode || aNode == aEndNode) {
>+    if (aNode->IsNodeOfType(nsINode::eDATA_NODE)) {
>+      return true; // text node or something
>+    }
>+    if (!aNode->HasChildren()) {
>+      MOZ_ASSERT(aNode != aStartNode || !aStartOffset || aStartOffset == 1,
>+        "aStartNode doesn't have children and not a data node, "
>+        "aStartOffset should be 0 or 1");
>+      MOZ_ASSERT(aNode != aEndNode || !aEndOffset || aEndOffset == 1,
>+        "aStartNode doesn't have children and not a data node, "
>+        "aStartOffset should be 0 or 1");


Yeah, I think we should be able assert here that if aNode is 
a*Node, a*Offset == 0

If that is not the case, I'm missing something important here :)
Attachment #8676065 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebeeeacb56e590cd5cac9906fef007eae9f3ed6d
Bug 1215798 nsContentIterator::Init(nsIDOMRange*) should not skip empty start node when mPre is true r=smaug
https://hg.mozilla.org/mozilla-central/rev/ebeeeacb56e5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
No longer depends on: 1224101
You need to log in before you can comment on or make changes to this bug.