Bug 107800

Summary: [Shadow] Implements event re-targeting for Touch Events.
Product: WebKit Reporter: Hayato Ito <hayato>
Component: UI EventsAssignee: Hayato Ito <hayato>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dfreedm, dglazkov, dominicc, esprehn+autocc, ojan.autocc, rego+ews, rniwa, shinyak, syoichi, webcomponents-bugzilla, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 109156    
Bug Blocks: 107796, 108908    
Attachments:
Description Flags
WIP. flaky. Not ready for review
none
Ready for review
none
Fix a build hopefully
none
comments addressed. none

Description Hayato Ito 2013-01-24 00:57:47 PST
I've filed a bug for the Shadow DOM Spec here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=20746.

We have to re-target Touch.target for multiple touches if each touch point is in different shadow trees.
Comment 1 Hayato Ito 2013-01-24 05:45:24 PST
At first glance, we have to:

- Introduces TouchEventDispatchMediator
- Refactor EventContext so that it has additional fields, such as touches, targetTouches and changedTouches. Another ides is to have another class which inherites EventContext.
- Refactor EventRelatedTargetAdjuster so that it can be re-used from TouchEventDispatchMediator.

More importantly, we should be careful to the performance. In naive implementation, it takes O(n^2) times than simple MouseEvent relatedTarget-retargeting.
TouchEvent retargeting will involve N events times N retargetings (N is the number of touch points).
Comment 2 Dominic Cooney 2013-01-28 06:02:41 PST
Actually not O(n^2) but O(nm) where n is number of targets and m number of touches, right?
Comment 3 Shinya Kawanaka 2013-01-28 22:38:44 PST
Actually we have to run event retargeting algorithm with single touch :-)
Comment 4 Hayato Ito 2013-02-22 04:27:03 PST
Created attachment 189745 [details]
WIP. flaky. Not ready for review
Comment 5 Dimitri Glazkov (Google) 2013-02-22 08:54:17 PST
Thank you for working on this!
Comment 6 Hayato Ito 2013-03-04 21:23:21 PST
Created attachment 191397 [details]
Ready for review
Comment 7 WebKit Review Bot 2013-03-04 21:29:03 PST
Attachment 191397 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/dom/shadow/resources/event-dispatching.js', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-distributed-text-node-expected.txt', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-distributed-text-node.html', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-text-node-in-shadow-root-expected.txt', u'LayoutTests/fast/dom/shadow/touch-event-retargeting-expected.txt', u'LayoutTests/fast/dom/shadow/touch-event-retargeting.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/EventContext.cpp', u'Source/WebCore/dom/EventContext.h', u'Source/WebCore/dom/EventRetargeter.cpp', u'Source/WebCore/dom/EventRetargeter.h', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/dom/Node.h', u'Source/WebCore/dom/Touch.cpp', u'Source/WebCore/dom/Touch.h', u'Source/WebCore/dom/TouchEvent.cpp', u'Source/WebCore/dom/TouchEvent.h', u'Source/WebCore/dom/TouchList.cpp', u'Source/WebCore/dom/TouchList.h', u'Source/WebCore/page/EventHandler.cpp']" exit_code: 1
Source/WebCore/dom/EventContext.h:80:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Hayato Ito 2013-03-04 21:31:47 PST
This is an expected style error. We should file a bug against check-webkit-style...

(In reply to comment #7)
> Attachment 191397 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/dom/shadow/resources/event-dispatching.js', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-distributed-text-node-expected.txt', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-distributed-text-node.html', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-text-node-in-shadow-root-expected.txt', u'LayoutTests/fast/dom/shadow/touch-event-retargeting-expected.txt', u'LayoutTests/fast/dom/shadow/touch-event-retargeting.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/EventContext.cpp', u'Source/WebCore/dom/EventContext.h', u'Source/WebCore/dom/EventRetargeter.cpp', u'Source/WebCore/dom/EventRetargeter.h', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/dom/Node.h', u'Source/WebCore/dom/Touch.cpp', u'Source/WebCore/dom/Touch.h', u'Source/WebCore/dom/TouchEvent.cpp', u'Source/WebCore/dom/TouchEvent.h', u'Source/WebCore/dom/TouchList.cpp', u'Source/WebCore/dom/TouchList.h', u'Source/WebCore/page/EventHandler.cpp']" exit_code: 1
> Source/WebCore/dom/EventContext.h:80:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 1 in 21 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Build Bot 2013-03-04 22:27:27 PST
Comment on attachment 191397 [details]
Ready for review

Attachment 191397 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16988081
Comment 10 Hayato Ito 2013-03-04 22:40:34 PST
Mac bot seems unhappy. Let me take a look.

(In reply to comment #9)
> (From update of attachment 191397 [details])
> Attachment 191397 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-commit-queue.appspot.com/results/16988081
Comment 11 Build Bot 2013-03-04 22:44:04 PST
Comment on attachment 191397 [details]
Ready for review

Attachment 191397 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16999059
Comment 12 Hayato Ito 2013-03-04 22:58:32 PST
It seems I have to update the patch to support the port where TOUCH_EVENTS flag is disabled.

(In reply to comment #10)
> Mac bot seems unhappy. Let me take a look.
> 
> (In reply to comment #9)
> > (From update of attachment 191397 [details] [details])
> > Attachment 191397 [details] [details] did not pass mac-wk2-ews (mac-wk2):
> > Output: http://webkit-commit-queue.appspot.com/results/16988081
Comment 13 Hayato Ito 2013-03-05 02:46:09 PST
Created attachment 191447 [details]
Fix a build hopefully
Comment 14 WebKit Review Bot 2013-03-05 02:53:00 PST
Attachment 191447 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/dom/shadow/resources/event-dispatching.js', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-distributed-text-node-expected.txt', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-distributed-text-node.html', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-text-node-in-shadow-root-expected.txt', u'LayoutTests/fast/dom/shadow/touch-event-retargeting-expected.txt', u'LayoutTests/fast/dom/shadow/touch-event-retargeting.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/EventContext.cpp', u'Source/WebCore/dom/EventContext.h', u'Source/WebCore/dom/EventRetargeter.cpp', u'Source/WebCore/dom/EventRetargeter.h', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/dom/Node.h', u'Source/WebCore/dom/Touch.cpp', u'Source/WebCore/dom/Touch.h', u'Source/WebCore/dom/TouchEvent.cpp', u'Source/WebCore/dom/TouchEvent.h', u'Source/WebCore/dom/TouchList.cpp', u'Source/WebCore/dom/TouchList.h', u'Source/WebCore/page/EventHandler.cpp']" exit_code: 1
Source/WebCore/dom/EventContext.h:82:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Dimitri Glazkov (Google) 2013-03-05 09:45:51 PST
Comment on attachment 191447 [details]
Fix a build hopefully

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

This looks great, I only have two quibbles below: the loop avoidance and the cloning of touches. If the latter is not important (I don't know), we can land this and improve on it.

> Source/WebCore/dom/EventRetargeter.cpp:135
> +        TouchEventContext* touchEventContext = static_cast<TouchEventContext*>(eventPath[i].get());

Sounds like we need a toTouchEventContext helper?

> Source/WebCore/dom/EventRetargeter.cpp:150
> +    for (size_t i = 0; i< touchList.length(); ++i) {

"i<" to "i <"

Also, I wonder if we could check to see if adjustment is going to be needed at a given node in event path and eliminate this loop in those cases?

> Source/WebCore/dom/EventRetargeter.cpp:156
> +            touchListPath[j]->append(touch.cloneWithNewTarget(adjustedNodes[j].get()));

Wait.. this is interesting. Are we supposed to return the same object from .touches each time? If so, this code is not doing the right thing -- we should be only adjusting the target on touch.

> Source/WebCore/dom/EventRetargeter.h:54
> +    typedef Vector<RefPtr<TouchList> > TouchListPath;

EventPathTouchLists?

> Source/WebCore/dom/EventRetargeter.h:69
> +    static void calculateAdjustedNodes(const Node*, const Node* relatedNode, bool stopEventAtBoundaryIfNeeded, EventPath&, AdjustedNodes&);

Turn stopEventAtBoundaryIfNeeded into an enum?
Comment 16 Daniel Freedman 2013-03-05 12:32:31 PST
(In reply to comment #15)
> (From update of attachment 191447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191447&action=review
> 
> This looks great, I only have two quibbles below: the loop avoidance and the cloning of touches. If the latter is not important (I don't know), we can land this and improve on it.
> 
> > Source/WebCore/dom/EventRetargeter.cpp:135
> > +        TouchEventContext* touchEventContext = static_cast<TouchEventContext*>(eventPath[i].get());
> 
> Sounds like we need a toTouchEventContext helper?
> 
> > Source/WebCore/dom/EventRetargeter.cpp:150
> > +    for (size_t i = 0; i< touchList.length(); ++i) {
> 
> "i<" to "i <"
> 
> Also, I wonder if we could check to see if adjustment is going to be needed at a given node in event path and eliminate this loop in those cases?
> 
> > Source/WebCore/dom/EventRetargeter.cpp:156
> > +            touchListPath[j]->append(touch.cloneWithNewTarget(adjustedNodes[j].get()));
> 
> Wait.. this is interesting. Are we supposed to return the same object from .touches each time? If so, this code is not doing the right thing -- we should be only adjusting the target on touch.

Chrome and Safari seem to diverge here. In Chrome, each touch object is created anew for each touch event. In iOS safari, the touch objects are reused between touch events.

Technically, the spec states that Touch objects are immutable, and it's attributes must not change: http://www.w3.org/TR/touch-events/#touch-interface

However, neither the safari documentation, nor the touch event specs state if the Touch object should or should not be reused between touch events.

> 
> > Source/WebCore/dom/EventRetargeter.h:54
> > +    typedef Vector<RefPtr<TouchList> > TouchListPath;
> 
> EventPathTouchLists?
> 
> > Source/WebCore/dom/EventRetargeter.h:69
> > +    static void calculateAdjustedNodes(const Node*, const Node* relatedNode, bool stopEventAtBoundaryIfNeeded, EventPath&, AdjustedNodes&);
> 
> Turn stopEventAtBoundaryIfNeeded into an enum?
Comment 17 Hayato Ito 2013-03-05 16:35:32 PST
Thank you for the review. Let me upload a new patch soon. But I am WebKit gardener from today... :)

(In reply to comment #15)
> (From update of attachment 191447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191447&action=review
> 
> This looks great, I only have two quibbles below: the loop avoidance and the cloning of touches. If the latter is not important (I don't know), we can land this and improve on it.
> 
> > Source/WebCore/dom/EventRetargeter.cpp:135
> > +        TouchEventContext* touchEventContext = static_cast<TouchEventContext*>(eventPath[i].get());
> 
> Sounds like we need a toTouchEventContext helper?

Sure. Let me add the helper.

> 
> > Source/WebCore/dom/EventRetargeter.cpp:150
> > +    for (size_t i = 0; i< touchList.length(); ++i) {
> 
> "i<" to "i <"

Ops. Let me fix.

> 
> Also, I wonder if we could check to see if adjustment is going to be needed at a given node in event path and eliminate this loop in those cases?

Yay, we can.  I have to admit that this patch is naive in terms of the performance, but this is the safest and easy-to-understand approach as the first wave.

I have a vague plan that I'll introduce per-treescope EventContext rather than current per-node EventContext. Touch Events dispatching will get a benefit from that, I think. So instead of making this first patch complex, I'd like to delay such an optimization and work on per-treescope EventContext in another bug.

Are you happy with this? If so, I'll file a bug for per-treescope EventContext.

> 
> > Source/WebCore/dom/EventRetargeter.cpp:156
> > +            touchListPath[j]->append(touch.cloneWithNewTarget(adjustedNodes[j].get()));
> 
> Wait.. this is interesting. Are we supposed to return the same object from .touches each time? If so, this code is not doing the right thing -- we should be only adjusting the target on touch.

That's exactly what I did in the first WIP patch. https://bugs.webkit.org/attachment.cgi?id=189745
I have encountered the strange aliasing problem. The reason is what Daniel pointed out. Touch object may be re-used between multiple Touch event dispatchings. EventHandler keeps Touch objects between multiple Touch Events dispatching. So we have to clone it.

> 
> > Source/WebCore/dom/EventRetargeter.h:54
> > +    typedef Vector<RefPtr<TouchList> > TouchListPath;
> 
> EventPathTouchLists?

Okay.

> 
> > Source/WebCore/dom/EventRetargeter.h:69
> > +    static void calculateAdjustedNodes(const Node*, const Node* relatedNode, bool stopEventAtBoundaryIfNeeded, EventPath&, AdjustedNodes&);
> 
> Turn stopEventAtBoundaryIfNeeded into an enum?

Okay. Let me do it.
(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 191447 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191447&action=review
> > 
> > This looks great, I only have two quibbles below: the loop avoidance and the cloning of touches. If the latter is not important (I don't know), we can land this and improve on it.
> > 
> > > Source/WebCore/dom/EventRetargeter.cpp:135
> > > +        TouchEventContext* touchEventContext = static_cast<TouchEventContext*>(eventPath[i].get());
> > 
> > Sounds like we need a toTouchEventContext helper?
> > 
> > > Source/WebCore/dom/EventRetargeter.cpp:150
> > > +    for (size_t i = 0; i< touchList.length(); ++i) {
> > 
> > "i<" to "i <"
> > 
> > Also, I wonder if we could check to see if adjustment is going to be needed at a given node in event path and eliminate this loop in those cases?
> > 
> > > Source/WebCore/dom/EventRetargeter.cpp:156
> > > +            touchListPath[j]->append(touch.cloneWithNewTarget(adjustedNodes[j].get()));
> > 
> > Wait.. this is interesting. Are we supposed to return the same object from .touches each time? If so, this code is not doing the right thing -- we should be only adjusting the target on touch.
> 
> Chrome and Safari seem to diverge here. In Chrome, each touch object is created anew for each touch event. In iOS safari, the touch objects are reused between touch events.
> 
> Technically, the spec states that Touch objects are immutable, and it's attributes must not change: http://www.w3.org/TR/touch-events/#touch-interface
> 
> However, neither the safari documentation, nor the touch event specs state if the Touch object should or should not be reused between touch events.

Yay. I thought the fact, Touch is immutable, can justify the cloning of Touch object since users cannot notice Touch is the same instance or not in their event listeners. Therefore, in practical, it does not matter for users wheter Touch object is cloned or not as long as Touch is immutable.

As for the performance, I tried to re-use Touch object if there is an available Touch objects which has the same target. But I don't think it is worth that maintaing such Touch objects pool and looking for existing re-usable Touch objects. It might be an expensive operation.

> 
> > 
> > > Source/WebCore/dom/EventRetargeter.h:54
> > > +    typedef Vector<RefPtr<TouchList> > TouchListPath;
> > 
> > EventPathTouchLists?
> > 
> > > Source/WebCore/dom/EventRetargeter.h:69
> > > +    static void calculateAdjustedNodes(const Node*, const Node* relatedNode, bool stopEventAtBoundaryIfNeeded, EventPath&, AdjustedNodes&);
> > 
> > Turn stopEventAtBoundaryIfNeeded into an enum?
Comment 18 Hayato Ito 2013-03-05 17:25:42 PST
Created attachment 191617 [details]
comments addressed.
Comment 19 WebKit Review Bot 2013-03-05 17:36:18 PST
Attachment 191617 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/dom/shadow/resources/event-dispatching.js', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-distributed-text-node-expected.txt', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-distributed-text-node.html', u'LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-text-node-in-shadow-root-expected.txt', u'LayoutTests/fast/dom/shadow/touch-event-retargeting-expected.txt', u'LayoutTests/fast/dom/shadow/touch-event-retargeting.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/EventContext.cpp', u'Source/WebCore/dom/EventContext.h', u'Source/WebCore/dom/EventRetargeter.cpp', u'Source/WebCore/dom/EventRetargeter.h', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/dom/Node.h', u'Source/WebCore/dom/Touch.cpp', u'Source/WebCore/dom/Touch.h', u'Source/WebCore/dom/TouchEvent.cpp', u'Source/WebCore/dom/TouchEvent.h', u'Source/WebCore/dom/TouchList.cpp', u'Source/WebCore/dom/TouchList.h', u'Source/WebCore/page/EventHandler.cpp']" exit_code: 1
Source/WebCore/dom/EventContext.h:84:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 WebKit Review Bot 2013-03-05 21:18:19 PST
Comment on attachment 191617 [details]
comments addressed.

Clearing flags on attachment: 191617

Committed r144877: <http://trac.webkit.org/changeset/144877>
Comment 21 WebKit Review Bot 2013-03-05 21:18:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Hayato Ito 2013-03-05 23:32:35 PST
I've filed a bug for per-scope (Touch)EventContext for future improvement.
https://bugs.webkit.org/show_bug.cgi?id=111527.

(In reply to comment #21)
> All reviewed patches have been landed.  Closing bug.