Bug 63816 – Remove calls to deprecatedNode in EventHandler.cpp

• Component: HTML Editing
• Status: RESOLVED LATER

[Ryosuke Niwa]

Refactoring to remove calls to deprecateNode in EventHandler.cpp

[Ryosuke Niwa]

Created attachment 99484 [details]
Patch

[Darin Adler]

Comment on attachment 99484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99484&action=review

> Source/WebCore/ChangeLog:14
> +        (WebCore::EventHandler::updateSelectionForMouseDrag): Replaced the call to deprecatedNode by a call to
> +        containerNode because the node is only used to find the containing block via its renderer.

I don’t understand why this explanation makes the change OK. Couldn’t the container node have a different containing block than the anchor node?

> Source/WebCore/page/EventHandler.cpp:339
> -        if (pos.isNotNull() && pos.deepEquivalent().deprecatedNode()->isDescendantOf(URLElement))
> +        if (pos.isNotNull() && pos.deepEquivalent().containerNode()->isDescendantOf(URLElement))

I haven’t carefully watched other deprecatedNode/containerNode conversions in the past, but this one changes behavior.

This will now give the wrong answer for a position that is a BeforeAnchor or AfterAnchor of a node that is child of the URL element. The containerNode function will return the URL element itself, and isDescandantOf will return false.

The old expression would have been true in that case.

The old code already seems wrong for BeforeChildren and AfterChildren positions.

The change in behavior may be harmless because positionForPoint never returns a BeforeAnchor or AfterAnchor position.

The code doesn’t make much sense. Why is it interesting if the container node is a descendant of the URL element? I think the real question is whether the position is inside the URL element, and we should have a function that correctly asks that question.

We’d probably need to change this from deprecatedNode->isDescendantOf(URLElement) to URLElement->contains(containerNode) to make it make sense.

[Ryosuke Niwa]

Comment on attachment 99484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99484&action=review

>> Source/WebCore/page/EventHandler.cpp:339
>> +        if (pos.isNotNull() && pos.deepEquivalent().containerNode()->isDescendantOf(URLElement))
> 
> I haven’t carefully watched other deprecatedNode/containerNode conversions in the past, but this one changes behavior.
> 
> This will now give the wrong answer for a position that is a BeforeAnchor or AfterAnchor of a node that is child of the URL element. The containerNode function will return the URL element itself, and isDescandantOf will return false.
> 
> The old expression would have been true in that case.
> 
> The old code already seems wrong for BeforeChildren and AfterChildren positions.
> 
> The change in behavior may be harmless because positionForPoint never returns a BeforeAnchor or AfterAnchor position.
> 
> The code doesn’t make much sense. Why is it interesting if the container node is a descendant of the URL element? I think the real question is whether the position is inside the URL element, and we should have a function that correctly asks that question.
> 
> We’d probably need to change this from deprecatedNode->isDescendantOf(URLElement) to URLElement->contains(containerNode) to make it make sense.

Right, all these deprecatedNode/containerNode conversions will correct behavior.  In this particular case, we're interested in whether a URL element contains a Position or not.  And in that sense, it makes no sense to care about positions that are before or after an anchor element.

[Ryosuke Niwa]

Created attachment 99504 [details]
Fixed per comment

[Darin Adler]

Comment on attachment 99504 [details]
Fixed per comment

View in context: https://bugs.webkit.org/attachment.cgi?id=99504&action=review

> Source/WebCore/ChangeLog:15
> +        (WebCore::EventHandler::updateSelectionForMouseDrag): Replaced the call to deprecatedNode by a call to
> +        containerNode because the node is only used to find the containing block via its renderer.

I don’t understand why this explanation makes the change OK. Couldn’t the container node have a different containing block than the anchor node?

[Ryosuke Niwa]

(In reply to comment #5)
> (From update of attachment 99504 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99504&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        (WebCore::EventHandler::updateSelectionForMouseDrag): Replaced the call to deprecatedNode by a call to
> > +        containerNode because the node is only used to find the containing block via its renderer.
> 
> I don’t understand why this explanation makes the change OK. Couldn’t the container node have a different containing block than the anchor node?

Yes.  I'm correcting the behavior here because when we're deciding whether a position is inside an element or not, we shouldn't be starting its search from the anchor node.

i.e. if a position is before anchor, we shouldn't consider that position to be inside the anchor.  We have this bug all over the place in editing code, and the bug 52099 is all about fixing these bugs.

I've written an article about this.  Please take a look at "Deprecated node()" in https://rniwa.com/2011-06-26/position-and-anchor-types/

[Ryosuke Niwa]

Ping reviewers.

[Ryosuke Niwa]

Ping reviewers again