Bug 89674 – Cannot open calendar picker for input type=date using a touch tap gesture if TOUCH_ADJUSTMENT is enabled.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Kevin Ellis]

Cannot open calendar picker for input type=date using a touch tap gesture if TOUCH_ADJUSTMENT is enabled.

[Kevin Ellis]

Created attachment 148831 [details]
Patch

[Allan Sandfeld Jensen]

*** This bug has been marked as a duplicate of bug 89556 ***

[Allan Sandfeld Jensen]

Comment on attachment 148831 [details]
Patch

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

Looks mostly good, but needs to be merged with my similar patch in 89556, and we need to decide if the returned targetNode should be the shadow-element or the shadow-ancestor.

> Source/WebCore/page/EventHandler.cpp:2493
> +    bool success = findBestClickableCandidate(targetNode, targetPoint, touchCenter, touchRect, *nodeList.get());
> +    if (success && targetNode)
> +        targetNode = targetNode->shadowAncestorNode();
> +    return success;

I am unsure about this change. For point-adjustment it doesn't matter, but we want to fix the problem of performing unnecesary later hit-testing later and just use the found target node. If we return a shadowAncestorNode instead of the shadow element, we would have the same problem again then.

[Kevin Ellis]

Comment on attachment 148831 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2493
>> +    return success;
> 
> I am unsure about this change. For point-adjustment it doesn't matter, but we want to fix the problem of performing unnecesary later hit-testing later and just use the found target node. If we return a shadowAncestorNode instead of the shadow element, we would have the same problem again then.

Was erring on the side of caution.  Without this node adjustment, we introduce a regression in touchadjustment/html_label.html.  I see that you have addressed the change in behavior in your patch for 89556.  Happy to back out the changes to event handler and land the test portion of the patch after your patch, or alternatively it might make sense to push this change in first and then back out the node adjustment in your more detailed patch.  Option 3 is to merge the nested shadow node test into your patch.

[Kevin Ellis]

Reopening to attach new patch.

[Kevin Ellis]

Created attachment 149020 [details]
Patch

[Kevin Ellis]

Comment on attachment 148831 [details]
Patch

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

>>> Source/WebCore/page/EventHandler.cpp:2493
>>> +    return success;
>> 
>> I am unsure about this change. For point-adjustment it doesn't matter, but we want to fix the problem of performing unnecesary later hit-testing later and just use the found target node. If we return a shadowAncestorNode instead of the shadow element, we would have the same problem again then.
> 
> Was erring on the side of caution.  Without this node adjustment, we introduce a regression in touchadjustment/html_label.html.  I see that you have addressed the change in behavior in your patch for 89556.  Happy to back out the changes to event handler and land the test portion of the patch after your patch, or alternatively it might make sense to push this change in first and then back out the node adjustment in your more detailed patch.  Option 3 is to merge the nested shadow node test into your patch.

Added FIXME to remove need for shadowAncestorNode.

[Antonio Gomes]

Comment on attachment 149020 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2494
> +    // FIXME: Should be able to handle targetNode being a shadow DOM node to avoid performing uncessary hit tests
> +    // in the case where further processing on the node is required. Returning the shadow ancestor prevents a
> +    // regression in touchadjustment/html-label.html. Some refinement is required to testing/internals to
> +    // handle targetNode being a shadow DOM node (see bug 69556). 

This is supported in HitTestResult side. The BlackBerry port, for example, only works with shadow content, if they have a media element (<video> or <audio>) as ancestor.

The bug 69556 does not seem related. Maybe wrong bug ID?

> LayoutTests/touchadjustment/nested-shadow-node.html:37
> +    function addShadowDom() {

Dom => DOM

> LayoutTests/touchadjustment/nested-shadow-node.html:81
> +        var left = touchX - padding/2;
> +        var top = touchY - padding/2;

nit: space before/after "/"

[Kevin Ellis]

Created attachment 149081 [details]
Patch

[Kevin Ellis]

Comment on attachment 149020 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2494
>> +    // handle targetNode being a shadow DOM node (see bug 69556). 
> 
> This is supported in HitTestResult side. The BlackBerry port, for example, only works with shadow content, if they have a media element (<video> or <audio>) as ancestor.
> 
> The bug 69556 does not seem related. Maybe wrong bug ID?

Wrong ID.  Corrected to 89556 (Touch adjustment does not target shadow DOM elements).

>> LayoutTests/touchadjustment/nested-shadow-node.html:37
>> +    function addShadowDom() {
> 
> Dom => DOM

Done.

>> LayoutTests/touchadjustment/nested-shadow-node.html:81
>> +        var top = touchY - padding/2;
> 
> nit: space before/after "/"

Done.

[Allan Sandfeld Jensen]

(In reply to comment #9)
> Created an attachment (id=149081) [details]
> Patch

Even though we plan to land it is a two patches, I would still prefer if you closed this bug as a duplicated, and landed the patch under the original bug. They are the same bug-report, and the title is more accurate in the older bug.

[Kevin Ellis]

Test added to 89556.

*** This bug has been marked as a duplicate of bug 89556 ***

[Adam Barth]

Comment on attachment 148831 [details]
Patch

Cleared review? from obsolete attachment 148831 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Adam Barth]

Comment on attachment 149081 [details]
Patch

Cleared review? from attachment 149081 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Adam Barth]

Comment on attachment 149020 [details]
Patch

Cleared review? from obsolete attachment 149020 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).