Bug 46015 – Implement shadow DOM-aware event targeting and introduce EventContext to track the context of each event dispatch.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Implement shadow DOM-aware event targeting and introduce EventContext to track the context of each event dispatch.

[Dimitri Glazkov (Google)]

Created attachment 67976 [details]
End of week checkpoint

[Dimitri Glazkov (Google)]

Notes to self:

- use templates to combine EventContext and WindowEventContext
- study SVG event firing and simplify
- add project changes for all platforms

[Dimitri Glazkov (Google)]

Created attachment 69537 [details]
Patch

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> Notes to self:
> 
> - use templates to combine EventContext and WindowEventContext

This seemed like a good idea, but turned cumbersome very quickly.

> - study SVG event firing and simplify

I chickened out here and did only a very limited part, leaving out a FIXME. SVG is very, very hairy.

> - add project changes for all platforms

Done.

[WebKit Review Bot]

Attachment 69537 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4230038

[Dimitri Glazkov (Google)]

Comment on attachment 69537 [details]
Patch

Will check on Chromium borkage...

[Dimitri Glazkov (Google)]

Created attachment 69640 [details]
Patch

[Dimitri Glazkov (Google)]

Created attachment 69665 [details]
Patch

[Darin Adler]

Comment on attachment 69665 [details]
Patch

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

Design looks good. Are you sure you have enough test coverage?

> WebCore/dom/EventContext.cpp:52
> +Node* EventContext::node() const
> +{
> +    return m_node.get();
> +}
> +
> +EventTarget* EventContext::target() const
> +{
> +    return m_target.get();
> +}

I’d like to see these inlined.

> WebCore/dom/EventContext.cpp:61
> +WindowEventContext::WindowEventContext(Event* event, Node* node, const Vector<EventContext>& ancestors)

This class should go into a separate source file.

It’s a little strange to pass in a vector simply so we can extract the last element of it.

> WebCore/dom/EventContext.cpp:89
> +DOMWindow* WindowEventContext::window() const
> +{
> +    return m_window.get();
> +}
> +
> +EventTarget* WindowEventContext::target() const
> +{
> +    return m_target.get();
> +}

I’d like to see these inlined.

> WebCore/dom/EventContext.h:35
> +class ContainerNode;

Not used in this header.

> WebCore/dom/Node.cpp:2553
> +    bool skip = false;
> +    do {

I think that shouldSkipNextXXX (whatever XXX is) is a better boolean local variable name than “skip”. Since skip is a verb it’s hard to know what it means.

do while (true) is not the usual idiom for an infinite loop. I’d prefer that you use while (true), which is the more usual one in this project at least.

[Dimitri Glazkov (Google)]

Comment on attachment 69665 [details]
Patch

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

>> WebCore/dom/EventContext.cpp:52
>> +}
> 
> I’d like to see these inlined.

Done.

>> WebCore/dom/EventContext.cpp:61
>> +WindowEventContext::WindowEventContext(Event* event, Node* node, const Vector<EventContext>& ancestors)
> 
> This class should go into a separate source file.
> 
> It’s a little strange to pass in a vector simply so we can extract the last element of it.

Agreed. Split up the file and moved resolving of top ancestor back into Node.cpp

>> WebCore/dom/EventContext.cpp:89
>> +}
> 
> I’d like to see these inlined.

Done.

>> WebCore/dom/EventContext.h:35
>> +class ContainerNode;
> 
> Not used in this header.

Removed.

>> WebCore/dom/Node.cpp:2553
>> +    do {
> 
> I think that shouldSkipNextXXX (whatever XXX is) is a better boolean local variable name than “skip”. Since skip is a verb it’s hard to know what it means.
> 
> do while (true) is not the usual idiom for an infinite loop. I’d prefer that you use while (true), which is the more usual one in this project at least.

Done. Sorry, "do while" stuff was due to me mucking with trying to not make it infinite at first :)

[Dimitri Glazkov (Google)]

(In reply to comment #9)
> (From update of attachment 69665 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69665&action=review
> 
> Design looks good. Are you sure you have enough test coverage?

There's pretty good coverage for events fired from shadow DOM already, but it wouldn't hurt to write more tests specifically around target/currentTarget and how they change when going in/out of shadow DOM. I will tackle this next.

[Dimitri Glazkov (Google)]

Created attachment 72351 [details]
Patch for landing.

[Early Warning System Bot]

Attachment 72351 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4787068

[Dimitri Glazkov (Google)]

Created attachment 72360 [details]
Patch for landing for realz.

[Darin Adler]

Comment on attachment 72360 [details]
Patch for landing for realz.

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

> LayoutTests/ChangeLog:8
> +        Tuned the test to better reflect its point: the even should indeed fire (it used to be swallowed),

even -> event

> LayoutTests/ChangeLog:9
> +        but it's target should be a non-shadow node.

it's -> its

> LayoutTests/fast/events/shadow-boundary-crossing.html:9
> +            success = event.target.parentNode;

If the aim here is to check that this is a non-shadow node, maybe we should check for the specific correct node, instead of allowing any node with a parent.

> WebCore/dom/EventContext.h:40
> +    EventContext(Node*, EventTarget* currentTarget, EventTarget* target);

Our usual style is to use PassRefPtr for arguments when the object will take a ref to them, even if the object is not the “sole owner”. I think you should follow that here.

Can the node be a ContainerNode* instead of Node*? Do we construct these with arbitrary nodes?

> WebCore/dom/Node.cpp:1445
> +ContainerNode* Node::parentOrHostNode()

Candidate for inlining?

> WebCore/dom/Node.cpp:2516
> -void Node::eventAncestors(Vector<RefPtr<ContainerNode> > &ancestors)
> +void Node::eventAncestors(Vector<EventContext> &ancestors, EventTarget* originalTarget)

Misplaced "&" here. It's supposed to be next to the type, not the argument name.

Functions like this one, that use an out argument, should typically have “get” in their name. Not sure why this didn’t have that already.

> WebCore/dom/Node.cpp:2544
> +    };

Stray semicolon here. It should be removed.

> WebCore/dom/Node.cpp:2575
> +    RefPtr<EventTarget> originalTarget(event->target());

I prefer the "=" style syntax to the constructor style in cases like this. What do you think?

> WebCore/dom/Node.cpp:2618
> +            goto doneDispatching;

This could just be a “break” and probably should be. The reason we use goto above is that it’s nested inside two loops.

> WebCore/dom/Node.h:216
>      // FIXME: Should be named getEventAncestors.
> -    void eventAncestors(Vector<RefPtr<ContainerNode> > &ancestors);
> +    void eventAncestors(Vector<EventContext> &ancestors, EventTarget*);

As I mentioned above, misplaced & here, and the name should be changed as the FIXME says.

> WebCore/dom/WindowEventContext.cpp:50
> +        topLevelContainer = topEventContext->node();
> +        targetForWindowEvents = topEventContext->target();
> +    }

This might read better like this:

    EventTarget* targetForWindowEvents = topEventContext ? topEventContext->node() : node;
    Node* topLevelContainer = topEventContext ? topEventContext->target() : node;

Not sure. I suppose it depends on whether you are comfortable with ?: or not.

> WebCore/dom/WindowEventContext.h:43
> +    WindowEventContext(Event*, Node*, const EventContext*);

Same comment as above about PassRefPtr.

[Eric Seidel (no email)]

Attachment 72351 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4795092

[Dimitri Glazkov (Google)]

Comment on attachment 72360 [details]
Patch for landing for realz.

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

>> LayoutTests/ChangeLog:8
>> +        Tuned the test to better reflect its point: the even should indeed fire (it used to be swallowed),
> 
> even -> event

DOH.

>> LayoutTests/ChangeLog:9
>> +        but it's target should be a non-shadow node.
> 
> it's -> its

Ouch. I apologize.

>> LayoutTests/fast/events/shadow-boundary-crossing.html:9
>> +            success = event.target.parentNode;
> 
> If the aim here is to check that this is a non-shadow node, maybe we should check for the specific correct node, instead of allowing any node with a parent.

Sure! Fixed.

>> WebCore/dom/EventContext.h:40
>> +    EventContext(Node*, EventTarget* currentTarget, EventTarget* target);
> 
> Our usual style is to use PassRefPtr for arguments when the object will take a ref to them, even if the object is not the “sole owner”. I think you should follow that here.
> 
> Can the node be a ContainerNode* instead of Node*? Do we construct these with arbitrary nodes?

Sure, changed to PassRefPtr.

The reason I ended up using Node* was not a good one: the loop in getEventAncestors starts with "ancestor = this" (where "this" is a Node). I tried to rework the loop on the spot just now, but it was getting inelegant. I added a FIXME instead.

>> WebCore/dom/Node.cpp:1445
>> +ContainerNode* Node::parentOrHostNode()
> 
> Candidate for inlining?

Sure. Done.

>> WebCore/dom/Node.cpp:2516
>> +void Node::eventAncestors(Vector<EventContext> &ancestors, EventTarget* originalTarget)
> 
> Misplaced "&" here. It's supposed to be next to the type, not the argument name.
> 
> Functions like this one, that use an out argument, should typically have “get” in their name. Not sure why this didn’t have that already.

Done.

>> WebCore/dom/Node.cpp:2544
>> +    };
> 
> Stray semicolon here. It should be removed.

Sorry -- removed.

>> WebCore/dom/Node.cpp:2575
>> +    RefPtr<EventTarget> originalTarget(event->target());
> 
> I prefer the "=" style syntax to the constructor style in cases like this. What do you think?

Sure. Fixed.

>> WebCore/dom/Node.cpp:2618
>> +            goto doneDispatching;
> 
> This could just be a “break” and probably should be. The reason we use goto above is that it’s nested inside two loops.

Fixed.

>> WebCore/dom/Node.h:216
>> +    void eventAncestors(Vector<EventContext> &ancestors, EventTarget*);
> 
> As I mentioned above, misplaced & here, and the name should be changed as the FIXME says.

Done.

>> WebCore/dom/WindowEventContext.cpp:50
>> +    }
> 
> This might read better like this:
> 
>     EventTarget* targetForWindowEvents = topEventContext ? topEventContext->node() : node;
>     Node* topLevelContainer = topEventContext ? topEventContext->target() : node;
> 
> Not sure. I suppose it depends on whether you are comfortable with ?: or not.

I like it. Changed.

>> WebCore/dom/WindowEventContext.h:43
>> +    WindowEventContext(Event*, Node*, const EventContext*);
> 
> Same comment as above about PassRefPtr.

Sure, done. It didn't come out looking pretty, since node may or may not be ref'd up -- but the constructor signature does indicate intent more clearly.

[Dimitri Glazkov (Google)]

Created attachment 72410 [details]
Patch for landing 2.0

[Dimitri Glazkov (Google)]

Committed r70984: <http://trac.webkit.org/changeset/70984>

[Dimitri Glazkov (Google)]

Reverted r70984 for reason:

Made media/audio-delete-while-slider-thumb-clicked.html crash.

Committed r70985: <http://trac.webkit.org/changeset/70985>

[Dimitri Glazkov (Google)]

(In reply to comment #20)
> Reverted r70984 for reason:
> 
> Made media/audio-delete-while-slider-thumb-clicked.html crash.
> 
> Committed r70985: <http://trac.webkit.org/changeset/70985>

I can't repro on platform/mac. Sounds like a port-specific misuse of something, argh...

[Dimitri Glazkov (Google)]

(In reply to comment #21)
> (In reply to comment #20)
> > Reverted r70984 for reason:
> > 
> > Made media/audio-delete-while-slider-thumb-clicked.html crash.
> > 
> > Committed r70985: <http://trac.webkit.org/changeset/70985>
> 
> I can't repro on platform/mac. Sounds like a port-specific misuse of something, argh...

Ah, turns out it's a difference in garbage collector. New patch/fix coming up...

[Dimitri Glazkov (Google)]

Created attachment 72582 [details]
With a stale ptr fix

[Dimitri Glazkov (Google)]

(In reply to comment #23)
> Created an attachment (id=72582) [details]
> With a stale ptr fix

Thanks for your patience. I found a stale pointer that's only visible with the new patch and over-active garbage collector. It's pretty obvious by inspection, but I am going to work on a layout test to make sure we capture the regression. Apparently, I am doomed to write large patches :)

[Dimitri Glazkov (Google)]

(In reply to comment #24)
> (In reply to comment #23)
> > Created an attachment (id=72582) [details] [details]
> > With a stale ptr fix
> 
> Thanks for your patience. I found a stale pointer that's only visible with the new patch and over-active garbage collector. It's pretty obvious by inspection, but I am going to work on a layout test to make sure we capture the regression. Apparently, I am doomed to write large patches :)

Turns out this was 2 pixels short of actually reaching a thumb on platform/mac:

http://trac.webkit.org/browser/trunk/LayoutTests/media/audio-delete-while-slider-thumb-clicked.html#L57

So this test didn't actually test what it had intended. In Chromium, the scrubber thumb is more rectangular in shape and it was triggering the crash. I'll improve the test and submit with this change.

[WebKit Review Bot]

Attachment 72360 [details] did not build on win:
Build output: http://queues.webkit.org/results/4942020

[Dimitri Glazkov (Google)]

(In reply to comment #26)
> Attachment 72360 [details] did not build on win:
> Build output: http://queues.webkit.org/results/4942020

Yeah, I realized that today too :)

[Dimitri Glazkov (Google)]

Committed r71244: <http://trac.webkit.org/changeset/71244>

[WebKit Review Bot]

http://trac.webkit.org/changeset/71244 might have broken Leopard Intel Release (Tests), Leopard Intel Debug (Tests), SnowLeopard Intel Release (Tests), GTK Linux 32-bit Release, GTK Linux 32-bit Debug, and GTK Linux 64-bit Debug
The following tests are not passing:
editing/spelling/context-menu-suggestions.html
media/controls-right-click-on-timebar.html

[Ryosuke Niwa]

(In reply to comment #29)
> http://trac.webkit.org/changeset/71244 might have broken Leopard Intel Release (Tests), Leopard Intel Debug (Tests), SnowLeopard Intel Release (Tests), GTK Linux 32-bit Release, GTK Linux 32-bit Debug, and GTK Linux 64-bit Debug
> The following tests are not passing:
> editing/spelling/context-menu-suggestions.html
> media/controls-right-click-on-timebar.html

http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r71244%20(23660)/results.html
context-menu-suggestions.html indeed seems to be failing consistently.  Could you look into this?

[Dimitri Glazkov (Google)]

Sorry, rolled out again in http://trac.webkit.org/changeset/71278. Turns out Darin was right: there's not enough test coverage. I realized that with this change, I also need to adjust event handling in existing shadow DOM elements, because right now they expect their events to be handled by their hosts, which breaks context menu, for instance.

[Dimitri Glazkov (Google)]

(In reply to comment #31)
> Sorry, rolled out again in http://trac.webkit.org/changeset/71278. Turns out Darin was right: there's not enough test coverage. I realized that with this change, I also need to adjust event handling in existing shadow DOM elements, because right now they expect their events to be handled by their hosts, which breaks context menu, for instance.

Found the problem. http://trac.webkit.org/changeset/60418 erroneously flipped a condition:

Before http://trac.webkit.org/browser/trunk/WebCore/rendering/TextControlInnerElements.cpp?rev=58914#L123
After http://trac.webkit.org/browser/trunk/WebCore/rendering/TextControlInnerElements.cpp?rev=60418#L141

Prior to event retargeting, it would just fall back to HTMLInputElement::defaultEventHandler, so this problem wasn't visible.

[Darin Adler]

(In reply to comment #32)
> http://trac.webkit.org/changeset/60418 erroneously flipped a condition:

Oops, sorry!

[Dimitri Glazkov (Google)]

(In reply to comment #33)
> (In reply to comment #32)
> > http://trac.webkit.org/changeset/60418 erroneously flipped a condition:
> 
> Oops, sorry!

No worries, it's all good. With the help of Erik Arvidsson, I made good progress on layout test coverage of the edge cases around event shadow boundary crossing and will include the first set of tests with the patch.

[Dimitri Glazkov (Google)]

Created attachment 73131 [details]
Patch

[Darin Adler]

Comment on attachment 73131 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        * media/audio-delete-while-slider-thumb-clicked.html:

No comment about why this test had to change?

> WebCore/rendering/ShadowElement.h:52
> -    HTMLElement* m_shadowParent;
> +    RefPtr<HTMLElement> m_shadowParent;

Are you sure this change is safe? Can’t this lead to a reference cycle?

[Dimitri Glazkov (Google)]

(In reply to comment #36)
> (From update of attachment 73131 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73131&action=review
> 
> > LayoutTests/ChangeLog:11
> > +        * media/audio-delete-while-slider-thumb-clicked.html:
> 
> No comment about why this test had to change?

The explanation is at the top, but I should probably move it down to individual test. Will re-upload in a sec.

> > WebCore/rendering/ShadowElement.h:52
> > -    HTMLElement* m_shadowParent;
> > +    RefPtr<HTMLElement> m_shadowParent;
> 
> Are you sure this change is safe? Can’t this lead to a reference cycle?

Hmm. That's a good point. Because ShadowElement's owner (like RenderSlider) is not ref-counted, I don't believe so:

HTMLInputElement-raw->RenderSlider-RefPtr->ShadowElement->RefPtr->HTMLInputElement.

[Dimitri Glazkov (Google)]

Created attachment 73137 [details]
Patch

[Darin Adler]

Comment on attachment 73137 [details]
Patch

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

> WebCore/dom/Node.cpp:2535
>  #if ENABLE(SVG)
> -            // Skip <use> shadow tree elements.
> -            if (ancestor->isSVGElement() && ancestor->isShadowNode())
> -                continue;
> +        // Skip SVGShadowTreeRootElement.
> +        shouldSkipNextAncestor = ancestor->isSVGElement() && ancestor->isShadowNode();
> +        if (shouldSkipNextAncestor)
> +            continue;
>  #endif

Idea probably for next iteration: If we put the isSVGElement && isShadowNode check into an inline function we could get the #if out of this function, much as you did with the eventTargetRespectingSVGTargetRules function.

> WebCore/dom/Node.cpp:2555
> +    return ancestors.isEmpty() ? 0 : &(ancestors.last());

You don’t need those parentheses. To me they make the code harder to read, although others may not feel the same way.

> WebCore/dom/Node.h:214
> +    // Node's parent or shadow tree host.
> +    ContainerNode* parentOrHostNode();

I’m not sure this function needs “Node” in its name.

> WebCore/dom/WindowEventContext.cpp:41
> +    // We don't dispatch load events to the window. That quirk was originally
> +    // added because Mozilla doesn't propagate load events to the window object.

Should we say “This quirk” rather than “That quirk”?

[Dimitri Glazkov (Google)]

(In reply to comment #39)
> (From update of attachment 73137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73137&action=review
> 
> > WebCore/dom/Node.cpp:2535
> >  #if ENABLE(SVG)
> > -            // Skip <use> shadow tree elements.
> > -            if (ancestor->isSVGElement() && ancestor->isShadowNode())
> > -                continue;
> > +        // Skip SVGShadowTreeRootElement.
> > +        shouldSkipNextAncestor = ancestor->isSVGElement() && ancestor->isShadowNode();
> > +        if (shouldSkipNextAncestor)
> > +            continue;
> >  #endif
> 
> Idea probably for next iteration: If we put the isSVGElement && isShadowNode check into an inline function we could get the #if out of this function, much as you did with the eventTargetRespectingSVGTargetRules function.

Yeah, I really need to take another shot at this mess.

> 
> > WebCore/dom/Node.cpp:2555
> > +    return ancestors.isEmpty() ? 0 : &(ancestors.last());
> 
> You don’t need those parentheses. To me they make the code harder to read, although others may not feel the same way.

Removed.

> 
> > WebCore/dom/Node.h:214
> > +    // Node's parent or shadow tree host.
> > +    ContainerNode* parentOrHostNode();
> 
> I’m not sure this function needs “Node” in its name.

It's mimicking parentNode(). I'll leave it as-is for now. I am not entirely happy with the term "host" either, but I'll paint that shed in a better color when we get closer to actual XBL2 implementation.

> > WebCore/dom/WindowEventContext.cpp:41
> > +    // We don't dispatch load events to the window. That quirk was originally
> > +    // added because Mozilla doesn't propagate load events to the window object.
> 
> Should we say “This quirk” rather than “That quirk”?

Yep!

[Dimitri Glazkov (Google)]

Committed r71934: <http://trac.webkit.org/changeset/71934>

[WebKit Review Bot]

http://trac.webkit.org/changeset/71934 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
media/controls-right-click-on-timebar.html

[Dimitri Glazkov (Google)]

(In reply to comment #42)
> http://trac.webkit.org/changeset/71934 might have broken GTK Linux 64-bit Debug
> The following tests are not passing:
> media/controls-right-click-on-timebar.html

Looking into the breakage ...

[Dimitri Glazkov (Google)]

(In reply to comment #43)
> (In reply to comment #42)
> > http://trac.webkit.org/changeset/71934 might have broken GTK Linux 64-bit Debug
> > The following tests are not passing:
> > media/controls-right-click-on-timebar.html
> 
> Looking into the breakage ...

It's due to https://bugs.webkit.org/show_bug.cgi?id=40601.