REGRESSION (Safari 5.0-5.1): Event.eventPhase is not set to 2 (at target) when handling dblclick event in <input>

[Eric Seidel (no email)]

Event.eventPhase is not set to 2 (at target) when handling dblclick event in <input>

I suspect this is confusion caused by our shadow DOM?

I've attached a copy of:
http://samples.msdn.microsoft.com/ietestcenter/domevents/event.eventphase.html

with a little bit of console.log logging added.

[Eric Seidel (no email)]

Created attachment 122316 [details]
test case from IETC

[Alexey Proskuryakov]

This used to pass in Safari 5.0.

[Dimitri Glazkov (Google)]

Will look into this.

[Dimitri Glazkov (Google)]

This is a very cool bug. I didn't realize that the meaning of AT_TARGET changes in the presence of the shadow DOM, and the AT_TARGET must be fired more than one time -- once for each shadow boundary. Since we are retargeting events, we must also report each "new" target as a separate occurrence.

[Dimitri Glazkov (Google)]

Spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15543

[Hayato Ito]

(In reply to comment #5)
> Spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15543

[Hayato Ito]

Let me take this bug.

(In reply to comment #6)
> 
> (In reply to comment #5)
> > Spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15543

[Hayato Ito]

Created attachment 122611 [details]
multiple AT_TARGET

[Hayato Ito]

Although there is on-going discussion on spec bug, https://www.w3.org/Bugs/Public/show_bug.cgi?id=15543, I've implemented mutlple AT_TARGET event mechanism based on my proposal, which fixes the regression.

[Dimitri Glazkov (Google)]

Comment on attachment 122611 [details]
multiple AT_TARGET

nice modification to the test. Ideally, I would like to avoid phase-tweaking logic in EventContext -- to keep class responsibilities clean and simple. Can you file a bug to track this and adjusting to final spec behavior?

[Eric Seidel (no email)]

Comment on attachment 122611 [details]
multiple AT_TARGET

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

> Source/WebCore/dom/EventContext.cpp:49
> +        if (eventPhase == Event::CAPTURING_PHASE && event->bubbles())
> +            return;

This return is new.  Is it covered by any tests?

[Dimitri Glazkov (Google)]

Comment on attachment 122611 [details]
multiple AT_TARGET

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

> LayoutTests/fast/dom/shadow/shadow-boundary-events-expected.txt:-80
> -PASS dispatchedEvent("blur") is ["shadowF(@divE)(capturing phase)", "shadowF(@shadowF)(capturing phase)", "shadowG(@shadowG)(capturing phase)", "divH(@divH)"]

Right here.

[Hayato Ito]

Thank you for the review.

(In reply to comment #10)
> (From update of attachment 122611 [details])
> nice modification to the test. Ideally, I would like to avoid phase-tweaking logic in EventContext -- to keep class responsibilities clean and simple. Can you file a bug to track this and adjusting to final spec behavior?

I agree. It seems that EventContext should be a plain data holder class.
Let me file another bug to track this.
I'll land this patch as is since it got r+.

[WebKit Review Bot]

Comment on attachment 122611 [details]
multiple AT_TARGET

Clearing flags on attachment: 122611

Committed r105123: <http://trac.webkit.org/changeset/105123>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.