Touch adjustment does not target shadow DOM elements

[Allan Sandfeld Jensen]

EventHandler::bestClickableNodeForTouchPoint() is currently ignoring shadow DOM elements, which means that it is currently not possible to use the controls of media-elements for instance.

[Allan Sandfeld Jensen]

Created attachment 148529 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148529 [details]
Patch

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

New failing tests:
touchadjustment/html-label.html

[WebKit Review Bot]

Created attachment 148538 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Allan Sandfeld Jensen]

Created attachment 148547 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 148547 [details]
Patch

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

LGTM

> LayoutTests/touchadjustment/media-element.html:28
> +    function testRoundTouch(x, y, radius)
> +    {

I think we normally keep the { on the same line in JS

[Build Bot]

Comment on attachment 148547 [details]
Patch

Attachment 148547 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12996218

[Build Bot]

Comment on attachment 148547 [details]
Patch

Attachment 148547 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13001200

[Allan Sandfeld Jensen]

Created attachment 148557 [details]
Patch

[Antonio Gomes]

Allan, does it make use of bug #80847 ?

[Allan Sandfeld Jensen]

(In reply to comment #9)
> Allan, does it make use of bug #80847 ?

Yes, I think that is what enables the fix to be this simple.

[Allan Sandfeld Jensen]

*** Bug 89674 has been marked as a duplicate of this bug. ***

[Allan Sandfeld Jensen]

(In reply to comment #11)
> *** Bug 89674 has been marked as a duplicate of this bug. ***

The test-case in the patch posted in bug #89674 looks like a more safe way to test shadow-dom that what I do.

[Antonio Gomes]

Comment on attachment 148557 [details]
Patch

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

Code looks ok. One question: does it overlap with bug 89674?

> LayoutTests/touchadjustment/media-element.html:33
> +        var adjustedNode = internals.touchNodeAdjustedToBestClickableNode(x, y, width, height, document);

should/could we unify internals::touchNodeAdjustedToBestClickableNode and Internals::nodeFromRect ?

> LayoutTests/touchadjustment/media-element.html:40
> +        adjustedNode = testRoundTouch(120, 480, 120);

can not you do something like

 26       var playCoords;
 27       try {
 28           playCoords = mediaControlsButtonCoordinates(video, "play-button");
 29       } catch (exception) {
 30           failTest(exception.description);
 31           return;
?

[Kevin Ellis]

Created attachment 149527 [details]
Patch

[Kevin Ellis]

*** Bug 89674 has been marked as a duplicate of this bug. ***

[Kevin Ellis]

Created attachment 149531 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 148557 [details]
Patch

Newer patch available.

[WebKit Review Bot]

Comment on attachment 149531 [details]
Patch

Clearing flags on attachment: 149531

Committed r121305: <http://trac.webkit.org/changeset/121305>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.