Re: [blink-dev] Touch adjustment questions

[Timothy Dresser]

-blink-dev

+input-dev +Zeeshan Qureshi 

Do you have any data indicating that touch adjustment requires optimization?

On Mon Feb 09 2015 at 1:41:05 PM Antonio Gomes <toni...@gmail.com> wrote:

Hi Rick/blink-dev.

I was writing this on IRC, but given that it became a bit large, I decided to email you and blink-dev instead.

From my understanding, tap highlight today works as following:

1- a rect-based hit test is performed, and then result is passed to touch adjust.

2- touch adjust does some manipulations (which btw look to me a bit tricky, maybe over expensive/complicated given the amount of traversals that happen), and then a best target is found.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/page/TouchAdjustment.cpp&cl=GROK&l=247&gsn=bestClickableNodeForHitTestResult&rcl=1423457752

3- the original hit test result then is manipulated to become point based whose target is the "best node" from tap highlight.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/page/EventHandler.cpp&sq=package:chromium&type=cs&l=2764&rcl=1423457752

4- Then WebViewImpl gets the adjust point based his test result target, and does some more upwards traversals, checking which ancestor has a hand cursor style applied.
 https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/web/WebViewImpl.cpp&q=WebViewImpl::bestTapNode&sq=package:chromium&type=cs&l=1277

5- then it walks its ancestors upwards again to find the biggest ancestor biggest that has a cursor hand style applied.

6- this is our best node, if any.

Some questions:

- steps (4) and (5) fix real world bugs like https://code.google.com/p/chromium/issues/detail?id=322323, but simple cases where I was expecting tap highlight to show up are getting off the road, e.g.:

<input type="button" onclick=""> Click me!!!</button>

Adding style=cursor:pointer "fixes"

- I'd say perf-wise, there could do some simplifications on (2). Is there any ongoing work on that area? I mean I've seen the effort to decrease the amount of hit tests performed on touch handling, but I am specifically talking about "touch adjustment" optimizations.

--Antonio Gomes

[Rick Byers]

On Tue, Feb 10, 2015 at 5:40 AM, Antonio Gomes <toni...@gmail.com> wrote:

Hi Rick/blink-dev.

I was writing this on IRC, but given that it became a bit large, I decided to email you and blink-dev instead.

From my understanding, tap highlight today works as following:

1- a rect-based hit test is performed, and then result is passed to touch adjust.

2- touch adjust does some manipulations (which btw look to me a bit tricky, maybe over expensive/complicated given the amount of traversals that happen), and then a best target is found.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/page/TouchAdjustment.cpp&cl=GROK&l=247&gsn=bestClickableNodeForHitTestResult&rcl=1423457752

Yes this part is pretty tricky.  It's evolved over time to avoid around issues with touch adjustment in practice.  Note that this stage, our primary use case is where to send the 'click' event (I wouldn't want to imply that this work is done for touch highlighting - highlighting just piggy-backs on the targeting logic since we want it to be consistent with the node that will be activated).

 

3- the original hit test result then is manipulated to become point based whose target is the "best node" from tap highlight.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/page/EventHandler.cpp&sq=package:chromium&type=cs&l=2764&rcl=1423457752

As the comment says, I consider this step to be due to bugs in rect-based hit-testing.  Ideally we'd be able to use the rect-based result as is but there's a number of special cases we'd have to work through to make that usable without breaking website compat.  I tried to simplify this and had a long tail of site compat issues that ultimately forced me to add this point-based test back in.

4- Then WebViewImpl gets the adjust point based his test result target, and does some more upwards traversals, checking which ancestor has a hand cursor style applied.
 https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/web/WebViewImpl.cpp&q=WebViewImpl::bestTapNode&sq=package:chromium&type=cs&l=1277

Right, now we're talking explicitly about link highlighting.  This is a heuristic to try to make the highlight match the developers intention about what the conceptually activatable node is.

5- then it walks its ancestors upwards again to find the biggest ancestor biggest that has a cursor hand style applied.

6- this is our best node, if any.

Some questions:

- steps (4) and (5) fix real world bugs like https://code.google.com/p/chromium/issues/detail?id=322323, but simple cases where I was expecting tap highlight to show up are getting off the road, e.g.:

<input type="button" onclick=""> Click me!!!</button>

Adding style=cursor:pointer "fixes"

Agreed.  Perhaps 'cursor: pointer' should be in our default stylesheet for buttons (it is for links, why not buttons?). 

- I'd say perf-wise, there could do some simplifications on (2). Is there any ongoing work on that area? I mean I've seen the effort to decrease the amount of hit tests performed on touch handling, but I am specifically talking about "touch adjustment" optimizations.

There isn't really active work here, no.  Improving touch adjustment (simplicity, performance, effectiveness) has been on my team's back-burner for awhile but it's never really become important enough to be prioritized about the other things we're working on.  I'd love to have someone working on this though.

However, we have been experimenting with the idea of disabling touch adjustment entirely on mobile-optimized sites (i.e. treating it only as a legacy compat thing for sites designed without touch in mind).  If we could do this without degradation of user experience it would be the best possible result in terms of simplification and performance I think.  WDYT?  Current blocker is our default checkboxes are too hard to activate without adjustment, but that should be fixable.  

--Antonio Gomes

[Rick Byers]

On Wed, Feb 11, 2015 at 11:35 AM, Rick Byers <rby...@chromium.org> wrote:

On Tue, Feb 10, 2015 at 5:40 AM, Antonio Gomes <toni...@gmail.com> wrote:

Hi Rick/blink-dev.

I was writing this on IRC, but given that it became a bit large, I decided to email you and blink-dev instead.

From my understanding, tap highlight today works as following:

1- a rect-based hit test is performed, and then result is passed to touch adjust.

2- touch adjust does some manipulations (which btw look to me a bit tricky, maybe over expensive/complicated given the amount of traversals that happen), and then a best target is found.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/page/TouchAdjustment.cpp&cl=GROK&l=247&gsn=bestClickableNodeForHitTestResult&rcl=1423457752

Yes this part is pretty tricky.  It's evolved over time to avoid around issues with touch adjustment in practice.  Note that this stage, our primary use case is where to send the 'click' event (I wouldn't want to imply that this work is done for touch highlighting - highlighting just piggy-backs on the targeting logic since we want it to be consistent with the node that will be activated).

 

3- the original hit test result then is manipulated to become point based whose target is the "best node" from tap highlight.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/page/EventHandler.cpp&sq=package:chromium&type=cs&l=2764&rcl=1423457752

As the comment says, I consider this step to be due to bugs in rect-based hit-testing.  Ideally we'd be able to use the rect-based result as is but there's a number of special cases we'd have to work through to make that usable without breaking website compat.  I tried to simplify this and had a long tail of site compat issues that ultimately forced me to add this point-based test back in.

4- Then WebViewImpl gets the adjust point based his test result target, and does some more upwards traversals, checking which ancestor has a hand cursor style applied.
 https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/web/WebViewImpl.cpp&q=WebViewImpl::bestTapNode&sq=package:chromium&type=cs&l=1277

Right, now we're talking explicitly about link highlighting.  This is a heuristic to try to make the highlight match the developers intention about what the conceptually activatable node is.

5- then it walks its ancestors upwards again to find the biggest ancestor biggest that has a cursor hand style applied.

6- this is our best node, if any.

Some questions:

- steps (4) and (5) fix real world bugs like https://code.google.com/p/chromium/issues/detail?id=322323, but simple cases where I was expecting tap highlight to show up are getting off the road, e.g.:

<input type="button" onclick=""> Click me!!!</button>

Adding style=cursor:pointer "fixes"

Agreed.  Perhaps 'cursor: pointer' should be in our default stylesheet for buttons (it is for links, why not buttons?). 

- I'd say perf-wise, there could do some simplifications on (2). Is there any ongoing work on that area? I mean I've seen the effort to decrease the amount of hit tests performed on touch handling, but I am specifically talking about "touch adjustment" optimizations.

There isn't really active work here, no.  Improving touch adjustment (simplicity, performance, effectiveness) has been on my team's back-burner for awhile but it's never really become important enough to be prioritized about the other things we're working on.  I'd love to have someone working on this though.

I should add that this hasn't shown up as an issue in practice in our perf testing.  It happens twice during tap (once for ShowPress - eg. highlight/:active time, and one at Tap - eg. 'click' event time).  The blink performance model is that the page needs to respond within 100ms.   The actual perf opportunity here is tiny (sub 1ms even on slow devices), so we're really focusing our perf efforts on things you might want to do every frame (where the budget is much much smaller).

That said, the simplicity and rationality arguments still stand.  And small perf wins are always nice to have, no matter how small.

[Elliott Sprehn]

On Tue, Feb 10, 2015 at 4:35 PM, Rick Byers <rby...@chromium.org> wrote:

...

Some questions:

- steps (4) and (5) fix real world bugs like https://code.google.com/p/chromium/issues/detail?id=322323, but simple cases where I was expecting tap highlight to show up are getting off the road, e.g.:

<input type="button" onclick=""> Click me!!!</button>

Adding style=cursor:pointer "fixes"

Agreed.  Perhaps 'cursor: pointer' should be in our default stylesheet for buttons (it is for links, why not buttons?). 

That's not web compatible, and it's not how normal native widgets work. When you hover a native button the cursor doesn't turn into a pointer.

 

[Antonio Gomes]

Hi Rick. Comments below. Cheers,

On Tue, Feb 10, 2015 at 8:35 PM, Rick Byers <rby...@chromium.org> wrote:

On Tue, Feb 10, 2015 at 5:40 AM, Antonio Gomes <toni...@gmail.com> wrote:

Hi Rick/blink-dev.
(...)

1- a rect-based hit test is performed, and then result is passed to touch adjust.

2- touch adjust does some manipulations (which btw look to me a bit tricky, maybe over expensive/complicated given the amount of traversals that happen), and then a best target is found.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/page/TouchAdjustment.cpp&cl=GROK&l=247&gsn=bestClickableNodeForHitTestResult&rcl=1423457752

Yes this part is pretty tricky.  It's evolved over time to avoid around issues with touch adjustment in practice.  Note that this stage, our primary use case is where to send the 'click' event (I wouldn't want to imply that this work is done for touch highlighting - highlighting just piggy-backs on the targeting logic since we want it to be consistent with the node that will be activated).

 

I see. A simplification of this code would also have to consider double tap zoom and context menu targets calculations, etc

 

Back of the old WebKit/BackBerry times (which was removed from webkit.org upstream last year), the solution used to consist in:

- rect hit test returns all nodes interesting with the given area.
- a routine  traversed that list, skipping non-clickable elements, and checking which clickable element had the biggest intersection area with  the given touch area.

- target point was adjusted to the middle of the intersected area
- (explicitly/implicitly z-ordering layers were taken into account).

Also at some point there were some discussion that the visual feedback of "active state" was something that was wanted to avoid to users when tapping.

I explain: when something clickable was tapped, the highlight was the only visual feedback users would get. The "active state" where clicked links used to change its font color to "red" did not happen.

It was visually clean to users.

3- the original hit test result then is manipulated to become point based whose target is the "best node" from tap highlight.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/page/EventHandler.cpp&sq=package:chromium&type=cs&l=2764&rcl=1423457752

As the comment says, I consider this step to be due to bugs in rect-based hit-testing.  Ideally we'd be able to use the rect-based result as is but there's a number of special cases we'd have to work through to make that usable without breaking website compat.  I tried to simplify this and had a long tail of site compat issues that ultimately forced me to add this point-based test back in.

That (the root bug) is something good to fix at the hit test level, as you said. I will try to look at it.

 

4- Then WebViewImpl gets the adjust point based his test result target, and does some more upwards traversals, checking which ancestor has a hand cursor style applied.
 https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/web/WebViewImpl.cpp&q=WebViewImpl::bestTapNode&sq=package:chromium&type=cs&l=1277

Right, now we're talking explicitly about link highlighting.  This is a heuristic to try to make the highlight match the developers intention about what the conceptually activatable node is.

5- then it walks its ancestors upwards again to find the biggest ancestor biggest that has a cursor hand style applied.

6- this is our best node, if any.

Some questions:

- steps (4) and (5) fix real world bugs like https://code.google.com/p/chromium/issues/detail?id=322323, but simple cases where I was expecting tap highlight to show up are getting off the road, e.g.:

<input type="button" onclick=""> Click me!!!</button>

Adding style=cursor:pointer "fixes"

I shared Elliot's dislike on this solution, though it would work I think.

What bothers me with the current Touch adjustment algorithm is that text nodes are not excluded right way from the valid clickage targets list.

If I have have

<input type=button>click me!</button>

#text: "click me!" is what is returned as the clickable target from TouchAdjustment. This is conceptually wrong, I would say.

But as mentioned, even if this is fixed, and <input> was returned, it would continue not being highlighted because it has no cursor:hand style by default.

Agreed.  Perhaps 'cursor: pointer' should be in our default stylesheet for buttons (it is for links, why not buttons?). 

- I'd say perf-wise, there could do some simplifications on (2). Is there any ongoing work on that area? I mean I've seen the effort to decrease the amount of hit tests performed on touch handling, but I am specifically talking about "touch adjustment" optimizations.

There isn't really active work here, no.  Improving touch adjustment (simplicity, performance, effectiveness) has been on my team's back-burner for awhile but it's never really become important enough to be prioritized about the other things we're working on.  I'd love to have someone working on this though.

Does blink have good layout test coverage for the compat cases you mentioned above? If so, I would like to experiment with an alternative algorithm/approach.

 

However, we have been experimenting with the idea of disabling touch adjustment entirely on mobile-optimized sites (i.e. treating it only as a legacy compat thing for sites designed without touch in mind).  If we could do this without degradation of user experience it would be the best possible result in terms of simplification and performance I think.  WDYT? 

Sounds like a good thing.
 

Current blocker is our default checkboxes are too hard to activate without adjustment, but that should be fixable.  

Agreed too.

--

--Antonio Gomes

[Rick Byers]

On Mon, Feb 16, 2015 at 3:11 PM, Antonio Gomes <toni...@gmail.com> wrote:

Hi Rick. Comments below. Cheers,

On Tue, Feb 10, 2015 at 8:35 PM, Rick Byers <rby...@chromium.org> wrote:

On Tue, Feb 10, 2015 at 5:40 AM, Antonio Gomes <toni...@gmail.com> wrote:

Hi Rick/blink-dev.
(...)
1- a rect-based hit test is performed, and then result is passed to touch adjust.

2- touch adjust does some manipulations (which btw look to me a bit tricky, maybe over expensive/complicated given the amount of traversals that happen), and then a best target is found.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/page/TouchAdjustment.cpp&cl=GROK&l=247&gsn=bestClickableNodeForHitTestResult&rcl=1423457752

Yes this part is pretty tricky.  It's evolved over time to avoid around issues with touch adjustment in practice.  Note that this stage, our primary use case is where to send the 'click' event (I wouldn't want to imply that this work is done for touch highlighting - highlighting just piggy-backs on the targeting logic since we want it to be consistent with the node that will be activated).

 

I see. A simplification of this code would also have to consider double tap zoom and context menu targets calculations, etc

 

Back of the old WebKit/BackBerry times (which was removed from webkit.org upstream last year), the solution used to consist in:

- rect hit test returns all nodes interesting with the given area.
- a routine  traversed that list, skipping non-clickable elements, and checking which clickable element had the biggest intersection area with  the given touch area.

- target point was adjusted to the middle of the intersected area
- (explicitly/implicitly z-ordering layers were taken into account).

Also at some point there were some discussion that the visual feedback of "active state" was something that was wanted to avoid to users when tapping.

I explain: when something clickable was tapped, the highlight was the only visual feedback users would get. The "active state" where clicked links used to change its font color to "red" did not happen.

It was visually clean to users.

Interesting.  We (or at least I) consider the active state as the primary feedback mechanism (since that's the core customizable part of the platform), and the highlight as more of a compat heuristic for sites not well designed for mobile.  I expect well-designed mobile sites to disable the highlight and rely on their own :active (or touchstart for compat with Safari) affordance instead.  In fact, in retrospect I think many of us would have preferred that we implement the touch highlight as a default style somehow for :active effects (with new standard web APIs to reproduce/customize/disable it).

3- the original hit test result then is manipulated to become point based whose target is the "best node" from tap highlight.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/page/EventHandler.cpp&sq=package:chromium&type=cs&l=2764&rcl=1423457752

As the comment says, I consider this step to be due to bugs in rect-based hit-testing.  Ideally we'd be able to use the rect-based result as is but there's a number of special cases we'd have to work through to make that usable without breaking website compat.  I tried to simplify this and had a long tail of site compat issues that ultimately forced me to add this point-based test back in.

That (the root bug) is something good to fix at the hit test level, as you said. I will try to look at it.

Thanks!  Do you recall if this was the intention of the original rect-based hit test design you landed in webkit?  Eg. we've found a bunch of places that look at the innerNode for a rect-based HitTestResult - probably a bug today since the innerNode (and other properties) are, I think, just one of many possible results of the rect-based test (not necessarily the best or one at the center point of the hit-test).  I think bokan@ was looking at this most recently.

4- Then WebViewImpl gets the adjust point based his test result target, and does some more upwards traversals, checking which ancestor has a hand cursor style applied.
 https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/web/WebViewImpl.cpp&q=WebViewImpl::bestTapNode&sq=package:chromium&type=cs&l=1277

Right, now we're talking explicitly about link highlighting.  This is a heuristic to try to make the highlight match the developers intention about what the conceptually activatable node is.

5- then it walks its ancestors upwards again to find the biggest ancestor biggest that has a cursor hand style applied.

6- this is our best node, if any.

Some questions:

- steps (4) and (5) fix real world bugs like https://code.google.com/p/chromium/issues/detail?id=322323, but simple cases where I was expecting tap highlight to show up are getting off the road, e.g.:

<input type="button" onclick=""> Click me!!!</button>

Adding style=cursor:pointer "fixes"

I shared Elliot's dislike on this solution, though it would work I think.

Elliot's point was a good one - we shouldn't change the experience so that it differs from the native one here.  Our 'cursor' hack was added to avoid a number of over-aggressive highlight cases (highlights being too big and visually distracting).  We figured (given our goals above) that it was better to err on the side of not showing the highlight at all than to show it inappropriately.

A better fix would probably be to explicitly include <button> like 'pointer: cursor' in the highlight selection algorithm.  It seems reasonable to have a 'isElementExplicitlyClickable' test that looks at signals like this.

What bothers me with the current Touch adjustment algorithm is that text nodes are not excluded right way from the valid clickage targets list.

If I have have

<input type=button>click me!</button>

#text: "click me!" is what is returned as the clickable target from TouchAdjustment. This is conceptually wrong, I would say.

But what about cases where the text overhangs it's container?  If we simply excluded text nodes right away as valid targets, then tapping on such cases (eg. http://jsbin.com/gonapo/1/edit) wouldn't match the behavior of clicking with a mouse, right?

Also, we went to some effort to try to highlight line-broken links well (with an independent highlight rect per line box - see http://jsbin.com/fazemo).  If we returned the anchor element instead of the text node we might need to be careful to ensure we don't end up highlighting the bounding box in such cases.  I don't know the details offhand though, perhaps this isn't a problem.

But as mentioned, even if this is fixed, and <input> was returned, it would continue not being highlighted because it has no cursor:hand style by default.

Agreed.  Perhaps 'cursor: pointer' should be in our default stylesheet for buttons (it is for links, why not buttons?). 

- I'd say perf-wise, there could do some simplifications on (2). Is there any ongoing work on that area? I mean I've seen the effort to decrease the amount of hit tests performed on touch handling, but I am specifically talking about "touch adjustment" optimizations.

There isn't really active work here, no.  Improving touch adjustment (simplicity, performance, effectiveness) has been on my team's back-burner for awhile but it's never really become important enough to be prioritized about the other things we're working on.  I'd love to have someone working on this though.

Does blink have good layout test coverage for the compat cases you mentioned above? If so, I would like to experiment with an alternative algorithm/approach.

We've got some coverage, but I wouldn't say it's great (as we fix specific cases we add tests for them, but haven't re-evaluated the test coverage of the space).  I'd absolutely welcome some experimentation here, even though we likely risk some regression (worst case we can revert).  You probably have more context on the adjustment code than any of us do (it's still largely just inherited from WebKit with some tweaks), so I'm happy to trust your judgement on how to proceed (given chromium's goals of course).  Feel free to shoot any questions to input-dev or #chromium-input-dev (me, mustaq@ and bokan@ in particular) - I'm happy to do our best to answer questions and try to predict potential issues.

However, we have been experimenting with the idea of disabling touch adjustment entirely on mobile-optimized sites (i.e. treating it only as a legacy compat thing for sites designed without touch in mind).  If we could do this without degradation of user experience it would be the best possible result in terms of simplification and performance I think.  WDYT? 

Sounds like a good thing.
 

Current blocker is our default checkboxes are too hard to activate without adjustment, but that should be fixable.  

Agreed too.

Great.  I filed a bug to track this.  First step I think is to look at the layout size of checkboxes in other mobile browsers.  I know on desktop there was concern that the default layout size was so standard that we might break sites if we increased it.  Perhaps that's not true in mobile browsers?

--

--Antonio Gomes