1656802 - handle overflow hidden scrollbars and related with pinch zooming

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, enhancement)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. When deciding if we want a scrollbar we need to consider only if the scrolled rect overflows the scrollport (not the visual viewport). r?emilio,kats

This will actually regress behaviour when overflow is auto and pinch zooming creates scrollable overflow (scrolling the visual viewport inside the layout viewport). We will fix that in later patches.

The reason that this is necessary is that the code as-is is incorrect if we have layout scrollbars (scrollbars that take up space). If we have layout scrollbars and we pinch zoom and we go from not needing a scrollbar to needing a scrollbar that scrollbar cannot take up layout space (even though it is a layout scrollbar). The scrollbar cannot change the size of the layout viewport (it does, however change the size of the visual viewport).

In later patches we fix this situation as well as the situation with an overflow hidden document (which also needs to create scrollbars when pinch zoomed).

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Add flags that let us differentiate not showing scrollbars because something is overflow hidden from not showing scrollbars for other reasons. r?emilio

For the former we are still allowed to show scrollbars if we need to scroll the visual viewport inside the layout viewport (as long as they take up no layout space). For the latter we still do not want to show scrollbars.

The ShowScrollbar enum is now only from layouts perspective and doesn't take into account anything about the visual viewport.

Depends on D85699

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Calculate if we need scrollbars to scroll the visual viewport. r?emilio,kats

This fixes the regression we created with the first patch.

Depends on D85700

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Add state variables to the scroll frame to track when scrollbars are only created to scroll the visual viewport within the layout viewport. r?emilio,kats

We need to distinguish these special scrollbars for several different reasons in upcoming patches.

Depends on D85701

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Add a comment explaining how overlay scrollbars work with their negative margin. r?emilio

Depends on D85702

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Shift back in layout scrollbars that are only for the visual viewport so they are visible. r?emilio

Layout scrollbars that were only created for the visual viewport had no space reserved for them, so we need to shift them back in like overlay scrollbars. Otherwise they sit just outside of the scroll port and are not visible.

Depends on D85703

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. In LayoutScrollbars move the calculation of the scrollbar rects to the first thing in the function. r?emilio

There is no dependency in this code as far as I can tell.

The next patch needs the scrollbar rects for the scroll corner calculation.

Depends on D85704

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Position the scrollcorner if we have both layout scrollbars and they are both for the visual viewport only. r?emilio

Otherwise the calculation above would lead to an empty rect for the scroll corner.

The scrollbar rects as computed now still overlap each other and the scroll corner, the next patch fixes that.

Depends on D85705

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. If both scrollbars are layout and present and only for the visual viewport they need to be adjusted to not overlap. r?emilio

The existing calculation will make them overlap. The AdjustOverlappingScrollbars code was written with overlay scrollbars in mind but it looks like it will work just fine for this case.

Depends on D85706

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Make visual viewport only layout scrollbars affect the composition bounds/visual viewport. r?emilio,kats

AFAICT the spec says that these layout scrollbars that take up no layout space that scroll the visual viewport do affect the size of the visual viewport.

Most other users don't care about the size of these special scrollbars.

I left nsIDOMWindowUtils::getScrollbarSize unchanged (NB different from nsIDOMWindowUtils::getScrollbarSizes which is modified by this patch) because I'm less sure. I will file a followup about it.

Depends on D85707

Comment 11

[Timothy Nikkel (:tnikkel)]

In addition to the patches I did as best a job as I could auditing any places that might need to be changed for these special "layout scrollbars that don't take any layout space". Bug 1656805 is the only thing I found that I didn't fix in these patches.

Comment 12

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

Can you describe a bit more what the overall goal of these patches is? Like specific use cases that it's meant to handle?

Comment 13

[Timothy Nikkel (:tnikkel)]

The main problem it sets out to solve is that when the user pinch zooms on a document with overflow: hidden we want to show scrollbars. This brings up the issue of if layout scrollbars are enabled we don't want the addition of these scrollbars to change the layout viewport. So we make them not take up any layout space: they become a special type of layout scrollbar that takes up no space. But it seems like we do want this special type of scrollbar to reduce the composition size/visual viewport size (because they completely cover part of the screen). And finally, for overflow: auto where there is no scrollbar visible until the user pinch zooms: the scrollbars should correctly already show up, but they will take up space. These patches aim to fix that (the fix is the same as the overflow hidden case, it's not a separate thing in the code).

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Is there any chance to get these code-paths tested? This looks fine to me but it's a bit unfortunate that there'll be no test-coverage for this rather non-trivial code.

Comment 15

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

I think it should be possible to write reftests that have an overflow:hidden page but also a zoom applied, and verify that scrollbars appear.

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Shift back in layout scrollbars that are only for the visual viewport so they are visible. r?emilio

Layout scrollbars that were only created for the visual viewport had no space reserved for them, so we need to shift them back in like overlay scrollbars. Otherwise they sit just outside of the scroll port and are not visible.

Depends on D85703

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Shift back in layout scrollbars that are only for the visual viewport so they are visible. r?emilio

Layout scrollbars that were only created for the visual viewport had no space reserved for them, so we need to shift them back in like overlay scrollbars. Otherwise they sit just outside of the scroll port and are not visible.

Depends on D85703

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Add comment pointing to bug about enabling new desktop zooming scrollbars by default.

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Shift back in layout scrollbars that are only for the visual viewport so they are visible. r?emilio

Layout scrollbars that were only created for the visual viewport had no space reserved for them, so we need to shift them back in like overlay scrollbars. Otherwise they sit just outside of the scroll port and are not visible.

Depends on D85703

Comment 20

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1656802. Shift back in layout scrollbars that are only for the visual viewport so they are visible. r?emilio

Layout scrollbars that were only created for the visual viewport had no space reserved for them, so we need to shift them back in like overlay scrollbars. Otherwise they sit just outside of the scroll port and are not visible.

Depends on D85703

Comment 21

[Timothy Nikkel (:tnikkel)]

I'll write a few tests for this next.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1517bc33eff
Add comment pointing to bug about enabling new desktop zooming scrollbars by default.
https://hg.mozilla.org/integration/autoland/rev/57fa48542435
When deciding if we want a scrollbar we need to consider only if the scrolled rect overflows the scrollport (not the visual viewport). r=emilio,kats
https://hg.mozilla.org/integration/autoland/rev/06a47c35c531
Add flags that let us differentiate not showing scrollbars because something is overflow hidden from not showing scrollbars for other reasons. r=emilio
https://hg.mozilla.org/integration/autoland/rev/2d3576ace2f1
Calculate if we need scrollbars to scroll the visual viewport. r=emilio,kats
https://hg.mozilla.org/integration/autoland/rev/386554110008
Add state variables to the scroll frame to track when scrollbars are only created to scroll the visual viewport within the layout viewport. r=emilio,kats
https://hg.mozilla.org/integration/autoland/rev/753f93dffe07
Add a comment explaining how overlay scrollbars work with their negative margin. r=emilio
https://hg.mozilla.org/integration/autoland/rev/21f574f6acb4
Shift back in layout scrollbars that are only for the visual viewport so they are visible. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8e217fc1c6d4
In LayoutScrollbars move the calculation of the scrollbar rects to the first thing in the function. r=emilio
https://hg.mozilla.org/integration/autoland/rev/fe2dd5de83ef
Position the scrollcorner if we have both layout scrollbars and they are both for the visual viewport only. r=emilio
https://hg.mozilla.org/integration/autoland/rev/c5b1b38d1d35
If both scrollbars are layout and present and only for the visual viewport they need to be adjusted to not overlap. r=emilio
https://hg.mozilla.org/integration/autoland/rev/941ee8aa9735
Make visual viewport only layout scrollbars affect the composition bounds/visual viewport. r=emilio,kats

Comment 23

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 11 changesets (bug 1656802) for async-scrolling reftests failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Jsva33dORZOnA_zjC5Q-dg.1&searchStr=geckoview-reftest-e10s&fromchange=8054d309c9d32f483fc967f10f0cf2ada964a923&tochange=8d315c1af284a3d51cdc65852b2330e11d60538d

Backout link: https://hg.mozilla.org/integration/autoland/rev/8d315c1af284a3d51cdc65852b2330e11d60538d

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312369793&repo=autoland&lineNumber=13449

[task 2020-08-07T11:06:36.093Z] 11:06:36     INFO -  REFTEST TEST-START | layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-1.html == layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-1-ref.html
[task 2020-08-07T11:06:36.093Z] 11:06:36     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-1.html | 2449 / 7231 (33%)
[task 2020-08-07T11:06:36.094Z] 11:06:36     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-1-ref.html | 2449 / 7231 (33%)
[task 2020-08-07T11:06:36.094Z] 11:06:36     INFO -  REFTEST INFO | REFTEST fuzzy test (2, 4) <= (0, 0) <= (2, 4)
[task 2020-08-07T11:06:36.094Z] 11:06:36  WARNING -  REFTEST TEST-UNEXPECTED-PASS | layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-1.html == layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-1-ref.html | image comparison, max difference: 0, number of differing pixels: 0
[task 2020-08-07T11:06:36.111Z] 11:06:36     INFO -  REFTEST   IMAGE 1 (TEST): data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAA...
[task 2020-08-07T11:06:36.121Z] 11:06:36     INFO -  REFTEST   IMAGE 2 (REFERENCE): data:image/png;base64,iVBORw0KGgoA...
[task 2020-08-07T11:06:36.121Z] 11:06:36     INFO -  REFTEST TEST-END | layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-1.html == layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-1-ref.html
[task 2020-08-07T11:06:36.121Z] 11:06:36     INFO -  REFTEST TEST-START | layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-2.html == layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-2-ref.html
[task 2020-08-07T11:06:36.121Z] 11:06:36     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-2.html | 2450 / 7231 (33%)
[task 2020-08-07T11:06:36.121Z] 11:06:36     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-2-ref.html | 2450 / 7231 (33%)
[task 2020-08-07T11:06:36.121Z] 11:06:36     INFO -  REFTEST INFO | REFTEST fuzzy test (3, 4) <= (0, 0) <= (3, 4)
[task 2020-08-07T11:06:36.122Z] 11:06:36  WARNING -  REFTEST TEST-UNEXPECTED-PASS | layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-2.html == layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-2-ref.html | image comparison, max difference: 0, number of differing pixels: 0
[task 2020-08-07T11:06:36.129Z] 11:06:36     INFO -  REFTEST   IMAGE 1 (TEST): data:image/png;base64,iVBORw0KGgoAAAANSUhE...
[task 2020-08-07T11:06:36.136Z] 11:06:36     INFO -  REFTEST   IMAGE 2 (REFERENCE): data:image/png;base64,iVBORw0KGgoAAAANSUh...
[task 2020-08-07T11:06:36.136Z] 11:06:36     INFO -  REFTEST TEST-END | layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-2.html == layout/reftests/async-scrolling/position-sticky-transformed-in-scrollframe-2-ref.html
[task 2020-08-07T11:06:36.136Z] 11:06:36     INFO -  REFTEST TEST-START | layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-1.html == layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-ref.html
[task 2020-08-07T11:06:36.136Z] 11:06:36     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-1.html | 2451 / 7231 (33%)
[task 2020-08-07T11:06:36.136Z] 11:06:36     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-ref.html | 2451 / 7231 (33%)
[task 2020-08-07T11:06:36.136Z] 11:06:36     INFO -  REFTEST INFO | REFTEST fuzzy test (3, 4) <= (0, 0) <= (3, 4)
[task 2020-08-07T11:06:36.137Z] 11:06:36  WARNING -  REFTEST TEST-UNEXPECTED-PASS | layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-1.html == layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-ref.html | image comparison, max difference: 0, number of differing pixels: 0
[task 2020-08-07T11:06:36.144Z] 11:06:36     INFO -  REFTEST   IMAGE 1 (TEST): data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAyAAAAPoCAY...
[task 2020-08-07T11:06:36.151Z] 11:06:36     INFO -  REFTEST   IMAGE 2 (REFERENCE): data:image/png;base64,iVBORw0KGgoAAAANSUhEUg....
[task 2020-08-07T11:06:36.151Z] 11:06:36     INFO -  REFTEST TEST-END | layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-1.html == layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-ref.html
[task 2020-08-07T11:06:36.151Z] 11:06:36     INFO -  REFTEST TEST-START | layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-2.html == layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-ref.html
[task 2020-08-07T11:06:36.152Z] 11:06:36     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-2.html | 2452 / 7231 (33%)
[task 2020-08-07T11:06:36.152Z] 11:06:36     INFO -  REFTEST INFO | REFTEST fuzzy test (3, 4) <= (0, 0) <= (3, 4)
[task 2020-08-07T11:06:36.152Z] 11:06:36  WARNING -  REFTEST TEST-UNEXPECTED-PASS | layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-2.html == layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-ref.html | image comparison, max difference: 0, number of differing pixels: 0
[task 2020-08-07T11:06:36.158Z] 11:06:36     INFO -  REFTEST   IMAGE 1 (TEST): data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAyAAAAPoCAYAAAAmy5q...
[task 2020-08-07T11:06:36.165Z] 11:06:36     INFO -  REFTEST   IMAGE 2 (REFERENCE): data:image/png;base64,iVBORw0KGgoA...
[task 2020-08-07T11:06:36.165Z] 11:06:36     INFO -  REFTEST TEST-END | layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-2.html == layout/reftests/async-scrolling/position-sticky-in-transformed-scrollframe-ref.html

Comment 24

[Timothy Nikkel (:tnikkel)]

The android failures are due to the change in the "shift scrollbars back in" patch. Turns out that on android the pref size of scrollbars is 0 (and the min size is positive!). I'm going to go back to my original patch for that part.

Comment 25

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13e2f6649566fc6ee66dd4e5adb45cdc820e310e

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8bff4f89905
Add comment pointing to bug about enabling new desktop zooming scrollbars by default.
https://hg.mozilla.org/integration/autoland/rev/169cb537921e
When deciding if we want a scrollbar we need to consider only if the scrolled rect overflows the scrollport (not the visual viewport). r=emilio,kats
https://hg.mozilla.org/integration/autoland/rev/e6f3c23488da
Add flags that let us differentiate not showing scrollbars because something is overflow hidden from not showing scrollbars for other reasons. r=emilio
https://hg.mozilla.org/integration/autoland/rev/72f6b67a2b9b
Calculate if we need scrollbars to scroll the visual viewport. r=emilio,kats
https://hg.mozilla.org/integration/autoland/rev/0305c832b34c
Add state variables to the scroll frame to track when scrollbars are only created to scroll the visual viewport within the layout viewport. r=emilio,kats
https://hg.mozilla.org/integration/autoland/rev/7df8ec36a7b0
Add a comment explaining how overlay scrollbars work with their negative margin. r=emilio
https://hg.mozilla.org/integration/autoland/rev/2508c5b144b3
Shift back in layout scrollbars that are only for the visual viewport so they are visible. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7e10936d9dfe
In LayoutScrollbars move the calculation of the scrollbar rects to the first thing in the function. r=emilio
https://hg.mozilla.org/integration/autoland/rev/061f1d8a6f6f
Position the scrollcorner if we have both layout scrollbars and they are both for the visual viewport only. r=emilio
https://hg.mozilla.org/integration/autoland/rev/3807b793a250
If both scrollbars are layout and present and only for the visual viewport they need to be adjusted to not overlap. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7b93758ae83e
Make visual viewport only layout scrollbars affect the composition bounds/visual viewport. r=emilio,kats

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)

I think it should be possible to write reftests that have an overflow:hidden page but also a zoom applied, and verify that scrollbars appear.

reftest only has a mechanism for async zoom AFAIK (and we need the main thread to update to get the scrollbars), so I think mochitest is the way this will need to be tested.

Comment 28

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a8bff4f89905
https://hg.mozilla.org/mozilla-central/rev/169cb537921e
https://hg.mozilla.org/mozilla-central/rev/e6f3c23488da
https://hg.mozilla.org/mozilla-central/rev/72f6b67a2b9b
https://hg.mozilla.org/mozilla-central/rev/0305c832b34c
https://hg.mozilla.org/mozilla-central/rev/7df8ec36a7b0
https://hg.mozilla.org/mozilla-central/rev/2508c5b144b3
https://hg.mozilla.org/mozilla-central/rev/7e10936d9dfe
https://hg.mozilla.org/mozilla-central/rev/061f1d8a6f6f
https://hg.mozilla.org/mozilla-central/rev/3807b793a250
https://hg.mozilla.org/mozilla-central/rev/7b93758ae83e