Issue 370843002: Allow touch adjustment to return Shadow DOM node.

Issue 370843002: Allow touch adjustment to return Shadow DOM node. (Closed)

Created:
6 years, 5 months ago by Zeeshan Qureshi

Modified:
6 years, 5 months ago

Reviewers:
esprehn, Rick Byers

CC:
blink-reviews, arv+blink, eae+blinkwatch, blink-reviews-events_chromium.org, dglazkov+blink, Inactive, watchdog-blink-watchlist_google.com

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

Allow touch adjustment to return Shadow DOM node. Touch adjustment currently considers a shadow DOM node when finding the best clickable one but then walks up the tree to the shadow host and returns it. Make it return the shadow node instead so that we don't have to hit test again when processing a gesture tap. BUG=391524 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177885

Patch Set 1 #

Patch Set 2 : Add simple shadow dom test #

Total comments: 14

Patch Set 3 : Address comments #

Patch Set 4 : Remove unnecessary internals api #

Total comments: 5

Patch Set 5 : Use testTouchPoint #

Created: 6 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+83 lines, -30 lines)			Patch
M	LayoutTests/touchadjustment/disabled-formelements.html	View	1 2 3	2 chunks	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/touchadjustment/editable-content.html	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/touchadjustment/html-label.html	View	1 2 3	1 chunk	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/touchadjustment/nested-shadow-node.html	View	1 2 3 4	2 chunks	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/touchadjustment/nested-shadow-node-expected.txt	View	1 2 3 4	1 chunk	+5 lines, -5 lines	0 comments	Download
M	LayoutTests/touchadjustment/resources/touchadjustment.js	View	1 2 3 4	1 chunk	+14 lines, -1 line	0 comments	Download
A	LayoutTests/touchadjustment/simple-shadow-dom.html	View	1 2 3	1 chunk	+51 lines, -0 lines	0 comments	Download
A +	LayoutTests/touchadjustment/simple-shadow-dom-expected.txt	View	1	1 chunk	+2 lines, -3 lines	0 comments	Download
M	LayoutTests/touchadjustment/small-target-test.html	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/page/EventHandler.cpp	View	1 2 3 4	1 chunk	+1 line, -11 lines	0 comments	Download

Messages

Total messages: 12 (0 generated)

Expand Messages | Collapse Messages

Zeeshan Qureshi

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustment/nested-shadow-node.html File LayoutTests/touchadjustment/nested-shadow-node.html (right): https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustment/nested-shadow-node.html#newcode55 LayoutTests/touchadjustment/nested-shadow-node.html:55: element = document.getElementById("targetDiv"); This doesn't make sense to me. ...

6 years, 5 months ago (2014-07-05 03:18:31 UTC) #1

Zeeshan Qureshi

https://codereview.chromium.org/370843002/diff/20001/Source/core/page/EventHandler.cpp File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/370843002/diff/20001/Source/core/page/EventHandler.cpp#newcode2494 Source/core/page/EventHandler.cpp:2494: // FIXME: targetNode and success are only used by ...

6 years, 5 months ago (2014-07-05 03:22:13 UTC) #2

Rick Byers

Good work tracking down the source of the focus/OSK problem, thanks! I agree that having ...

6 years, 5 months ago (2014-07-07 18:25:45 UTC) #3

Good work tracking down the source of the focus/OSK problem,  thanks!

I agree that having touch adjustment always return shadow nodes is the right
thing to do.  Now that
HitTestFlags.ConfusingAndOftenMisusedDisallowshadowContent is gone, most event
handling code should be properly handling shadow DOM nodes already, so this
shouldn't break anything (famous last words <grin>).

A couple minor comments.

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
File LayoutTests/touchadjustment/nested-shadow-node.html (right):

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
LayoutTests/touchadjustment/nested-shadow-node.html:55: element =
document.getElementById("targetDiv");
On 2014/07/05 03:18:31, Zeeshan Qureshi wrote:
> This doesn't make sense to me. This should be the shadowDiv as far as I can
see.

Shadow DOM provides an encapsulation boundary.  shadowDiv is inside the shadow
tree and so won't be directly accessible to the code here.  This test is
verifying that the fact that shadow DOM is used here is really an implementation
detail the outside code is unaware of - the key property you need to maintain
with your change here.

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
File LayoutTests/touchadjustment/resources/touchadjustment.js (right):

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
LayoutTests/touchadjustment/resources/touchadjustment.js:69: if (!allowShadowDOM
&& adjustedNode && adjustedNode.nodeType == 1) {
There's probably only the one test that actually relies on adjusting into shadow
DOM.  Perhaps you can make this function ALWAYS allow shadow DOM (as the
internal API now does) and move the logic for properly testing the shadow cases
into the appropriate test cases?  Then you don't have to add this wrapper
function.

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
LayoutTests/touchadjustment/resources/touchadjustment.js:70: while
(internals.shadowHost(adjustedNode)) {
Rather than add shadowHost to internals, you should be able to get the same
thing by using the existing Element.shadowRoot and ShadowRoot.host (or maybe
olderShadowRoot) properties.  See
http://w3c.github.io/webcomponents/spec/shadow/#widl-Element-shadowRoot and
http://w3c.github.io/webcomponents/spec/shadow/#widl-ShadowRoot-host

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
File LayoutTests/touchadjustment/simple-shadow-dom.html (right):

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
LayoutTests/touchadjustment/simple-shadow-dom.html:3: <head>
nit: for new tests we generally avoid adding any unnecessary tags.  You should
be able to omit the html and body tags.

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
LayoutTests/touchadjustment/simple-shadow-dom.html:40:
element.addEventListener('click', function() {}, false);
what are these two handlers for?  Are you just trying to make sure the element
and document are also candidates for touch adjustment?

https://codereview.chromium.org/370843002/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/370843002/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2494: // FIXME: targetNode and success are
only used by Internals functions. We should
On 2014/07/05 03:22:13, Zeeshan Qureshi wrote:
> Will remove this comment too before landing.

Yeah I don't think it's true for 'targetNode' anymore - I changed the code to
store the targetNode in the hitTestResults (probably only when success is true).
 Really this design was a symptom of requiring extra hit tests for touch
adjustment.  So remove away....

Zeeshan Qureshi

Rick, tried to address your comments. https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustment/resources/touchadjustment.js File LayoutTests/touchadjustment/resources/touchadjustment.js (right): https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustment/resources/touchadjustment.js#newcode69 LayoutTests/touchadjustment/resources/touchadjustment.js:69: if (!allowShadowDOM && ...

6 years, 5 months ago (2014-07-09 16:10:15 UTC) #4

Rick, tried to address your comments.

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
File LayoutTests/touchadjustment/resources/touchadjustment.js (right):

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
LayoutTests/touchadjustment/resources/touchadjustment.js:69: if (!allowShadowDOM
&& adjustedNode && adjustedNode.nodeType == 1) {
On 2014/07/07 18:25:44, Rick Byers wrote:
> There's probably only the one test that actually relies on adjusting into
shadow
> DOM.  Perhaps you can make this function ALWAYS allow shadow DOM (as the
> internal API now does) and move the logic for properly testing the shadow
cases
> into the appropriate test cases?  Then you don't have to add this wrapper
> function.

There's a couple of tests that do adjustment on <input> elements and the hit
test traverses into their shadow dom so this seems easier than updating them.

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
LayoutTests/touchadjustment/resources/touchadjustment.js:70: while
(internals.shadowHost(adjustedNode)) {
On 2014/07/07 18:25:44, Rick Byers wrote:
> Rather than add shadowHost to internals, you should be able to get the same
> thing by using the existing Element.shadowRoot and ShadowRoot.host (or maybe
> olderShadowRoot) properties.  See
> http://w3c.github.io/webcomponents/spec/shadow/#widl-Element-shadowRoot and
> http://w3c.github.io/webcomponents/spec/shadow/#widl-ShadowRoot-host

Looks like this is for traversing down the tree.

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
File LayoutTests/touchadjustment/simple-shadow-dom.html (right):

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
LayoutTests/touchadjustment/simple-shadow-dom.html:3: <head>
On 2014/07/07 18:25:44, Rick Byers wrote:
> nit: for new tests we generally avoid adding any unnecessary tags.  You should
> be able to omit the html and body tags.

Oh I thought you needed the tags if you added <head>, removed now.

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustm...
LayoutTests/touchadjustment/simple-shadow-dom.html:40:
element.addEventListener('click', function() {}, false);
On 2014/07/07 18:25:44, Rick Byers wrote:
> what are these two handlers for?  Are you just trying to make sure the element
> and document are also candidates for touch adjustment?

Yes, I borrowed this from the existing shadow dom tests but I'm pretty sure
that's the purpose.

https://codereview.chromium.org/370843002/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/370843002/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2494: // FIXME: targetNode and success are
only used by Internals functions. We should
On 2014/07/07 18:25:44, Rick Byers wrote:
> On 2014/07/05 03:22:13, Zeeshan Qureshi wrote:
> > Will remove this comment too before landing.
> 
> Yeah I don't think it's true for 'targetNode' anymore - I changed the code to
> store the targetNode in the hitTestResults (probably only when success is
true).
>  Really this design was a symptom of requiring extra hit tests for touch
> adjustment.  So remove away....

Done.

Rick Byers

https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustment/resources/touchadjustment.js File LayoutTests/touchadjustment/resources/touchadjustment.js (right): https://codereview.chromium.org/370843002/diff/20001/LayoutTests/touchadjustment/resources/touchadjustment.js#newcode70 LayoutTests/touchadjustment/resources/touchadjustment.js:70: while (internals.shadowHost(adjustedNode)) { On 2014/07/09 16:10:14, Zeeshan Qureshi wrote: ...

6 years, 5 months ago (2014-07-09 19:46:58 UTC) #5

Zeeshan Qureshi

> This has the drawback of being specific to a single-level nesting of input, but ...

6 years, 5 months ago (2014-07-10 17:50:19 UTC) #6

Rick Byers

Nice, this is better - thanks! LGTM with nits https://codereview.chromium.org/370843002/diff/60001/LayoutTests/touchadjustment/nested-shadow-node.html File LayoutTests/touchadjustment/nested-shadow-node.html (right): https://codereview.chromium.org/370843002/diff/60001/LayoutTests/touchadjustment/nested-shadow-node.html#newcode86 LayoutTests/touchadjustment/nested-shadow-node.html:86: ...

6 years, 5 months ago (2014-07-10 20:09:11 UTC) #7

Zeeshan Qureshi

https://codereview.chromium.org/370843002/diff/60001/LayoutTests/touchadjustment/resources/touchadjustment.js File LayoutTests/touchadjustment/resources/touchadjustment.js (right): https://codereview.chromium.org/370843002/diff/60001/LayoutTests/touchadjustment/resources/touchadjustment.js#newcode69 LayoutTests/touchadjustment/resources/touchadjustment.js:69: // Not exactly the best method of detecting shadow ...

6 years, 5 months ago (2014-07-10 20:46:17 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zeeshanq@chromium.org/370843002/80001

6 years, 5 months ago (2014-07-10 20:53:23 UTC) #10

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu on tryserver.chromium.gpu ...

6 years, 5 months ago (2014-07-10 21:54:32 UTC) #11

Message was sent while issue was closed.

Change committed as 177885

Expand Messages | Collapse Messages