107797 – [Shadow] Gesture event is not fired in ShadowDOM

RESOLVED FIXED 107797

[Shadow] Gesture event is not fired in ShadowDOM

https://bugs.webkit.org/show_bug.cgi?id=107797

Summary [Shadow] Gesture event is not fired in ShadowDOM

Shinya Kawanaka

Reported 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.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Shinya Kawanaka

Comment 1 2013-01-24 03:42:37 PST

Created attachment 184458 [details] Patch

Early Warning System Bot

Comment 2 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

Peter Beverloo (cr-android ews)

Comment 3 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

Early Warning System Bot

Comment 4 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

WebKit Review Bot

Comment 5 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

Shinya Kawanaka

Comment 6 2013-01-24 18:17:26 PST

Created attachment 184631 [details] Patch

Shinya Kawanaka

Comment 7 2013-01-25 00:21:06 PST

Created attachment 184686 [details] Patch

Shinya Kawanaka

Comment 8 2013-01-25 00:21:38 PST

Uploaded the same patch to use cr-linux bot again.

Hajime Morrita

Comment 9 2013-01-25 01:16:36 PST

Comment on attachment 184686 [details] Patch What pappens for UA shadows?

WebKit Review Bot

Comment 10 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

Shinya Kawanaka

Comment 11 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.

Shinya Kawanaka

Comment 12 2013-01-27 21:57:53 PST

Created attachment 184937 [details] Patch

Dominic Cooney

Comment 13 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.

Hajime Morrita

Comment 14 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.

Shinya Kawanaka

Comment 15 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...

Shinya Kawanaka

Comment 16 2013-01-28 18:12:44 PST

Created attachment 185124 [details] Patch

Dominic Cooney

Comment 17 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.

WebKit Review Bot

Comment 18 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>

WebKit Review Bot

Comment 19 2013-01-28 22:24:04 PST

All reviewed patches have been landed. Closing bug.

Shinya Kawanaka

Comment 20 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.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Shinya Kawanaka

Reported

2013-01-24 00:39 PST

Modified

2013-01-28 22:36 PST History

CC List

7 users Show

URL

Keywords

Depends on

Blocks

107796

Dependencies

tree graph