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

VERIFIED FIXED in Firefox 60

Status

()

defect
P3
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: rbarker, Assigned: JanH)

Tracking

48 Branch
mozilla61
All
Android
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 verified, firefox61 verified)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments)

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.
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.
OS: Unspecified → Android
Priority: -- → P2
Hardware: Unspecified → All
Version: unspecified → 48 Branch
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
Flags: needinfo?(bugmail)
See Also: → 1426846, 1433701
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.
Component: Toolbar → Panning and Zooming
Flags: needinfo?(bugmail)
Product: Firefox for Android → Core
Whiteboard: [gfx-noted]
See Also: 1433701
Duplicate of this bug: 1433701
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.
Assignee: nobody → jh+bugzilla
Summary: Scroll bars disappear when zoomed out. → Scroll bars disappear when zoomed in/out (content resolution !≈ 1)
@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.
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
(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 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.
Attachment #8962195 - Flags: review?(botond) → review+
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 on attachment 8964378 [details]
Bug 1281907 - Part 2 - Add reftest for scrollbars with pinch-zooming.

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

Thanks!
Attachment #8964378 - Flags: review?(botond) → review+
Pushed by mozilla@buttercookie.de:
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
https://hg.mozilla.org/mozilla-central/rev/a2e0f2e299bc
https://hg.mozilla.org/mozilla-central/rev/5506727613da
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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
Attachment #8962195 - Flags: approval-mozilla-beta?
Comment on attachment 8964378 [details]
Bug 1281907 - Part 2 - Add reftest for scrollbars with pinch-zooming.

See above.
Attachment #8964378 - Flags: approval-mozilla-beta?
Comment on attachment 8962195 [details]
Bug 1281907 - Part 1 - Include zoom level compensation in clip transform passed to caller.

fix scrollbars on android, beta60+
Attachment #8962195 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8964378 [details]
Bug 1281907 - Part 2 - Add reftest for scrollbars with pinch-zooming.

... and test.
Attachment #8964378 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite+
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?
Flags: needinfo?(jh+bugzilla)
(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.
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
Flags: needinfo?(jh+bugzilla)
Flags: qe-verify+
Bogdan is working on the uplifts these days and will be able to help here.
Flags: needinfo?(bogdan.surd)
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.
Device:
 - Samsung Galaxy S8 (Android 8.0)

Issue is fixed in 60.0b12 as well. Marking as verified.
Status: RESOLVED → VERIFIED
Flags: needinfo?(bogdan.surd)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.