Bug 76217 - REGRESSION (Safari 5.0-5.1): Event.eventPhase is not set to 2 (at target) when handling dblclick event in <input>
Summary: REGRESSION (Safari 5.0-5.1): Event.eventPhase is not set to 2 (at target) whe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks: 76198
  Show dependency treegraph
 
Reported: 2012-01-12 14:58 PST by Eric Seidel (no email)
Modified: 2012-01-16 19:56 PST (History)
6 users (show)

See Also:


Attachments
test case from IETC (3.02 KB, text/html)
2012-01-12 14:59 PST, Eric Seidel (no email)
no flags Details
multiple AT_TARGET (6.21 KB, patch)
2012-01-16 03:40 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.