1636277 - Pinch-zooming in on BMO does not create vertical viewport scrollbar

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

STR

1. Run latest nightly with apz.allow_zooming=true
2. Load https://bugzilla.mozilla.org/show_bug.cgi?id=1556556
3. Observe that at unit zoom, there are no viewport scrollbars. This is because the document is sized to the layout viewport, and at unit zoom, the visual viewport coincides with the layout viewport. (The scrollbar which does appear is for a subframe.)
4. Pinch-zoom in, thereby causing the visual and layout viewports to diverge.
5. Observe that a horizontal viewport scrollbar appears.

Expected results

A vertical viewport scrollbar appears as well. (Since the visual and layout viewports are aspect-ratio locked, it follows that if the visual viewport becomes smaller than the layout viewport in the horizontal dimension, it must do so in the vertical dimension as well.)

Actual results

A vertical viewport scrollbar does not appear.

Comment 1

[Botond Ballo [:botond]]

This likely has to do with the root element on BMO being overflow-y: hidden.

Note that overflow: hidden does not suppress creation of viewport scrollbars when zoomed on mobile, but perhaps the relevant logic is currently mobile-specific, or specific to overlay scrollbars.

Comment 2

[Timothy Nikkel (:tnikkel)]

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

This likely has to do with the root element on BMO being overflow-y: hidden.

Note that overflow: hidden does not suppress creation of viewport scrollbars when zoomed on mobile, but perhaps the relevant logic is currently mobile-specific, or specific to overlay scrollbars.

Doesn't seem to be different for overlay vs not.

If I make these Auto when the style is overflow hidden and it's the root scroll frame things seem to work as we want.

https://searchfox.org/mozilla-central/rev/61fceb7c0729773f544a9656f474e36cd636e5ea/layout/generic/nsGfxScrollFrame.cpp#314

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1636277. Allow root scroll frames to show scrollbars even if they are overflow hidden because pinch zooming might require them. r?emilio

We can't pinch zoom all root scroll frames but applying this to all root scroll frames seem reasonable.

Comment 4

[Timothy Nikkel (:tnikkel)]

Doing some testing on macOS. Chrome never seems to add scrollbars after pinchzooming that weren't there before, and the behaviour is the same whether I have "Show scroll bars" in system settings set to "Automatically based on mouse or trackpad" or "Always".

Safari seems to have the best behaviour, even with show scrollbars is set to always, it will add scrollbars as expected and they get added after you lift your fingers from the pinch gesture.

Doing a quick test on Windows pinchzooming google.ca, Chrome is the same, never adds scrollbars after pinchzooming that weren't there before. Edge adds non-floating scrollbars while you are pinch zooming.

Comment 5

[Timothy Nikkel (:tnikkel)]

It looks like neither fennec nor fenix show this scrollbar.

Comment 6

[Timothy Nikkel (:tnikkel)]

Making the scrollbar show is not so hard. Once it's showing there are many bugs. The worst and seems like the best place to start is: the scroll thumb is offset, it looks the right size, and moves properly, but when scrolled all the way to the top it's halfway down the track, and when scrolled to the bottom it can be partway or completely off screen. I've verified that everything on the layout side seems correct, the frame tree looks correct, the display list looks correct, the layer tree looks correct. So there is something changing the scrollbars on the compositor side?

Comment 7

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #6)

So there is something changing the scrollbars on the compositor side?

APZ can apply a transform to scroll thumbs to account for async scrolling (WR call site, non-WR call site), it could be that.

Comment 8

[Timothy Nikkel (:tnikkel)]

This comment
https://searchfox.org/mozilla-central/rev/7dafc35406b9c945189c617d427f5458933fd3fb/gfx/layers/apz/src/APZCTreeManager.cpp#3746
// |asyncTransform| represents the amount by which we have scrolled and
// zoomed since the last paint. Because the scrollbar was sized and positioned
// based on the painted content, we need to adjust it based on asyncTransform
// so that it reflects what the user is actually seeing now.
probably explains it (I'll look into the code next to confirm). I've had to change the scrollbars so that they reflect the visual viewport offset and the visual viewport size, thus likely already taking into account some (all?) of the transforms that function does, and the asyncTransform is persistant in the overflow hidden case (the painted content doesn't "sync up" with the async transform ever).

Comment 9

[Timothy Nikkel (:tnikkel)]

Modified ComputeTransformForScrollThumb so that the thumbs usual draws correctly, but it quick frequently flashes in the wrong position. Not sure what is going on with that yet. There is another issue which is that we can get stuck scrolled down by a small but noticeable amount even when fully zoomed out. After some debugging this seems to be caused by bug 1611660, when we add/remove the scrollbars (the kind that takes space, which is what I happened to have left my system in when testing various combinations) we send new metrics with new composition bounds, new scrollable rect, and new layout viewport. We accept the first two but ignore the layout viewport update. Then some pinch zooming causes some scroll offset changes and that makes us call KeepLayoutViewportEnclosingVisualViewport and we don't hit the early return, and then the layout viewport gets moved, and we end up scrolling back on the main thread (and not just the visual viewport offset, the scroll frame position).

Fix one thing, either expose or create two problems.

Comment 11

[Will]

Attachment: VerticalScrollbarChanges.png

Is bugzilla the only website that has this issue?

And why do the scrollbars look different? Try this: start from default zoom (reflow zoom) and go to menu > zoom in (Ctrl++) a bunch and notice after a zooming in enough, the vertical scrollbar changes in appearance. The arrows at the top and bottom go from white to grey and the thumb becomes significantly brighter. Strange

Comment 12

[Botond Ballo [:botond]]

(In reply to Will from comment #11)

Is bugzilla the only website that has this issue?

I think it may affect any site where the page is not scrollable at unit zoom.

And why do the scrollbars look different? Try this: start from default zoom (reflow zoom) and go to menu > zoom in (Ctrl++) a bunch and notice after a zooming in enough, the vertical scrollbar changes in appearance. The arrows at the top and bottom go from white to grey and the thumb becomes significantly brighter. Strange

If this happens without performing pinch-zoom at all, it's definitely worth filing a separate bug about.

Comment 13

[Will]

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

(In reply to Will from comment #11)

Is bugzilla the only website that has this issue?

I think it may affect any site where the page is not scrollable at unit zoom.

And why do the scrollbars look different? Try this: start from default zoom (reflow zoom) and go to menu > zoom in (Ctrl++) a bunch and notice after a zooming in enough, the vertical scrollbar changes in appearance. The arrows at the top and bottom go from white to grey and the thumb becomes significantly brighter. Strange

If this happens without performing pinch-zoom at all, it's definitely worth filing a separate bug about.

Are you able to repro the vertical scrollbar changing in appearance after you zoom in a bunch via reflow zoom? No pinch zoom is applied at all

Comment 14

[Botond Ballo [:botond]]

I am, yes. Seems like a potential bug in the scrollbar-color implementation where, past a certain zoom level, we start ignoring the custom style.

Comment 15

[Timothy Nikkel (:tnikkel)]

(In reply to Will from comment #11)

Is bugzilla the only website that has this issue?

Any site that uses overflow: hidden would be affected. overflow: scroll and overflow: auto should show the scrollbar on pinch zoom even if there wasn't a scrollbar visible before pinch zoom (interacting with the scrollbar may not work properly, which is known and tracked by other bugs).

Comment 16

[Kris Taeleman (:ktaeleman)]

Verified fixed with tnikkel's scrollbar changes in 1657822