1408262 - Don't clobber lastInserNode

Status: RESOLVED INVALID

(Core :: DOM: Editor, defect)

Description

[(no longer active)]

The correct child at offset variable to use here should
correspond to curNode.

Comment 1

[(no longer active)]

Attachment: Don't clobber lastInserNode

Comment 2

[(no longer active)]

I found this bug while debugging something unrelated...

Comment 3

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

Comment on attachment 8918118 [details] [diff] [review]
Don't clobber lastInserNode

I don't understand this change.

The original code before bug 1407447, lastInsertNode is updated with GetChildAt() when InsertNodeAtPoint() succeeded. Additionally, lastInsertNode is referred after the while loop.

https://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/editor/libeditor/HTMLEditorDataTransfer.cpp#550,556,563
>         // Assume failure means no legal parent in the document hierarchy,
>         // try again with the parent of curNode in the paste hierarchy.
>         nsCOMPtr<nsIDOMNode> parent;
>         while (NS_FAILED(rv) && curNode) {
>           curNode->GetParentNode(getter_AddRefs(parent));
>           if (parent && !TextEditUtils::IsBody(parent)) {
>             rv = InsertNodeAtPoint(parent, address_of(parentNode), &offsetOfNewNode, true);
>             if (NS_SUCCEEDED(rv)) {
>               bDidInsert = true;
>               insertedContextParent = parent;
>               lastInsertNode = GetChildAt(parentNode, offsetOfNewNode);
>             }
>           }
>           curNode = parent;
>         }
>       }
>       if (lastInsertNode) {

So, looks like that this patch really changes the original behavior. Do I misunderstand something? Or do you think the original behavior is wrong? (but you marked this bug as regression...)

Comment 4

[(no longer active)]

Hmm, maybe this wasn't a good idea, I'm not sure any more after reading your comment.  Sorry for the noise.