1917985 - Apply scroll snapping without external offsets during scene building

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, task)

Comment 1

[Glenn Watson [:gw]]

Previously, we removed external_scroll_offset from snapping calculations during scene building, relying on the fact that Gecko was currently supplying snapped offsets here. However, we want to remove the snapping at this point from Gecko, and have it applied by WR itself during frame building to allow fractional scroll offsets.

Now that https://bugzilla.mozilla.org/show_bug.cgi?id=1887125 has landed, we can move to the next step in WR.

To do this, we'll apply a snap inside WR when the external_scroll_offset + APZ scroll offset is accumulated, before the final transform for the spatial node is calculated. If the existing and new tests from #1887125 pass after that change, that may be all that is required. If that causes test failures, further investigation will be needed to see why that doesn't work and how we can resolve it while retaining correct device pixel snapping.

Comment 2

[Markus Stange [:mstange]]

That all sounds great to me. I would add one thing:

(In reply to Glenn Watson [:gw] from comment #1)

If the existing and new tests from #1887125 pass after that change, that may be all that is required.

I'd say: "[...] after that change and with layout.scroll.disable-pixel-alignment flipped to true, [...]".

Comment 3

[Glenn Watson [:gw]]

Attachment: Bug 1917985 - Apply scroll snapping without external offsets during scene building

Apply the external scroll offset normalization prior to snapping
during scene building. This makes snapping be unaffected by the
external scroll offset, which may now be fractional.

Enable layout.scroll.disable-pixel-alignment and update the
tests for this case.

Note that although this makes several tests pass, I had to add
fails-if() on draw snapshot mode for a couple of the tests. I believe
this is because that drawing path doesn't correctly handle the
layout.scroll.disable-pixel-alignment option.

Comment 4

[Glenn Watson [:gw]]

The patch enables that pref and passes all of the snap/scroll tests. However, it seems to make some existing APZ tests become perma-fails.

I kicked off a try run with just that preference enabled, but nothing else from my patch, and that alone is enough to cause these APZ perma-fails. In this try run https://treeherder.mozilla.org/jobs?repo=try&revision=06698dc5ac3846239df3143fc3d274d9b2d4ed99, https://treeherder.mozilla.org/logviewer?job_id=492226734&repo=try&lineNumber=6457 and https://treeherder.mozilla.org/logviewer?job_id=492219394&repo=try&lineNumber=9224 are some of the APZ tests that become perma-fails.

So I think we'll need someone on the APZ team to investigate these and fix them before we can land the fractional scroll work? Alternatively I could land my patch with the pref disabled, and change the test expectations, and the pref could be enabled at a later time when APZ team can investigate those perma-fails?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I did look at a couple of test failures, they use Element.scrollTop which has also a problem (bug 1674687). In short the attribute needs to be double but it's long. So I guess in this check

    is(subframe.scrollTop > 0, true, "We should have scrolled the subframe down")

With pixel alignment, the subframe.scrollTop would be aligned to 1, without it, the value would be rounded to 0.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I locally run test_bug1151667.html and dumped the scrollTop, it was actually 1.

Comment 7

[Botond Ballo [:botond]]

(In reply to Glenn Watson [:gw] from comment #4)

Alternatively I could land my patch with the pref disabled, and change the test expectations, and the pref could be enabled at a later time when APZ team can investigate those perma-fails?

I would suggest going with this approach.

I assume by "change test expectations" you mean adding pref(layout.scroll.disable-pixel-alignment,true) to the snapped-scrolled-content-1.html test cases, to validate going forward that they are passing in this configuration even though this is not yet the default configuration.

Comment 8

[Glenn Watson [:gw]]

(In reply to Botond Ballo [:botond] from comment #7)

(In reply to Glenn Watson [:gw] from comment #4)

Alternatively I could land my patch with the pref disabled, and change the test expectations, and the pref could be enabled at a later time when APZ team can investigate those perma-fails?

I would suggest going with this approach.

I assume by "change test expectations" you mean adding pref(layout.scroll.disable-pixel-alignment,true) to the snapped-scrolled-content-1.html test cases, to validate going forward that they are passing in this configuration even though this is not yet the default configuration.

Yes, that's right. I've updated the patch if you can review the test changes before I land. To clarify:

• Added pref(layout.scroll.disable-pixel-alignment,true) to all of that block of tests.
• Added fails-if(useDrawSnapshot) to 3 tests - these tests fail in drawSnapshot mode with the preference enabled and without the WR change present.
• Added fails-if(useDrawSnapshot) to 2 tests - these tests were unconditional FAILs, but now pass in WR mode with the patch (continue to fail in drawSnapshot mode).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c6bf9d53ca1
Apply scroll snapping without external offsets during scene building r=gfx-reviewers,botond,lsalzman

Comment 10

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/6c6bf9d53ca1

Comment 11

[Botond Ballo [:botond]]

Thanks Glenn! I filed two follow-ups:

• bug 1946610 to track enabling layout.scroll.disable-pixel-alignment by default
• bug 1946611 to track getting the tests to pass in useDrawSnapshot mode