1407305 - Avoid using nsINode::GetChildAt() in EditorBase::GetPriorNode() and EditorBase::GetNextNode()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[(no longer active)]

Attachment: Part 1: Add a aChildAtOffset argument to HTMLEditor::GetPriorHTMLNode()

Comment 2

[(no longer active)]

Attachment: Part 2: Add a aChildAtOffset argument to HTMLEditor::GetNextHTMLNode()

Comment 3

[(no longer active)]

Attachment: Part 3: Avoid using GetChildAt() in EditorBase::GetPriorNode()

Comment 4

[(no longer active)]

Attachment: Part 4: Avoid using GetChildAt() in EditorBase::GetNextNode()

Comment 5

[(no longer active)]

Attachment: Part 2: Add a aChildAtOffset argument to HTMLEditor::GetNextHTMLNode()

Comment 6

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

Comment on attachment 8917052 [details] [diff] [review]
Part 1: Add a aChildAtOffset argument to HTMLEditor::GetPriorHTMLNode()

Although, this review is not requested with MozReview. So, I cannot check other blocks which are not shown in this diff easily. Therefore, I don't check whether you forget to add lines to modify |child| variable around |offset| in each method which calls GetPrioerHTMLNode(). Please double check it before landing by yourself.

>@@ -5545,42 +5554,45 @@ HTMLEditRules::GetPromotedPoint(RulesEndpoint aWhere,
>   // aWhere == kEnd
>   // some special casing for text nodes
>   if (node->IsNodeOfType(nsINode::eTEXT)) {
>     if (!node->GetParentNode()) {
>       // Okay, can't promote any further
>       return EditorDOMPoint(node, offset);
>     }
>     // want to be after the text node
>     offset = 1 + node->GetParentNode()->IndexOf(node);
>+    child = node;
>     node = node->GetParentNode();

The offset includes |+1|. So, I guess that the child should be node->GetNextSibling().

>   }
> 
>   // look ahead through any further inline nodes that aren't across a <br> from
>   // us, and that are enclosed in the same block.
>   nsCOMPtr<nsIContent> nextNode =
>     htmlEditor->GetNextHTMLNode(node, offset, true);
> 
>   while (nextNode && !IsBlockNode(*nextNode) && nextNode->GetParentNode()) {
>     offset = 1 + nextNode->GetParentNode()->IndexOf(nextNode);
>+    child = node;

ditto.

>@@ -5614,16 +5626,17 @@ HTMLEditRules::GetPromotedPoint(RulesEndpoint aWhere,
>     // Don't walk past the editable section. Note that we need to check before
>     // walking up to a parent because we need to return the parent object, so
>     // the parent itself might not be in the editable area, but it's OK.
>     if (!htmlEditor->IsDescendantOfEditorRoot(node) &&
>         !htmlEditor->IsDescendantOfEditorRoot(parent)) {
>       break;
>     }
> 
>+    child = node;
>     node = parent;
>     // we want to be AFTER nearNode
>     offset = parentOffset + 1;

ditto.

>@@ -6537,17 +6551,17 @@ HTMLEditRules::ReturnInParagraph(Selection* aSelection,
>     sibling = mHTMLEditor->CreateBR(parent, offset);
>     if (newSelNode) {
>       // We split the parent after the br we've just inserted.
>       selNode = GetAsDOMNode(parent);
>       selOffset = offset + 1;
>     }
>   }
>   *aHandled = true;
>-  return SplitParagraph(aPara, sibling, aSelection, address_of(selNode), &selOffset);
>+  return SplitParagraph(GetAsDOMNode(aPara), sibling, aSelection, address_of(selNode), &selOffset);

nit: too long line. please break after |aSelection,|.

>@@ -7528,16 +7542,17 @@ HTMLEditRules::AdjustSelection(Selection* aSelection,
> 
>   // get the (collapsed) selection location
>   nsCOMPtr<nsINode> selNode, temp;
>   int32_t selOffset;
>   nsresult rv =
>     EditorBase::GetStartNodeAndOffset(aSelection,
>                                       getter_AddRefs(selNode), &selOffset);
>   NS_ENSURE_SUCCESS(rv, rv);
>+  nsIContent* child = aSelection->GetRangeAt(0)->GetChildAtStartOffset();

I wonder, if GetStartNodeAndOffset() returns RawRangeBoundary, we don't need to call GetChildAtStartOffset() at every caller. However, it must cause changing a lot of lines. So, shouldn't be scope of this bug.

Comment 7

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

Comment on attachment 8917175 [details] [diff] [review]
Part 2: Add a aChildAtOffset argument to HTMLEditor::GetNextHTMLNode()

>@@ -5579,21 +5582,21 @@ HTMLEditRules::GetPromotedPoint(RulesEndpoint aWhere,
>     offset = 1 + node->GetParentNode()->IndexOf(node);
>     child = node;
>     node = node->GetParentNode();
>   }
> 
>   // look ahead through any further inline nodes that aren't across a <br> from
>   // us, and that are enclosed in the same block.
>   nsCOMPtr<nsIContent> nextNode =
>-    htmlEditor->GetNextHTMLNode(node, offset, true);
>+    htmlEditor->GetNextHTMLNode(node, offset, child, true);
> 
>   while (nextNode && !IsBlockNode(*nextNode) && nextNode->GetParentNode()) {
>     offset = 1 + nextNode->GetParentNode()->IndexOf(nextNode);
>-    child = node;
>+    child = nextNode->GetNextSibling();

Oh, you're fixing the bug of part 1 in part 2... Please do this in part 1.

>@@ -5606,42 +5609,42 @@ HTMLEditRules::GetPromotedPoint(RulesEndpoint aWhere,
>     // Don't walk past the editable section. Note that we need to check before
>     // walking up to a parent because we need to return the parent object, so
>     // the parent itself might not be in the editable area, but it's OK.
>     if (!htmlEditor->IsDescendantOfEditorRoot(node) &&
>         !htmlEditor->IsDescendantOfEditorRoot(parent)) {
>       break;
>     }
> 
>-    child = node;
>+    child = node->GetNextSibling();

Ditto.

>@@ -7543,23 +7546,24 @@ HTMLEditRules::AdjustSelection(Selection* aSelection,
> 
>   // get the (collapsed) selection location
>   nsCOMPtr<nsINode> selNode, temp;
>   int32_t selOffset;
>   nsresult rv =
>     EditorBase::GetStartNodeAndOffset(aSelection,
>                                       getter_AddRefs(selNode), &selOffset);
>   NS_ENSURE_SUCCESS(rv, rv);
>-  nsIContent* child = aSelection->GetRangeAt(0)->GetChildAtStartOffset();
>+  nsINode* child = aSelection->GetRangeAt(0)->GetChildAtStartOffset();

Really sad...

>@@ -7673,34 +7677,35 @@ HTMLEditRules::AdjustSelection(Selection* aSelection,
>   }
>   return NS_OK;
> }
> 
> 
> nsresult
> HTMLEditRules::FindNearSelectableNode(nsINode* aSelNode,
>                                       int32_t aSelOffset,
>-                                      nsIContent* aChildAtOffset,
>+                                      nsINode* aChildAtOffset,

Sad...

Comment 8

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7)
> >@@ -7543,23 +7546,24 @@ HTMLEditRules::AdjustSelection(Selection* aSelection,
> > 
> >   // get the (collapsed) selection location
> >   nsCOMPtr<nsINode> selNode, temp;
> >   int32_t selOffset;
> >   nsresult rv =
> >     EditorBase::GetStartNodeAndOffset(aSelection,
> >                                       getter_AddRefs(selNode), &selOffset);
> >   NS_ENSURE_SUCCESS(rv, rv);
> >-  nsIContent* child = aSelection->GetRangeAt(0)->GetChildAtStartOffset();
> >+  nsINode* child = aSelection->GetRangeAt(0)->GetChildAtStartOffset();
> 
> Really sad...
> 
> >@@ -7673,34 +7677,35 @@ HTMLEditRules::AdjustSelection(Selection* aSelection,
> >   }
> >   return NS_OK;
> > }
> > 
> > 
> > nsresult
> > HTMLEditRules::FindNearSelectableNode(nsINode* aSelNode,
> >                                       int32_t aSelOffset,
> >-                                      nsIContent* aChildAtOffset,
> >+                                      nsINode* aChildAtOffset,
> 
> Sad...

FWIW while testing I found out that we actually observe document nodes here from EditorBase::GetNodeLocation(), so using nsIContent* was just the only option here.

Comment 9

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> >@@ -5545,42 +5554,45 @@ HTMLEditRules::GetPromotedPoint(RulesEndpoint aWhere,
> >   // aWhere == kEnd
> >   // some special casing for text nodes
> >   if (node->IsNodeOfType(nsINode::eTEXT)) {
> >     if (!node->GetParentNode()) {
> >       // Okay, can't promote any further
> >       return EditorDOMPoint(node, offset);
> >     }
> >     // want to be after the text node
> >     offset = 1 + node->GetParentNode()->IndexOf(node);
> >+    child = node;
> >     node = node->GetParentNode();
> 
> The offset includes |+1|. So, I guess that the child should be
> node->GetNextSibling().

FWIW this one was actually correct since the node here is a text node.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa6c322a98b0
Part 1: Add a aChildAtOffset argument to HTMLEditor::GetPriorHTMLNode(); r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc1c6227e652
Part 2: Add a aChildAtOffset argument to HTMLEditor::GetNextHTMLNode(); r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6f9b6b56eb
Part 3: Avoid using GetChildAt() in EditorBase::GetPriorNode(); r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/e22de36807c6
Part 4: Avoid using GetChildAt() in EditorBase::GetNextNode(); r=masayuki

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa6c322a98b0
https://hg.mozilla.org/mozilla-central/rev/dc1c6227e652
https://hg.mozilla.org/mozilla-central/rev/8c6f9b6b56eb
https://hg.mozilla.org/mozilla-central/rev/e22de36807c6