1281907 - Scroll bars disappear when zoomed in/out (content resolution !≈ 1)

Status: VERIFIED FIXED

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

Description

[Randall Barker [:rbarker]]

1) Visit planet.mozilla.org
2) Pinch zoom out as far as possible.

Expected behavior: Vertical scroll bar is visible.
Actual behavior: Vertical scroll bar is not visible.

This reproduces on both Nexus 4 and Galaxy S4.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Marking as P2 because in practice we've found that nobody really cares about scrollbars. At least, nobody complains about them being missing on mobile. Also, the scrollbars only disappear when zoomed out; they are visible when the page first loads or zoomed in.

Comment 2

[Jan Henning [:JanH]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> Marking as P2 because in practice we've found that nobody really cares about
> scrollbars. At least, nobody complains about them being missing on mobile.

I hereby complain :-), as did two people in bug 1426846.

> Also, the scrollbars only disappear when zoomed out; they are visible when
> the page first loads or zoomed in.

As far as I can see, they are also missing when zooming out compared to the initial document resolution (where possible, e.g. on http://www.456bereastreet.com/lab/html5-input-types/), generally on pages without a mobile-friendly meta viewport setting (both bug 1433701), or possibly also if the page is too long (bug 1426846).

Also, what component should this be moved into now that the former Android graphics component has morphed into being about the dynamic toolbar only?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

The APZ component is probably best for now. However right now I'm not sure if anybody has cycles to work on this. Randall or somebody else on snorp's team might be best if this is something that you would like to see fixed soon.

Comment 5

[Botond Ballo [:botond]]

I've also noticed this, and it has annoyed me, mostly because without a scrollbar you don't have a way of getting a sense for "how far down the page" you are.

Comment 6

[Jan Henning [:JanH]]

@Botond: In case you've got any thoughts on that, it has turned out that my proposed fix is basically reverting bug 1165536.
I.e. as far as I can tell empirically (I'm not at all familiar with this code), the resolution compensation keeps the scrollbars at the same position of the screen regardless of the resolution, but the clip rects are scaled by the pinch zoom together with the content itself and therefore we get a mismatch between the position of the scrollbars and their clip rects unless the resolution is ≈ 1.0.

Comment 7

[Jan Henning [:JanH]]

Attachment: Bug 1281907 - Part 1 - Include zoom level compensation in clip transform passed to caller.

If we don't do that, in the case of a root scrollbar the clip transform passed
to our caller will incorrectly be scaled with the current content resolution.
This means that that the position of the clip rect won't match the actual
position of the scrollbar if the resolution isn't ~ 1.0, so the scrollbars will
be clipped out of existence when the content is zoomed in or out.

Review commit: https://reviewboard.mozilla.org/r/231000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231000/

Comment 8

[Botond Ballo [:botond]]

Thanks for working on this, Jan!

I tried to dig a bit to understand why we chose to exclude that compensation in bug 1165536; I looked at bug 1164767 comment 3, including the "long IRC discussion" mentioned in that comment (which starts here [1]), but the reason seems to have to do with how Layout positions scrollbars in the presence of a resolution, which I'm having a hard time reasoning about.

I'm inclined to say that if this change fixes the problem, and doesn't regress any tests, we should go ahead and land it.

However, it would be good to add a new test for this.

We have a couple of existing reftests [2] [3] that test scrollbar rendering during zooming. I believe the reason they're not catching this bug is that this bug has to do with the pres shell resolution, which is the resolution at which the main thread re-renders content after zooming, but the tests only use an async zoom (a zoom added by the compositor without re-rendering the content).

To trigger a pres shell resolution, we can use a meta viewport tag with an initial-scale set to our desired zoom. (We then don't need to use the reftest-async-zoom attribute, though we still can if we want.)

Would you be willing to try writing a test like that?

[1] https://mozilla.logbot.info/apz/20150515#c8365
[2] https://searchfox.org/mozilla-central/source/gfx/layers/apz/test/reftest/async-scrollbar-zoom-1.html
[3] https://searchfox.org/mozilla-central/source/gfx/layers/apz/test/reftest/async-scrollbar-zoom-2.html

Comment 9

[Jan Henning [:JanH]]

(In reply to Botond Ballo [:botond] from comment #8)
> Would you be willing to try writing a test like that?

Provided I can get them to run properly on my phone, then sure, I'll give it a shot.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8962195 [details]
Bug 1281907 - Part 1 - Include zoom level compensation in clip transform passed to caller.

I was hoping to test this patch to verify that it fixes the problem, but I can't even successfully launch Fennec at the moment (it startup-crashes on me), so I'll just r+ this and trust that you've verified that it works.

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8962195 [details]
Bug 1281907 - Part 1 - Include zoom level compensation in clip transform passed to caller.

https://reviewboard.mozilla.org/r/231000/#review237652

(Whoops, I should've done that through MozReview.)

Comment 12

[Jan Henning [:JanH]]

Attachment: Bug 1281907 - Part 2 - Add reftest for scrollbars with pinch-zooming.

Review commit: https://reviewboard.mozilla.org/r/233014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233014/

Comment 13

[Jan Henning [:JanH]]

Comment on attachment 8962195 [details]
Bug 1281907 - Part 1 - Include zoom level compensation in clip transform passed to caller.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231000/diff/1-2/

Comment 14

[Jan Henning [:JanH]]

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

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8964378 [details]
Bug 1281907 - Part 2 - Add reftest for scrollbars with pinch-zooming.

https://reviewboard.mozilla.org/r/233014/#review238618

Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2e0f2e299bc
Part 1 - Include zoom level compensation in clip transform passed to caller. r=botond
https://hg.mozilla.org/integration/autoland/rev/5506727613da
Part 2 - Add reftest for scrollbars with pinch-zooming. r=botond

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a2e0f2e299bc
https://hg.mozilla.org/mozilla-central/rev/5506727613da

Comment 18

[Jan Henning [:JanH]]

Comment on attachment 8962195 [details]
Bug 1281907 - Part 1 - Include zoom level compensation in clip transform passed to caller.

Approval Request Comment
[Feature/Bug causing the regression]: Presumably APZ on Android (bug 1206872).
[User impact if declined]: Scrollbars on Android aren't shown if the document is displayed at a resolution other than 1.0, e.g. when pinch-zooming or commonly when viewing Desktop pages.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Tested locally.
[Needs manual test from QE? If yes, steps to reproduce]: Load any page, zoom in, zoom out and check that the scrollbars are always shown.
[List of other uplifts needed for the feature/fix]: New test from Part 2.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's just a small change to make sure that the scrollbar's clip rects use the same transformation as the scrollbars themselves when the document resolution changes to a value other than 1.0. Besides, today the scrollbars are already completely broken for document resolutions !≈ 1.0, so things can't get much worse anyway.
[String changes made/needed]: none

Comment 19

[Jan Henning [:JanH]]

Comment on attachment 8964378 [details]
Bug 1281907 - Part 2 - Add reftest for scrollbars with pinch-zooming.

See above.

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8962195 [details]
Bug 1281907 - Part 1 - Include zoom level compensation in clip transform passed to caller.

fix scrollbars on android, beta60+

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8964378 [details]
Bug 1281907 - Part 2 - Add reftest for scrollbars with pinch-zooming.

... and test.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This modified code added by bug 1443792 and bug 1449145. Both landed for Firefox 61 and haven't been uplifted to beta (60), that's why the patches fail to apply. Is the uplift request obsolete?

Comment 23

[Botond Ballo [:botond]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #22)
> This modified code added by bug 1443792 and bug 1449145. Both landed for
> Firefox 61 and haven't been uplifted to beta (60), that's why the patches
> fail to apply. Is the uplift request obsolete?

No, the affected code just moved around a bit, so the patch needs a rebase for beta.

Comment 24

[Jan Henning [:JanH]]

Bug 1449145 only added a "const" to a variable declaration in the immediate neighbourhood, but didn't change anything else [1] and bug 1443792 seems to have been just a straight copy-paste operation, at least as far as that particular function is concerned [2].
So applying the patch analogously to gfx/layers/composite/AsyncCompositionManager.cpp on Beta should work - if you want, I can prepare a rebased Beta patch, though.


[1] https://hg.mozilla.org/mozilla-central/rev/7db0e20d6a37#l1.53
[2] https://hg.mozilla.org/mozilla-central/rev/5a049955eaf3#l5.46

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d6cfd5638b85
https://hg.mozilla.org/releases/mozilla-beta/rev/8bf7d4cef4c3

Comment 26

[Ioana Chiorean]

Bogdan is working on the uplifts these days and will be able to help here.

Comment 27

[Bogdan Surd, QA [:bsurd, NI]]

Device:
 - Nexus 5 (Android 6.0.1);

Verified in the latest Nightly using the steps provided in Comment 0. 
Everything worked as expected for multiple sites that I visited. This fix does not seem to be in the latest Beta so I will be leaving my NI to re-check this with the next beta release.

Comment 28

[Bogdan Surd, QA [:bsurd, NI]]

Device:
 - Samsung Galaxy S8 (Android 8.0)

Issue is fixed in 60.0b12 as well. Marking as verified.