Bug 107797 - [Shadow] Gesture event is not fired in ShadowDOM
Summary: [Shadow] Gesture event is not fired in ShadowDOM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on:
Blocks: 107796
  Show dependency treegraph
 
Reported: 2013-01-24 00:39 PST by Shinya Kawanaka
Modified: 2013-01-28 22:36 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.91 KB, patch)
2013-01-24 03:42 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (6.97 KB, patch)
2013-01-24 18:17 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (6.97 KB, patch)
2013-01-25 00:21 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (7.15 KB, patch)
2013-01-27 21:57 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (7.35 KB, patch)
2013-01-28 18:12 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2013-01-24 00:39:00 PST
When an element in ShadowDOM is touched, the event will be fired from its host element.
If the element is in nested ShadowDOM, the element in ShadowDOM will be exposed.
Comment 1 Shinya Kawanaka 2013-01-24 03:42:37 PST
Created attachment 184458 [details]
Patch
Comment 2 Early Warning System Bot 2013-01-24 14:28:03 PST
Comment on attachment 184458 [details]
Patch

Attachment 184458 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16072933
Comment 3 Peter Beverloo (cr-android ews) 2013-01-24 14:36:49 PST
Comment on attachment 184458 [details]
Patch

Attachment 184458 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16110265
Comment 4 Early Warning System Bot 2013-01-24 15:00:18 PST
Comment on attachment 184458 [details]
Patch

Attachment 184458 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16078921
Comment 5 WebKit Review Bot 2013-01-24 15:11:37 PST
Comment on attachment 184458 [details]
Patch

Attachment 184458 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16080895
Comment 6 Shinya Kawanaka 2013-01-24 18:17:26 PST
Created attachment 184631 [details]
Patch
Comment 7 Shinya Kawanaka 2013-01-25 00:21:06 PST
Created attachment 184686 [details]
Patch
Comment 8 Shinya Kawanaka 2013-01-25 00:21:38 PST
Uploaded the same patch to use cr-linux bot again.
Comment 9 Hajime Morrita 2013-01-25 01:16:36 PST
Comment on attachment 184686 [details]
Patch

What pappens for UA shadows?
Comment 10 WebKit Review Bot 2013-01-25 06:41:47 PST
Comment on attachment 184686 [details]
Patch

Attachment 184686 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16113414

New failing tests:
fast/dom/shadow/touch-event.html
fast/events/touch/emulate-touch-events.html
Comment 11 Shinya Kawanaka 2013-01-27 17:54:11 PST
(In reply to comment #9)
> (From update of attachment 184686 [details])
> What pappens for UA shadows?

Basically it should work, since this patch convert the target node into the shadow host.
For the complete fix, we have to have the event-retargeting, which will be addressed in Bug 107800.

However... it's true that we have to have a test.
Comment 12 Shinya Kawanaka 2013-01-27 21:57:53 PST
Created attachment 184937 [details]
Patch
Comment 13 Dominic Cooney 2013-01-28 06:28:44 PST
Comment on attachment 184937 [details]
Patch

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

I am not a reviewer but I have a question and some comments inline.

> Source/WebCore/ChangeLog:12
> +        for backward compatibility.

What does this mean for nested Shadow DOM?

> LayoutTests/fast/dom/shadow/touch-event.html:2
> +<html><body>

You could just write

<body>

> LayoutTests/fast/dom/shadow/touch-event.html:48
> +container.innerHTML = "";

Use '' for consistency.

Maybe

container.remove()

is neater.

> LayoutTests/fast/dom/shadow/touch-event.html:52
> +</body></html>

You could omit this line.
Comment 14 Hajime Morrita 2013-01-28 16:19:38 PST
(In reply to comment #13)
> 
> What does this mean for nested Shadow DOM?
> 
> > LayoutTests/fast/dom/shadow/touch-event.html:2
> > +<html><body>
> 
> You could just write
> 
> <body>
> 
> 
> > LayoutTests/fast/dom/shadow/touch-event.html:52
> > +</body></html>
> 
> You could omit this line.

WebKit tests traditionally don't do this kind of optimization.
It's a matter of taste though.
Comment 15 Shinya Kawanaka 2013-01-28 17:56:48 PST
Comment on attachment 184937 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        for backward compatibility.
> 
> What does this mean for nested Shadow DOM?

We have two options which shadow ancestor nodes we use for nested Shadow DOM.
1) Always use nodes in document tree
2) Use host element in parent tree scope.

If we don't use nested ShadowDOM, it's same.

If we use nested ShadowDOM, event listener will get different touchTarget. For (1), event listener in ShadowDOM will get wrong touchTarget, while for (2), event listener in document tree scope will get wrong touchTarget.
In my opinion, we should prioritize the correctness of document tree scope all time.

>> LayoutTests/fast/dom/shadow/touch-event.html:52
>> +</body></html>
> 
> You could omit this line.

In my belief, end tag should not be omitted without a strong reason...
Comment 16 Shinya Kawanaka 2013-01-28 18:12:44 PST
Created attachment 185124 [details]
Patch
Comment 17 Dominic Cooney 2013-01-28 22:05:51 PST
(In reply to comment #15)
> (From update of attachment 184937 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184937&action=review
> 
> >> Source/WebCore/ChangeLog:12
> >> +        for backward compatibility.
> > 
> > What does this mean for nested Shadow DOM?
> 
> We have two options which shadow ancestor nodes we use for nested Shadow DOM.
> 1) Always use nodes in document tree
> 2) Use host element in parent tree scope.
> 
> If we don't use nested ShadowDOM, it's same.
> 
> If we use nested ShadowDOM, event listener will get different touchTarget. For (1), event listener in ShadowDOM will get wrong touchTarget, while for (2), event listener in document tree scope will get wrong touchTarget.
> In my opinion, we should prioritize the correctness of document tree scope all time.

What does "prioritize the correctness of document tree scope" mean? It means you don’t care what touch targets event handlers in nested Shadow DOM see? That does not seem right.
Comment 18 WebKit Review Bot 2013-01-28 22:24:00 PST
Comment on attachment 185124 [details]
Patch

Clearing flags on attachment: 185124

Committed r141054: <http://trac.webkit.org/changeset/141054>
Comment 19 WebKit Review Bot 2013-01-28 22:24:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Shinya Kawanaka 2013-01-28 22:36:30 PST
> What does "prioritize the correctness of document tree scope" mean? It means you don’t care what touch targets event handlers in nested Shadow DOM see? That does not seem right.

Did you see my comment and ChangeLog?
I chose (1), so touchTarget in ShadowDOM is wrong with this patch. To make it correct in both document tree and shadow tree, event retarget must be implemented. That problem will be addressed in Bug 107800.