1158392 - Enable containerless scrolling on Fennec on the Nightly channel

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Timothy Nikkel (:tnikkel)]

This should be ready now. I'll make a build and do some manual testing before landing this.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Timothy Nikkel (:tnikkel)]

Looks like this isn't ready yet. Videos don't get clipped properly and fixed pos stuff doesn't seem to stuff to the top toolbar properly either in some cases.

Comment 3

[Timothy Nikkel (:tnikkel)]

Pages where I've noticed problems

http://flatlandwrapwpg.com/
http://www.oregonbikepacking.com/portfolio-posts/hardman-portfolio/
http://fox2now.com/2015/04/23/new-kinloch-mayor-blocked-by-police-from-entering-city-hall/
http://www.winnipegfreepress.com/local/Selinger-silent-on-builders---rush-job-claim-301302231.html?device=mobile
http://www.usatoday.com/story/tech/2015/04/25/nepal-india-earthquake/26356487/

Comment 4

[Botond Ballo [:botond]]

I'm going to commandeer this bug to track enabling containerless scrolling on Android on the Nightly channel, which is pretty close to its original purpose.

Comment 5

[Botond Ballo [:botond]]

Here's how this is looking on Try with patches from the dependent bugs applied and the pref flipped:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=9431424fa0bf31f7c5090695c5944f79a18a7c5f

A few Android tests are still failing, but it's looking relatively green.

Comment 6

[Botond Ballo [:botond]]

Latest Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=a772337ce50e73b9334c47ff783503be742483e0

The tests that are still failing are:

layout/reftests/webm-video/poster-6.html
layout/reftests/webm-video/poster-7.html
layout/reftests/webm-video/poster-13.html
gfx/layers/apz/test/mochitest/test_interrupted_reflow.html

Weirdly, I can't repro any of these failures locally in the x86 emulator.

Comment 7

[Botond Ballo [:botond]]

Attachment: DIsplay list dump for poster-ref-black140x100.html

Comment 8

[Botond Ballo [:botond]]

Attachment: Display list dump for poster-6.html

Comment 9

[Botond Ballo [:botond]]

Attachment: Display list dump for poster-6.html on a successful (local) run

Comment 10

[Botond Ballo [:botond]]

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

Latest Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=a772337ce50e73b9334c47ff783503be742483e0

The tests that are still failing are:

[...]
gfx/layers/apz/test/mochitest/test_interrupted_reflow.html

This seems to be caused by the fact that we're not always layerizing the root content APZC. Possibly we have a mechanism for doing that that was used by container scrolling that now needs to carried over to containerless.

layout/reftests/webm-video/poster-6.html
layout/reftests/webm-video/poster-7.html
layout/reftests/webm-video/poster-13.html

These failures continue to mystify both me and Markus. The display list dumps posted in comments 7-9 were in an attempt to investigate them, but they don't seem to match the incorrect rendering we're seeing in the reftest analyzer.

Comment 11

[Botond Ballo [:botond]]

Attachment: Display list dump for poster-6.html

To be more specific, here is a display list dump from a failing run on Try.

It was taken using a test-pref(layout.display-list.dump,true) annotation on just one test (poster-6.html), so we can be more confident it's the right dump. Of the several dumps in the logcat, it's the last one, which is presumably the one that was captured by the reftest, but for good measure I did look at the other ones as well: they're either the same, or they still have the reftest-wait attribute (at which point the rendering is expected to be different).

The Try push it comes from is https://treeherder.mozilla.org/#/jobs?repo=try&revision=10ea755d147bcc9f73ac7a042fc0bb4cd5eb7eee.

Here's why I'm saying it doesn't match the rendering, which is a fullscreen (800x1000) black rectangle:

• The only content layers that are visible are 0x69fa7c00, 0x70b7b000, 0x70b79400, and 0x66511400.
• The first two are white color layers.
• The third is a painted layer whose only contributing display item is a CanvasBackgroundColor item, which is again white.
• The last is the video layer, and its visible region is 140x100, as expected.

Comment 12

[Botond Ballo [:botond]]

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

gfx/layers/apz/test/mochitest/test_interrupted_reflow.html

This seems to be caused by the fact that we're not always layerizing the root content APZC. Possibly we have a mechanism for doing that that was used by container scrolling that now needs to carried over to containerless.

Found the bug here. It looks like the RCD-RSF scroll frame object can be reused across multiple tests in a directory. In the directory gfx/layers/apz/test/mochitest, there is still one test (test_group_minimum_scale_size.html) that uses container scrolling (which is just an oversight). Due to the way we set mIsScrollableLayerInRootContainer, such that the flag can only go from false to true over the course of a scroll frame object's lifetime, and never from true to false, the object got "stuck" with a value of true even during later tests than ran with container scrolling disabled. The flag then inhibited layerization of the RCD-RSF via the containerless codepath.

Comment 13

[Botond Ballo [:botond]]

(That also explains why I couldn't reproduce the failure locally: I was running just the one test locally, but the failure mode requires running the entire directory at once. Why do I always forget to try that?)

Comment 14

[Botond Ballo [:botond]]

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

That also explains why I couldn't reproduce the failure locally: I was running just the one test locally, but the failure mode requires running the entire directory at once.

Unfortunately, the same isn't true of the webm-video reftests: I can't reproduce those failures locally even if I run the entire directory.

Comment 15

[Botond Ballo [:botond]]

In a newer Try push, a bunch of layout/reftests/bugs/1133905-*.html tests are failing too. That was likely introduced by bug 1520077 turning on apz.allow_zooming for those tests.

Comment 16

[Botond Ballo [:botond]]

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

In a newer Try push, a bunch of layout/reftests/bugs/1133905-*.html tests are failing too. That was likely introduced by bug 1520077 turning on apz.allow_zooming for those tests.

And, now that bug 1516377 has landed, layout/reftests/meta-viewport/position-fixed-out-of-view.html has started to fail as well...

Comment 17

[Botond Ballo [:botond]]

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

In a newer Try push, a bunch of layout/reftests/bugs/1133905-*.html tests are failing too. That was likely introduced by bug 1520077 turning on apz.allow_zooming for those tests.

I believe these tests have been invalidated by the changes to viewport sizing rules that we've been making. With the new rules in place, the tests and reference pages are no longer expected to render the same.

The tests currently "pass" because in automation with container scrolling they just render as blank. I'm not quite sure why, but I suspect that's a bug with container scrolling. I'm not to bother investigating and fixing that bug, given that container scrolling is going away.

I'm going to try and rework the tests so that they exercise the original intent, while passing with containerless scrolling.

Comment 18

[Botond Ballo [:botond]]

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

I'm going to try and rework the tests so that they exercise the original intent, while passing with containerless scrolling.

I have the LTR tests working locally. The RTL tests are failing at zoom levels greater than 1, and I haven't figured out why. I'm going to see if I can get them to run on desktop and debug them with rr.

Comment 19

[Botond Ballo [:botond]]

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

The RTL tests are failing at zoom levels greater than 1, and I haven't figured out why. I'm going to see if I can get them to run on desktop and debug them with rr.

Unfortunately, overlay scrollbars are much too broken on Linux to get the behaviour to even resemble Android.

Comment 20

[Botond Ballo [:botond]]

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

And, now that bug 1516377 has landed, layout/reftests/meta-viewport/position-fixed-out-of-view.html has started to fail as well...

There appears to be an underlying issue on this page that's unrelated to containerless scrolling. The layout viewport size is 1600x1000 while the visual viewport size is 800x1120. As neither is larger than the other in both dimensions, the logic to keep the layout viewport enclosing the visual viewport does not trigger, and things go wrong. It so happens that the way in which things go wrong manifests in the test failing with containerless scrolling, but the fix will need to be for the underlying issue.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

That's totally my fault. I was wondering whether we should check vertical direction overflow and horizontal direction overflow respectively in bug 1516377, but I couldn't come up with a test case at that time.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

(I should have written a simple test case rather than a reftest).

Comment 23

[Botond Ballo [:botond]]

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

There appears to be an underlying issue on this page that's unrelated to containerless scrolling. The layout viewport size is 1600x1000 while the visual viewport size is 800x1120.

This seems to be related to the Fennec dynamic toolbar; the 1120 is the widget height and the 1000 the content viewer height. Presumably the former includes the toolbar height and the latter does not. I filed bug 1527187 for this, and tweaked the testcase to work around it for now.

Comment 24

[Botond Ballo [:botond]]

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

The RTL tests are failing at zoom levels greater than 1

This is at least in part due to changes to layout viewport sizing made in bug 1423013. I'm going to disable the tests in this bug, and fix the issue in bug 1527511 (and re-enable the tests) before letting containerless scrolling ride the trains.

Comment 25

[Botond Ballo [:botond]]

Let's do GeckoView in another bug to facilitate granular backouts if necessary.

Comment 26

[Botond Ballo [:botond]]

Attachment: Bug 1158392 - Mark some more webm-video reftests as failing or random on Android. r=kats

Manual testing shows expected rendering of the pages in question. The tests
do not reproduce locally and are tricky to debug on Try. Moreover, most
other reftests in this directory are already marked as failing or random
on Android.

For these reasons, I don't want these failures to block the rollout of
containerless scrolling. Bug 1084564 tracks re-enabling them.

Comment 27

[Botond Ballo [:botond]]

Attachment: Bug 1158392 - Fix 1133905-*-vh-*.html reftests so they are valid in the presence of new viewport sizing rules. r=kats

Depends on D19679

Comment 28

[Botond Ballo [:botond]]

Attachment: Bug 1158392 - Disable 1133905-*-vh-rtl reftests with zoom levels > 1. r=kats

These were rendering as blank with container scrolling, and their failure with
containerless scrolling is caused by an unrelated viewport bug, tracked in
bug 1527511.

Depends on D19680

Comment 29

[Botond Ballo [:botond]]

Attachment: Bug 1158392 - Make sure the 113905-*-vh-*.html tests do not render as blank. r=kats

Depends on D19681

Comment 30

[Botond Ballo [:botond]]

Attachment: Bug 1158392 - Enable containerless scrolling on the nightly channel in Fennec. r=mstange

Depends on D19682

Comment 31

[Botond Ballo [:botond]]

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

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

I'm going to try and rework the tests so that they exercise the original intent, while passing with containerless scrolling.

I have the LTR tests working locally. The RTL tests are failing at zoom levels greater than 1,

It appears that some of the RTL tests are also failing on desktop now.

Comment 32

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [standards meeting Feb 18-23] from comment #31)

It appears that some of the RTL tests are also failing on desktop now.

Doh. These tests were disabled on desktop altogether with skip-if(!Android). I then added a fuzzy(...) which, you guessed it, overrides the skip-if... (it needs to be fuzzy-if(Android, ...) instead).

Comment 33

[Botond Ballo [:botond]]

I've re-pushed this to Try, and it looks good:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=76575a2bd952e18ad51112bb6386537f83bf0f6b

It's ready to land as soon as bug 1528743 is fixed.

Comment 34

[Hiroyuki Ikezoe (:hiro)]

\o/

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e68d1422ad6
Mark some more webm-video reftests as failing or random on Android. r=kats
https://hg.mozilla.org/integration/autoland/rev/c3c039fd4476
Fix 1133905-*-vh-*.html reftests so they are valid in the presence of new viewport sizing rules. r=kats
https://hg.mozilla.org/integration/autoland/rev/add02eb73b9c
Disable 1133905-*-vh-rtl reftests with zoom levels > 1. r=kats
https://hg.mozilla.org/integration/autoland/rev/c751a6dc7229
Make sure the 113905-*-vh-*.html tests do not render as blank. r=kats
https://hg.mozilla.org/integration/autoland/rev/7ea113ba9d69
Enable containerless scrolling on the nightly channel in Fennec. r=mstange

Comment 36

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/8e68d1422ad6
https://hg.mozilla.org/mozilla-central/rev/c3c039fd4476
https://hg.mozilla.org/mozilla-central/rev/add02eb73b9c
https://hg.mozilla.org/mozilla-central/rev/c751a6dc7229
https://hg.mozilla.org/mozilla-central/rev/7ea113ba9d69

Comment 37

[Hiroyuki Ikezoe (:hiro)]

\o/\o/\o/