Botond Ballo [:botond] Reporter
	Description • 9 years ago

The calculations done in ApplyAsyncTransformToScrollbarForContent (in AsyncCompositionManager.cpp) are fairly involved, and not easy to reason about in one's head.

It would be really good to have regression tests that exercise the various transforms applied in this function, to make sure we don't break things when we touch this code.

	Timothy Nikkel (:tnikkel)
	Comment 1 • 9 years ago

Bug 1160642 adds reftest-async-zoom which may be useful here.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 9 years ago

Pretty sure we can write R-ipc reftests with the current infra to test the scrollbar positioning.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 3 • 9 years ago

First cut of basic async scrollbar reftests. I mostly ripped off tn's work in bug 1133905 and updated some things. Try push shows that these tests pass on B2G except for the two which have a horizontal scrollbar and are RTL. I see the same sort of failure (with the scrollbar going past the end of the screen) when I run the tests using |mach reftest-ipc| locally on Linux, so I think that's a legitimate bug which needs fixing.

Locally on Linux all of the other tests also fail for one or both of a few reasons:
1) The scroll-up and scroll-left arrows are disabled (and therefore a slightly different color) in the async cases because the test starts off with the scroll position at (0,0) and then moves the scrollbars asynchronously to represent a new scroll position without repainting them to reflect the scrollability change. I can modify the test to work around this.
2) What seems like off-by-one in the scroll thumb position. It might just be that in the case of non-overlay scrollbars the thumb position calculation results in a slightly different result async vs sync.
3) Off-by-one pixel colors in the scrollbar button areas. Not sure what that's about, but I'm inclined to just fuzz it.

I'll look into the RTL issue a bit and try to write more tests that include different viewport sizes and zooming.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 9 years ago

Oh, https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1c1b814e44a

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 9 years ago

The horizontal scrollbar with RTL thing is probably my fault for trying to set a scroll-x of 450 on an RTL page. That "works" via reftest-async-scroll-x because there's no bounds checking, but it's nonsensical. I should be using -450 instead for both the test and reference.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 6 • 9 years ago

With that corrected the tests pass on B2G:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f92849ec96b6

I have the tests passing on Linux reftest-ipc (which runs with APZ enabled) as well, but they are failing intermittently. Not sure why, trying to track it down. It would be nice to land them in a passing state for desktop.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 9 years ago

My investigation so far seems to indicate there's a bug in the reftest harness itself which is resulting in my intermittent failure. It looks like the content window gets repainted once even after the reftest-content.js gets the "Completed" MakeProgress state, and that repaint is never reflected in the snapshot taken because there's no UpdateCanvas message sent from reftest-content.js to reftest.js. I'm not fully clear on where this repaint is coming from and why reftest-content.js assumed it was done before the repaint happened. Will keep digging.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 9 years ago

The repaint is coming from a call to SetDisplayPortMargins triggered by an APZC repaint request. Since the displayport margins are different I guess the resulting paint is slightly different too. Now I just need to figure out what the right fix here is...

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 9 years ago

These are just basic root frame async scroll tests. I would like to get these landed asap before continuing on the remaining tests (zooming, subframe, viewport changes).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 10 • 9 years ago

Whoops that was an old version

	Timothy Nikkel (:tnikkel)
	Comment 11 • 9 years ago

Comment on attachment 8605811 [details] [diff] [review]
Basic scrollbar tests

"async-scrollbars" might be a better name for the test files since they don't test the async scrolling of any content.

	Botond Ballo [:botond] Reporter
	Comment 12 • 9 years ago

Comment on attachment 8605811 [details] [diff] [review]
Basic scrollbar tests

Review of attachment 8605811 [details] [diff] [review]:
-----------------------------------------------------------------

Why do the test versions of the files have '1' in them but not the 'ref' versions?

::: gfx/layers/apz/reftests/async-scroll-1-h.html
@@ +3,5 @@
> +      reftest-async-scroll
> +      reftest-async-scroll-x="449" reftest-async-scroll-y="0"><head>
> +<meta name="viewport" content="width=device-width">
> +</head>
> +<!-- Doing scrollTo(1,0) is to activate the up arrow in the scrollbar

left arrow?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 13 • 9 years ago

Renamed the files so that they are of the form

async-scrollbar-1-h.html
async-scrollbar-1-h-ref.html

and so on. Also fixed s/up arrow/left arrow/

	Pulsebot
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fcbaf5a20e6

	Phil Ringnalda (:philor)
	Comment 15 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1fcbaf5a20e6

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 9 years ago

Unfortunately in the zooming scenarios we can't quite get rid of the fuzz because the border-radius on the scrollthumb doesn't look the same when layout paints it larger vs when it's scaled up in the compositor. This is an issue in the production code too.

	Timothy Nikkel (:tnikkel)
	Comment 17 • 9 years ago

Comment on attachment 8607606 [details] [diff] [review]
Zooming scrollbar tests

>+skip-if(!asyncPanZoom||!B2G) fuzzy-if(B2G,77,82) == async-scrollbar-2-vh.html async-scrollbar-2-vh-ref.html
>+skip-if(!asyncPanZoom||!B2G) fuzzy-if(B2G,94,146) == async-scrollbar-3-vh.html async-scrollbar-3-vh-ref.html

Why do we need to skip the tests outside of B2G? Shouldn't asyncPanZoom be enough?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 18 • 9 years ago

Eventually we'll enable asyncPanZoom on desktop platforms and I don't want these tests to run there because we are not planning on supporting zooming on them yet.

	Timothy Nikkel (:tnikkel)
	Comment 19 • 9 years ago

Do you think it's worth it to have a reftest annotation that distinguishes between "async pan only" and "async pan zoom"?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 20 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #19)
> Do you think it's worth it to have a reftest annotation that distinguishes
> between "async pan only" and "async pan zoom"?

I'm not sure. I took a quick look through the existing reftest annotations that mention asyncPanZoom and it's not clear to me which ones would need to be updated to "async pan only" if we did introduce that annotation. I don't know if there's much value in doing that right now, but if we write more tests and it turns out there's a significant chunk which are "pan only" then we can add it.

	Botond Ballo [:botond] Reporter
	Comment 21 • 9 years ago

Comment on attachment 8607606 [details] [diff] [review]
Zooming scrollbar tests

Review of attachment 8607606 [details] [diff] [review]:
-----------------------------------------------------------------

This generally looks good, but the async scroll amounts will need to be revised after making the change to the implementation of reftest-async-zoom that we discussed on IRC:

[13:04] <botond> kats: i find the way the way reftest-async-zoom is implemented a bit counterintuitive
[13:05] <kats> botond: how so?
[13:06] <botond> kats: so reftest-async-scroll-x and -y are stored in AsyncPanZoomController::mTestAsyncScrollOffset, which is a CSSPoint, suggesting that this scroll amount is in CSS pixels
[13:07] <botond> kats: but reftest-async-zoom is multiplied into the scale of the ViewTransform returned by AsyncPanZoomController::GetCurrentAsyncTransform, without being part of the GetZoom() that's used to adjust the translation
[13:07] <botond> kats: so, in the presence of the reftest-async-zoom, the interpretation of scroll-x and scroll-y is actually *post*-scaling offset, which is no longer CSS pixels
[13:08] <kats> botond: true
[13:08] <kats> i can fix that
[13:08] <botond> kats: for example, in the async-scrollbar-2-vh.html, the async-scroll-x=448 does not mean "scroll by 448 css pixels", it means "Scroll by 448 screen pixels after having zoomed"
[13:12] <botond> kats: were you thinking of fixing it by also applying mTestAsyncZoom to the zoom used here: https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp#2753 ?
[13:12] <kats> botond: yeah

::: gfx/layers/apz/reftests/reftest.list
@@ +11,5 @@
> +# currently allow async zooming, that's the only platform on which these tests
> +# are run. And because the scrollthumb gets async-scaled in the compositor, the
> +# border-radius ends of the scrollthumb are going to be a little off, hence the
> +# fuzzy-if clauses.
> +skip-if(!asyncPanZoom||!B2G) fuzzy-if(B2G,77,82) == async-scrollbar-2-vh.html async-scrollbar-2-vh-ref.html

Maybe have 'zoom' somewhere in the test names?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 22 • 9 years ago

I updated the patch on bug 1160642 to apply the reftest-async-zoom multiplier as we discussed. Updating the test accordingly, and also renaming the tests as you suggested.

	Botond Ballo [:botond] Reporter
	Comment 23 • 9 years ago

Comment on attachment 8608700 [details] [diff] [review]
Zooming scrollbar tests (v2)

Review of attachment 8608700 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with this change.

::: gfx/layers/apz/reftests/async-scrollbar-zoom-1.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html class="reftest-wait"
> +      reftest-async-scroll
> +      reftest-async-scroll-x="249" reftest-async-scroll-y="4999"

Shouldn't x be 224?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 24 • 9 years ago

Ah, you're right - nice catch! That also reduces the fuzz needed on that test since more of the pixels line up.

	Pulsebot
	Comment 25 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/004de000947c

	Phil Ringnalda (:philor)
	Comment 26 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/004de000947c

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 27 • 9 years ago

I'm going to close this bug and open a new one for any additional tests.