Move side-effects on hover/active state out of hit-testing

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

In an effort to make hit-testing a mere algorithm and not cause side-effect we should move the call to Document::updateHoverActiveState() out of RenderView()::hitTest. Since Document::updateHoverActiveState() only has an effect when the HitTestRequest is not readonly, only a few places activating hit-testing will need to be modified.

The bug will be followed up by one refactoring updateHoverActiveState to no longer take a HitTestRequest as an argument, and once  bug #96908 is also closed, it will be possible to remove all information about which type of event triggered the hit-test from HitTestRequest.

[Allan Sandfeld Jensen]

Created attachment 166698 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 167981 [details]
Patch

[Julien Chaffraix]

Comment on attachment 167981 [details]
Patch

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

Beth is probably a better reviewer than I am for hit-testing.

> Source/WebCore/ChangeLog:9
> +        a hit-test. This will help enable further clean-up and optimizations.

There is one big issue with that: if the caller doesn't call updateHoverActiveState then you have a bug, integrating it with hit-testing guaranteed that it was called. From this perspective, I don't see the code as written as an improvement as it is way more error-prone than the old one.

> Source/WebCore/ChangeLog:23
> +        (WebCore::RenderView::hitTest):

Could you please provide *some* explanations.

> Source/WebCore/dom/Document.cpp:5817
>      if (request.touchRelease()) {
> -        if (oldHoverNode) {
> -            for (RenderObject* curr = oldHoverNode->renderer(); curr; curr = curr->hoverAncestor()) {
> -                if (curr->node() && !curr->isText())
> -                    curr->node()->setHovered(false);
> -            }
> -            setHoverNode(0);
> -        }
> -        // A touch release can not set new hover or active target.
> -        return;
> +        // A touch release does not set a new hover target.
> +        innerNode = 0;
>      }

This should have been explained, I was going to comment that this was a change in behavior.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 167981 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167981&action=review
> 
> Beth is probably a better reviewer than I am for hit-testing.
> 
> > Source/WebCore/ChangeLog:9
> > +        a hit-test. This will help enable further clean-up and optimizations.
> 
> There is one big issue with that: if the caller doesn't call updateHoverActiveState then you have a bug, integrating it with hit-testing guaranteed that it was called. From this perspective, I don't see the code as written as an improvement as it is way more error-prone than the old one.
> 
Well, even in the old case you still needed to explicitly call Document::updateStyleIfNeeded()  to get the style change. Now updateStyleIfNeeded() is called from updateHoverActiveState(), so there is still only one function you need to call after a non-readonly hitTest. Secondly most calls to hitTest are not readonly.


> > Source/WebCore/ChangeLog:23
> > +        (WebCore::RenderView::hitTest):
> 
> Could you please provide *some* explanations.
> 
> > Source/WebCore/dom/Document.cpp:5817
> >      if (request.touchRelease()) {
> > -        if (oldHoverNode) {
> > -            for (RenderObject* curr = oldHoverNode->renderer(); curr; curr = curr->hoverAncestor()) {
> > -                if (curr->node() && !curr->isText())
> > -                    curr->node()->setHovered(false);
> > -            }
> > -            setHoverNode(0);
> > -        }
> > -        // A touch release can not set new hover or active target.
> > -        return;
> > +        // A touch release does not set a new hover target.
> > +        innerNode = 0;
> >      }
> 
> This should have been explained, I was going to comment that this was a change in behavior.

Right. The old code was mostly unnecessary, since the rest of the function does what is needed if innerNode is 0, but yes that should be explained.

[Allan Sandfeld Jensen]

(In reply to comment #4)
Secondly most calls to hitTest are not readonly.

Sorry. Most calls are readonly.

[Kenneth Rohde Christiansen]

Comment on attachment 167981 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        This patch changes hit-testing so that it no longer has any side-effects and is really purely
> +        a hit-test. This will help enable further clean-up and optimizations.
> +
> +        No change in functionality. No new tests.

I think the changelog should explain the long term goal with this change and why it is important

[Mike West]

Created attachment 191208 [details]
Patch

[Mike West]

(In reply to comment #7)
> Created an attachment (id=191208) [details]
> Patch

I talked with carewolf briefly this morning, and grabbed his patch in this hopes of getting this moving again. The attached patch rebases his last patch on top of trunk, with slight adjustments to both the explanation in the ChangeLog and the logic for hover/active in documents between the hit Element and the current document.

I've also moved the bit jchaffraix noted in https://bugs.webkit.org/show_bug.cgi?id=98168#c3 out to a separate bug, as it was tangential to the focus of this patch (bug 111303).

My interest in getting this going is to make implementation of mouseenter/mouseleave events simpler and more performant (bug 18930). It's one of those things that jQuery is currently working around with about ~20 lines of code that they'd love to drop.

[Allan Sandfeld Jensen]

Comment on attachment 191208 [details]
Patch

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

Wrong assert. Note you need another reviewer once you have added my name to the Changelog.

> Source/WebCore/ChangeLog:6
> +
> +        Reviewed by NOBODY (OOPS!).

When continuing other peoples patches you still need to mention them in the Changelog. For instance:
Based on patch by Allan Sandfeld Jensen.

> Source/WebCore/ChangeLog:25
> +        Document::updateHoverActiveState is currently called during hit testing
> +        to update the hover and active states of elements effected by mouse
> +        movements (or their keyboard equivalents). This conflates the hit test
> +        algorithm itself with side-effects associated with specific use-cases.
> +
> +        This conflation makes it very difficult to reuse the hover/active logic
> +        for things other than hit testing. 'mouseenter'/'mouseleave' events[1]
> +        are one example of a feature that would be simple to implement on top of
> +        this existing logic if we split it out from the hit testing path, and
> +        instead call it explicitly when necessary. An explicit split between
> +        hit testing and its side-effects will also enable us to simplify
> +
> +        This patch drops the call to Document::updateHoverActiveState from
> +        RenderView::hitTest, and adjusts the three call-sites in EventHandler
> +        to explicitly call out to it rather than Document::updateStyleIfNeeded.
> +        The latter call is still necessary but has been folded into
> +        updateHoverActiveState, as the former is never called without calling
> +        the latter.

Great!

> Source/WebCore/dom/Document.cpp:5847
> +    ASSERT(request.readOnly());

ASSERT(!request.readOnly());

[Allan Sandfeld Jensen]

(In reply to comment #8)
> I've also moved the bit jchaffraix noted in https://bugs.webkit.org/show_bug.cgi?id=98168#c3 out to a separate bug, as it was tangential to the focus of this patch (bug 111303).
> 
Note you need to call updateStyleIfNeeded() before that return then.

[Mike West]

(In reply to comment #9)
> > Source/WebCore/ChangeLog:6
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> When continuing other peoples patches you still need to mention them in the Changelog. For instance:
> Based on patch by Allan Sandfeld Jensen.

Ugh. Apologies. That's simple idiocy on my part.

> > Source/WebCore/dom/Document.cpp:5847
> > +    ASSERT(request.readOnly());
> 
> ASSERT(!request.readOnly());

That too. *facepalm*

[Mike West]

Created attachment 191217 [details]
Patch

[Julien Chaffraix]

Comment on attachment 191217 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:5899
> +        updateStyleIfNeeded();

This really looks like it should be some RAII pattern or else the next person adding a return will skip updating the style.

[Mike West]

Comment on attachment 191217 [details]
Patch

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

Thanks!

>> Source/WebCore/dom/Document.cpp:5899
>> +        updateStyleIfNeeded();
> 
> This really looks like it should be some RAII pattern or else the next person adding a return will skip updating the style.

This piece has actually been optimized away in another patch. When I rebase this for landing, it'll be gone. :)

[Mike West]

Created attachment 192048 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 192048 [details]
Patch for landing

Clearing flags on attachment: 192048

Committed r145126: <http://trac.webkit.org/changeset/145126>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

(In reply to comment #14)
> (From update of attachment 191217 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191217&action=review
> 
> Thanks!
> 
> >> Source/WebCore/dom/Document.cpp:5899
> >> +        updateStyleIfNeeded();
> > 
> > This really looks like it should be some RAII pattern or else the next person adding a return will skip updating the style.
> 
> This piece has actually been optimized away in another patch. When I rebase this for landing, it'll be gone. :)

He was not asking for updateStyleIfNeeded() to be removed, but to make it more reliable, and less likely to be missed in future returns. I am not sure we should worry about future returns, even this one should be removed soon, but removing updateStyleIfNeeded() just means you have bug now.

[Mike West]

(In reply to comment #18)
> (In reply to comment #14)
> > (From update of attachment 191217 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191217&action=review
> > 
> > Thanks!
> > 
> > >> Source/WebCore/dom/Document.cpp:5899
> > >> +        updateStyleIfNeeded();
> > > 
> > > This really looks like it should be some RAII pattern or else the next person adding a return will skip updating the style.
> > 
> > This piece has actually been optimized away in another patch. When I rebase this for landing, it'll be gone. :)
> 
> He was not asking for updateStyleIfNeeded() to be removed, but to make it more reliable, and less likely to be missed in future returns. I am not sure we should worry about future returns, even this one should be removed soon, but removing updateStyleIfNeeded() just means you have bug now.

I talked about this briefly with careworlf on IRC, but for clarity here: a patch for bug 111303 landed before this patch. The whole return block was removed, so it was no longer necessary to updateStyle here.