Closed Bug 1151617 Opened 5 years ago Closed 5 years ago

Write tests that exercise ApplyAsyncTransformToScrollbarForContent

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: botond, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 1139155
See Also: → 1128740
Whiteboard: [gfx-noted]
Bug 1160642 adds reftest-async-zoom which may be useful here.
Pretty sure we can write R-ipc reftests with the current infra to test the scrollbar positioning.
Assignee: nobody → bugmail.mozilla
Depends on: 1160642
No longer depends on: 1139155
Attached patch WIP (obsolete) — Splinter Review
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.
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.
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.
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.
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...
Attached patch Basic scrollbar tests (obsolete) — Splinter Review
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).
Attachment #8603445 - Attachment is obsolete: true
Attachment #8605810 - Flags: review?(tnikkel)
Attachment #8605810 - Flags: review?(botond)
Whoops that was an old version
Attachment #8605810 - Attachment is obsolete: true
Attachment #8605810 - Flags: review?(tnikkel)
Attachment #8605810 - Flags: review?(botond)
Attachment #8605811 - Flags: review?(tnikkel)
Attachment #8605811 - Flags: review?(botond)
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.
Attachment #8605811 - Flags: review?(tnikkel) → review+
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?
Attachment #8605811 - Flags: review?(botond) → review+
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/
Keywords: leave-open
Attached patch Zooming scrollbar tests (obsolete) — Splinter Review
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.
Attachment #8607606 - Flags: review?(tnikkel)
Attachment #8607606 - Flags: review?(botond)
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?
Attachment #8607606 - Flags: review?(tnikkel) → review+
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.
Do you think it's worth it to have a reftest annotation that distinguishes between "async pan only" and "async pan zoom"?
(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.
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?
Attachment #8607606 - Flags: review?(botond) → review-
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.
Attachment #8607606 - Attachment is obsolete: true
Attachment #8608700 - Flags: review?(botond)
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?
Attachment #8608700 - Flags: review?(botond) → review+
Ah, you're right - nice catch! That also reduces the fuzz needed on that test since more of the pixels line up.
I'm going to close this bug and open a new one for any additional tests.
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.