Bug 76217

Summary: REGRESSION (Safari 5.0-5.1): Event.eventPhase is not set to 2 (at target) when handling dblclick event in <input>
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Tools / TestsAssignee: Hayato Ito <hayato>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, dglazkov, dominicc, hayato, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 76198    
Attachments:
Description Flags
test case from IETC
none
multiple AT_TARGET none

Description Eric Seidel (no email) 2012-01-12 14:58:53 PST
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.
Comment 1 Eric Seidel (no email) 2012-01-12 14:59:30 PST
Created attachment 122316 [details]
test case from IETC
Comment 2 Alexey Proskuryakov 2012-01-12 16:33:55 PST
This used to pass in Safari 5.0.
Comment 3 Dimitri Glazkov (Google) 2012-01-12 16:37:19 PST
Will look into this.
Comment 4 Dimitri Glazkov (Google) 2012-01-12 17:20:48 PST
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.
Comment 5 Dimitri Glazkov (Google) 2012-01-12 17:27:46 PST
Spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15543
Comment 6 Hayato Ito 2012-01-12 20:56:47 PST

(In reply to comment #5)
> Spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15543
Comment 7 Hayato Ito 2012-01-12 20:57:31 PST
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
Comment 8 Hayato Ito 2012-01-16 03:40:56 PST
Created attachment 122611 [details]
multiple AT_TARGET
Comment 9 Hayato Ito 2012-01-16 03:42:36 PST
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.
Comment 10 Dimitri Glazkov (Google) 2012-01-16 09:24:59 PST
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?
Comment 11 Eric Seidel (no email) 2012-01-16 09:41:17 PST
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?
Comment 12 Dimitri Glazkov (Google) 2012-01-16 09:43:26 PST
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.
Comment 13 Hayato Ito 2012-01-16 17:18:47 PST
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+.
Comment 14 WebKit Review Bot 2012-01-16 19:56:32 PST
Comment on attachment 122611 [details]
multiple AT_TARGET

Clearing flags on attachment: 122611

Committed r105123: <http://trac.webkit.org/changeset/105123>
Comment 15 WebKit Review Bot 2012-01-16 19:56:38 PST
All reviewed patches have been landed.  Closing bug.