Bug 107800 – [Shadow] Implements event re-targeting for Touch Events.

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Hayato Ito]

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.

[Hayato Ito]

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

[Dominic Cooney]

Actually not O(n^2) but O(nm) where n is number of targets and m number of touches, right?

[Shinya Kawanaka]

Actually we have to run event retargeting algorithm with single touch :-)

[Hayato Ito]

Created attachment 189745 [details]
WIP. flaky. Not ready for review

[Dimitri Glazkov (Google)]

Thank you for working on this!

[Hayato Ito]

Created attachment 191397 [details]
Ready for review

[WebKit Review Bot]

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.

[Hayato Ito]

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.

[Build Bot]

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

[Hayato Ito]

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

[Build Bot]

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

[Hayato Ito]

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

[Hayato Ito]

Created attachment 191447 [details]
Fix a build hopefully

[WebKit Review Bot]

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.

[Dimitri Glazkov (Google)]

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?

[Daniel Freedman]

(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?

[Hayato Ito]

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?

[Hayato Ito]

Created attachment 191617 [details]
comments addressed.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 191617 [details]
comments addressed.

Clearing flags on attachment: 191617

Committed r144877: <http://trac.webkit.org/changeset/144877>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hayato Ito]

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.