Bug 100833 – [chromium] Two touchadjustment tests are failing on mac

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Pavel Podivilov]

touchadjustment/html-label.html and touchadjustment/nested-touch.html started to fail on mac after r132929.

FAIL adjusted node should be A#mylink. Was LABEL#mylabel.

[Rick Byers]

Shoot, not sure how that happened - I'll look into it ASAP

[Rick Byers]

Just to clarify, this is chromium mac.  touchadjustment tests aren't run at all for the mac port of WebKit.

I see the failures, eg. here: http://build.webkit.org/results/Chromium%20Mac%20Release%20%28Tests%29/r132992%20%2827175%29/touchadjustment/html-label-diff.txt

The revision range here is huge though (r132751 to r132992) - just out of curiosity (not that I'm really doubting my change is what broke this), do we have more fine grained test results somewhere?  Why do we have a gap of >240 commits?

[Pavel Podivilov]

(In reply to comment #2)
> Just to clarify, this is chromium mac.  touchadjustment tests aren't run at all for the mac port of WebKit.

Yes, it's chromium, see the bug title.

> 
> I see the failures, eg. here: http://build.webkit.org/results/Chromium%20Mac%20Release%20%28Tests%29/r132992%20%2827175%29/touchadjustment/html-label-diff.txt
> 
> The revision range here is huge though (r132751 to r132992) - just out of curiosity (not that I'm really doubting my change is what broke this), do we have more fine grained test results somewhere?  Why do we have a gap of >240 commits?

The best way to track layout test failures is to use our flakiness dashboard:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=touchadjustment%2Fhtml-label.html%2Ctouchadjustment%2Fnested-touch.html.

It shows results from chrome.webkit canary bots (http://build.chromium.org/p/chromium.webkit/console).

Click on the rightmost red bar for e.g. mac 10.6 to get the WebKit blamelist for the failure (http://trac.webkit.org/log/?verbose=on&rev=132930&stop_rev=132929).

Not sure about webkit.org chromium bots, me guess is most people just ignore them.

[Rick Byers]

(In reply to comment #3)
> (In reply to comment #2)
> > Just to clarify, this is chromium mac.  touchadjustment tests aren't run at all for the mac port of WebKit.
> 
> Yes, it's chromium, see the bug title.

Yeah, I just added [chromium] (wasn't present initially).
 
> > 
> > I see the failures, eg. here: http://build.webkit.org/results/Chromium%20Mac%20Release%20%28Tests%29/r132992%20%2827175%29/touchadjustment/html-label-diff.txt
> > 
> > The revision range here is huge though (r132751 to r132992) - just out of curiosity (not that I'm really doubting my change is what broke this), do we have more fine grained test results somewhere?  Why do we have a gap of >240 commits?
> 
> The best way to track layout test failures is to use our flakiness dashboard:
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=touchadjustment%2Fhtml-label.html%2Ctouchadjustment%2Fnested-touch.html.
> 
> It shows results from chrome.webkit canary bots (http://build.chromium.org/p/chromium.webkit/console).
> 
> Click on the rightmost red bar for e.g. mac 10.6 to get the WebKit blamelist for the failure (http://trac.webkit.org/log/?verbose=on&rev=132930&stop_rev=132929).
> 
> Not sure about webkit.org chromium bots, me guess is most people just ignore them.

(In reply to comment #3)
> (In reply to comment #2)
> > Just to clarify, this is chromium mac.  touchadjustment tests aren't run at all for the mac port of WebKit.
> 
> Yes, it's chromium, see the bug title.
> 
> > 
> > I see the failures, eg. here: http://build.webkit.org/results/Chromium%20Mac%20Release%20%28Tests%29/r132992%20%2827175%29/touchadjustment/html-label-diff.txt
> > 
> > The revision range here is huge though (r132751 to r132992) - just out of curiosity (not that I'm really doubting my change is what broke this), do we have more fine grained test results somewhere?  Why do we have a gap of >240 commits?
> 
> The best way to track layout test failures is to use our flakiness dashboard:
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=touchadjustment%2Fhtml-label.html%2Ctouchadjustment%2Fnested-touch.html.
> 
> It shows results from chrome.webkit canary bots (http://build.chromium.org/p/chromium.webkit/console).
> 
> Click on the rightmost red bar for e.g. mac 10.6 to get the WebKit blamelist for the failure (http://trac.webkit.org/log/?verbose=on&rev=132930&stop_rev=132929).
> 
> Not sure about webkit.org chromium bots, me guess is most people just ignore them.

Perfect, thank you for the details!

[Rick Byers]

As far as I can tell these tests are legitimately failing due to a problem with touch adjustment.  The failures we're previously masked - before my fix the code relied on clientWidth/clientHeight which are always 0 for these inline elements.  So rather than touch the center of the element as desired, we were always touching the top-left corner.  Now that we're actually touching the center, we're much closer to the node below and apparently the touch adjustment score is high enough that we're adjusting to it.

I don't know the touch adjustment algorithm well enough myself to say whether or not it's reasonable that we're adjusting to this other node.  In this case it's the smaller node and so we have a higher percentage overlap.

As for why we're getting different results on Mac and Linux: I suspect this is due to the differences in text metrics.  In particular, for html-label on Mac the A#mylink node is 18px tall, with 2px space below it before the top of the LABEL#mylabel node.  On Linux it's 19px tall with 4px space.  This means that on Mac the distance from the touch to the mylabel element is 2.5 pixels shorter, giving it a higher touch adjustment score.

[Rick Byers]

With the fix to bug 101046 the behavior of html-label is now different.  The direct hit is succeeding (which is an improvement), but the indirect hit on the same element is now failing.  It looks to me like we're now preferring the larger link above due to the greater overlap.  This seems reasonable to me since the label is nice and wide so the potential overlap is high.  So now that bug 101046 is fixed, the right thing to do is just to adjust the test.

[Rick Byers]

As for the nested-touch test, this started failing with my change because using getBoundingClientRect to get the size of the div included the border (1px on all edges), where the previous approach (clientWidth/clientHeight) didn't.  So what was already a very borderline case (touching 10 pixels below the box) got pushed over the edge by being slightly lower.  Again, the right change is to adjust the test to compensate.

[Rick Byers]

Created attachment 172221 [details]
Patch

[Allan Sandfeld Jensen]

(In reply to comment #5)
> As for why we're getting different results on Mac and Linux: I suspect this is due to the differences in text metrics.  In particular, for html-label on Mac the A#mylink node is 18px tall, with 2px space below it before the top of the LABEL#mylabel node.  On Linux it's 19px tall with 4px space.  This means that on Mac the distance from the touch to the mylabel element is 2.5 pixels shorter, giving it a higher touch adjustment score.

Perhaps a more robust solution would be changing the font to Ahem, so the tests are always the same between platforms? I did that to a few of the other touch adjustment tests to get around similar problems.

[Rick Byers]

Created attachment 172244 [details]
Patch

[Rick Byers]

(In reply to comment #9)
> (In reply to comment #5)
> > As for why we're getting different results on Mac and Linux: I suspect this is due to the differences in text metrics.  In particular, for html-label on Mac the A#mylink node is 18px tall, with 2px space below it before the top of the LABEL#mylabel node.  On Linux it's 19px tall with 4px space.  This means that on Mac the distance from the touch to the mylabel element is 2.5 pixels shorter, giving it a higher touch adjustment score.
> 
> Perhaps a more robust solution would be changing the font to Ahem, so the tests are always the same between platforms? I did that to a few of the other touch adjustment tests to get around similar problems.

Good call, I've done that now.  The new metrics made the targets even harder to hit (eg. the html-label indirect label case went from working at up to 4px outside the node to working at up to only 1px outside.  

I saw that you are using line-height: 20px in some of the other tests.  I initially did that too (it would work up to 8px outside the node), but I decided that was a bad idea.  Our biggest problem with touch adjustment in chrome has been when something that would have worked fine without adjustment no longer works (like the plugin boundary case kevers recently fixed).  So top priority for me is that we have good coverage of the direct hit cases in realistic scenarios.  So I'd rather not add artificial padding between the lines - reducing the coverage of the direct-hit tests.  The fact that touching just outside this label will fuzz up to the link when it's more than 1px to the right of the label seems fine to me.  So I'll just test that (the key thing is that it's not fuzzing up until the center point is off of the label).

[Rick Byers]

Created attachment 172245 [details]
Patch

[Allan Sandfeld Jensen]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #5)
> > > As for why we're getting different results on Mac and Linux: I suspect this is due to the differences in text metrics.  In particular, for html-label on Mac the A#mylink node is 18px tall, with 2px space below it before the top of the LABEL#mylabel node.  On Linux it's 19px tall with 4px space.  This means that on Mac the distance from the touch to the mylabel element is 2.5 pixels shorter, giving it a higher touch adjustment score.
> > 
> > Perhaps a more robust solution would be changing the font to Ahem, so the tests are always the same between platforms? I did that to a few of the other touch adjustment tests to get around similar problems.
> 
> Good call, I've done that now.  The new metrics made the targets even harder to hit (eg. the html-label indirect label case went from working at up to 4px outside the node to working at up to only 1px outside.  
> 
> I saw that you are using line-height: 20px in some of the other tests.  I initially did that too (it would work up to 8px outside the node), but I decided that was a bad idea.  Our biggest problem with touch adjustment in chrome has been when something that would have worked fine without adjustment no longer works (like the plugin boundary case kevers recently fixed).  So top priority for me is that we have good coverage of the direct hit cases in realistic scenarios. 

Most of the tests have been made to test specific new improvements or discovered bugs, we might actually be missing good test coverage of realistic 'standard' cases.

[Allan Sandfeld Jensen]

Comment on attachment 172245 [details]
Patch

Looks good to me (as a non-reviewer).

[Rick Byers]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (In reply to comment #5)
> > > > As for why we're getting different results on Mac and Linux: I suspect this is due to the differences in text metrics.  In particular, for html-label on Mac the A#mylink node is 18px tall, with 2px space below it before the top of the LABEL#mylabel node.  On Linux it's 19px tall with 4px space.  This means that on Mac the distance from the touch to the mylabel element is 2.5 pixels shorter, giving it a higher touch adjustment score.
> > > 
> > > Perhaps a more robust solution would be changing the font to Ahem, so the tests are always the same between platforms? I did that to a few of the other touch adjustment tests to get around similar problems.
> > 
> > Good call, I've done that now.  The new metrics made the targets even harder to hit (eg. the html-label indirect label case went from working at up to 4px outside the node to working at up to only 1px outside.  
> > 
> > I saw that you are using line-height: 20px in some of the other tests.  I initially did that too (it would work up to 8px outside the node), but I decided that was a bad idea.  Our biggest problem with touch adjustment in chrome has been when something that would have worked fine without adjustment no longer works (like the plugin boundary case kevers recently fixed).  So top priority for me is that we have good coverage of the direct hit cases in realistic scenarios. 
> 
> Most of the tests have been made to test specific new improvements or discovered bugs, we might actually be missing good test coverage of realistic 'standard' cases.

Makes sense.  Kevers has been talking about maybe doing some work here, and I'd also love to see us use our metrics system in chrome to track the effectiveness of touch adjustment in the wild (eg. how often users appear to refine their touch by touching a second time).  I'm not sure what the right cost-benefit trade off is though - it works quite well in practice most of the time :-)

[Rick Byers]

(In reply to comment #14)
> (From update of attachment 172245 [details])
> Looks good to me (as a non-reviewer).

Thanks!  Antonio?

[WebKit Review Bot]

Comment on attachment 172245 [details]
Patch

Clearing flags on attachment: 172245

Committed r133636: <http://trac.webkit.org/changeset/133636>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.