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

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a066078c290

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2424129c2355

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: nsContentIterator::Init(nsIDOMRange*) should not skip empty start node when mPre is true

I'm not sure if you're the best person to review a patch of nsContentIterator, though. See comment 0 for the detail.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 5

[Olli Pettay [:smaug][[e-mail]]]

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 <[e-mail]>
># 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) {?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91eb3d045807

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: nsContentIterator::Init(nsIDOMRange*) should not skip empty start node when mPre is true

Looks like 0 and 1 are possible offset even if the node is empty element.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> 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 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b0a713e2300

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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 :)

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ebeeeacb56e5