1423013 - Content that overflows the ICB on an overflow:hidden page is "out of reach"

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P2)

Description

[Botond Ballo [:botond]]

Many websites seem to do the following on mobile:

  - Have a meta viewport tag with width=device-width,
    causing the width of the initial containing block
    (ICB) to be narrow.

  - Have content that's wider than the ICB width.

  - Have overflow-x:hidden on the html or body element.

  - Have either no minimum scale, or a minimum scale
    that's small enough to fit the entire content width
    on screen.

Such sites have the following behaviour in mobile browsers:

  - In Chrome, the content that overflows the ICB is
    reachable via scrolling (possibly already visible on
    page load, depending on the page's initial-scale),
    because Chrome sizes the scrollport of the RCD-RSF
    to be wider than the ICB.

  - In Safari, the content that overflows the ICB is
    reachable via scrolling because Safarii seems to
    ignore overflow:hidden on the RCD-RSF.

  - In Firefox, the content that overflows the ICB is
    out of reach, because we size the scrollport of the
    RCD-RSF to the ICB, and respect overflow:hidden.

We should either unbreak these sites (presumably by doing either what Chrome does, or what Safari does), or agree with Chrome and Safari to all break them.

Comment 1

[Botond Ballo [:botond]]

Moving relevant webcompat issues over from bug 1123938.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

It seems both approach involves overriding overflow-x:hidden, which should be quite straightforward in our code by just adding a special case in nsPresContext::UpdateViewportScrollbarStylesOverride. But yeah, it isn't totally clear whether it is something we should do...

Comment 3

[Botond Ballo [:botond]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> It seems both approach involves overriding overflow-x:hidden, which should
> be quite straightforward in our code by just adding a special case in
> nsPresContext::UpdateViewportScrollbarStylesOverride. But yeah, it isn't
> totally clear whether it is something we should do...

Safari's approach involves overriding overflow-x:hidden. Chrome's behaviour is a bit different: they respect overflow-x:hidden, but they size the scroll port of the root scroll frame to be wider than we do in some cases (but not necessarily as wide as the entire content width, so that there may still be some areas that overflow-x:hidden makes unreachable).

This page illustrates the difference:

https://theres-waldo.github.io/mozilla-testcases/viewport/zoomable-overflow-hidden-minscale.html

In Safari, if you scroll horizontally, the blue/green region can be scrolled into view. In Chrome, the blue/green region remains out of reach.

The necessary ingredient to trigger this scenario is a "min-scale" attribute in the page's meta viewport tag. Chrome will size the scroll port of the root scroll frame to the "minimum scale size", which is the size of the visual viewport when zoomed out to the minimum scale. However, the content can be wider than that still (in this case, I made it wider by having an element with width:200%).

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase

This is a testcase for showing three different behaviors mentioned in comment 3:
* in Firefox, only the blue area is reachable, green and red areas are not
* in Chrome, the blue area and the green area are shown by default, and the red area is not reachable
* in Safari, the blue area and the green area are shown by default, and the red area is reachable via scrolling horizontally

Comment 5

[Julien Wajsberg [:julienw]]

I filed bug 1465732 today that is related. The main difference is that the wide element is an iframe, and chrome does something different in this case: it starts with the page zoomed out.

Comment 6

[Botond Ballo [:botond]]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> I filed bug 1465732 today that is related. The main difference is that the
> wide element is an iframe, and chrome does something different in this case:
> it starts with the page zoomed out.

The difference in how we choose the initial zoom level is tracked in bug 1423709.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Moved a bunch of sites will be solved by 'extend-to-zoom' machinery (bug 1494422).

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
> We should either unbreak these sites (presumably by doing either what Chrome
> does, or what Safari does), or agree with Chrome and Safari to all break
> them.

There's a discussion about which of these options to take here:

https://github.com/bokand/bokand.github.io/issues/3

Earlier this year we landed telemetry to help inform the issue (bug 14230174), and today I summarized the results in that thread:

https://github.com/bokand/bokand.github.io/issues/3#issuecomment-433517700

My take on it is that the fraction of websites affected by this issue is fairly significant (5-7% of all mobile page loads), and therefore the web-compatible solution would be for us to adopt Chrome's behaviour.

If anyone else has an opinion on this, please feel free to post in that thread.

Comment 9

[Sean Voisen (:svoisen)]

(In reply to Botond Ballo [:botond] [standards meeting Nov 5-10] from comment #8)

> My take on it is that the fraction of websites affected by this issue is
> fairly significant (5-7% of all mobile page loads), and therefore the
> web-compatible solution would be for us to adopt Chrome's behaviour.

Wow, that's quite high.

Do we want to wait on the Chrome telemetry as confirmation before proceeding with adopting Chrome's behavior? It sounds unlikely that their behavior will change given the webcompat results.

Comment 10

[Botond Ballo [:botond]]

(In reply to Sean Voisen (:svoisen) [On parental leave Nov 15 - Jan 7] from comment #9)
> (In reply to Botond Ballo [:botond] [standards meeting Nov 5-10] from
> comment #8)
> 
> > My take on it is that the fraction of websites affected by this issue is
> > fairly significant (5-7% of all mobile page loads), and therefore the
> > web-compatible solution would be for us to adopt Chrome's behaviour.
> 
> Wow, that's quite high.
> 
> Do we want to wait on the Chrome telemetry as confirmation before proceeding
> with adopting Chrome's behavior? It sounds unlikely that their behavior will
> change given the webcompat results.

No, I think that we should be good to go ahead and implement the behaviour change on our side.

Comment 11

[Botond Ballo [:botond]]

To briefly describe what the change to our Layout code will involve:

We currently assume that the following two quantities coincide:

  (1) The size of the "initial containing block" into which content
      is laid out.

  (2) The bounds past which we do not allow scrolling the root
      content document's root scroll frame if it's overflow:hidden.

The change will involve teasing apart (1) and (2), and changing the size used for (2) on mobile to be the "minimum scale size" (while the size used for (1) will continue to be based on the meta-viewport tag).

Moreover, we will also want to change the size of the rectangle to which we attach position:fixed elements to be based on (2) rather than (1) (that part should be straightforward, since bug 1465616 already allows us to vary the size of this rectangle independently).

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> Moreover, we will also want to change the size of the rectangle to which we
> attach position:fixed elements to be based on (2) rather than (1) (that part
> should be straightforward, since bug 1465616 already allows us to vary the
> size of this rectangle independently).

This part could even be done as an independent change, on pages which are not overflow:hidden. (On pages which are overflow:hidden, making this change in isolation could make fixed elements "out of reach" on some pages.)

Comment 14

[Chris Peterson [:cpeterson]]

[geckoview:p2] for mobile viewport webcompat

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Removing https://webcompat.com/issues/12243 and https://webcompat.com/issues/14490 since those also happen on Chrome.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Removing https://webcompat.com/issues/15216 since I can see the problem in RDM on Chrome.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Moved https://webcompat.com/issues/14740 into bug 1516364.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

FWIW, here is a try with patch set that most of the sites linked to this bug work fine.
There are two sites I am aware of that don't work fine.

- https://webcompat.com/issues/4481 (http://lp.verifone.com/emea/tapandgo/) doesn't scale down because of mRestoreResolution check in UpdateResolution(), I will probably defer this into a follow-up bug
- https://webcompat.com/issues/16851 (http://www.correios.com.br/) there are still unreachable area for some reasons

All the other sites work fine (It's possible I am missing some though).

What the patch set mainly does;

 1) Expanding scroll port size to reach out the overflow:hidden region
 2) Shrink the overflowed contents to the display size if user has never changed zoom level by APZ;
    This is actually what Chrome does, without this change some of the sites linked to this bug don't work as expected

I am going to defer position:fixed element change to a follow-up bug.   Also I am going to split some of trivial commits in the patch set into an independent bug now.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1423013 - Add a web platform test checking documentElement clientWidth is `Initial Containing Block` size even if there is visible overlow:hidden region due to minimum-scale. r=botond

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1423013 - Set explicit minimum-scale=1 to avoid overflow:hidden area reachable and visible. r=botond

Depends on D15344

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1423013 - Expand the layout viewport to the minimum scale size for overflow:hidden pages. r=botond

Depends on D15345

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1423013 - Introduce nsIPresShell::IsResolutionUpdatedByUser to tell whether the resolution has never been changed by user interactions. r=botond

Depends on D15347

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1423013 - Shrink the content to display size if user has never changed the site resolution. r=botond

Depends on D15348

Comment 24

[Hiroyuki Ikezoe (:hiro)]

(In reply to Botond Ballo [:botond] from comment #13)
> (In reply to Botond Ballo [:botond] from comment #11)
> > Moreover, we will also want to change the size of the rectangle to which we
> > attach position:fixed elements to be based on (2) rather than (1) (that part
> > should be straightforward, since bug 1465616 already allows us to vary the
> > size of this rectangle independently).
> 
> This part could even be done as an independent change, on pages which are
> not overflow:hidden. (On pages which are overflow:hidden, making this change
> in isolation could make fixed elements "out of reach" on some pages.)

Filed bug 1516377.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) (PTO: Dec 26 - Dec 31) from comment #18)
> - https://webcompat.com/issues/16851 (http://www.correios.com.br/) there are
> still unreachable area for some reasons

I moved this into bug 1498729 since there are two different meta viewport like this;

1) width=device-width, initial-scale=0.666, maximum-scale=1.0, minimum-scale=0.6666
2) width=device-width, initial-scale=1.0

Chrome does discard the first one when the second one appears so that they use the default minimum-scale value 0.25 so they can reach out the whole contents.

Comment 27

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
>  1) Expanding scroll port size to reach out the overflow:hidden region
>  2) Shrink the overflowed contents to the display size if user has never
> changed zoom level by APZ;
>     This is actually what Chrome does, without this change some of the sites
> linked to this bug don't work as expected

Do these two changes need to land together, or could we split out (2) into a separate bug?

In other words, would doing just (1) cause a regression compared to the current behaviour?

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(In reply to Botond Ballo [:botond] from comment #27)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> >  1) Expanding scroll port size to reach out the overflow:hidden region
> >  2) Shrink the overflowed contents to the display size if user has never
> > changed zoom level by APZ;
> >     This is actually what Chrome does, without this change some of the sites
> > linked to this bug don't work as expected
> 
> Do these two changes need to land together, or could we split out (2) into a
> separate bug?

Yes, we can split (2) into another bug.  Why I didn't do that is half of the sites (or more, I don't recall the exact number) in the See Also links are affected by (2), so it was just my laziness to move the bunch of the sites in a new bug.

> In other words, would doing just (1) cause a regression compared to the
> current behaviour?

No, I don't think so, it shouldn't cause any regressions.

I will drop the review request for (2) here.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Removing https://webcompat.com/issues/16557 since Chrome is not able to scroll to overflow ares.  But interestingly the patches here make Firefox scrollable the area.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Removing https://webcompat.com/issues/12969 since it happens on Chrome too, and the problem there is actually that overflow:hidden is specified in a div element instead of html or body.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

This will be a final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4e8f301a325e49c1c36c877dda81db708a35d95

The reftests added here failed on WebRender for some reasons I will skip them there and handle in bug 1520096 later.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db57901030e9
Add a web platform test checking documentElement clientWidth is `Initial Containing Block` size even if there is visible overlow:hidden region due to minimum-scale. r=botond
https://hg.mozilla.org/integration/autoland/rev/050a2ef2393d
Set explicit minimum-scale=1 to avoid overflow:hidden area reachable and visible. r=botond
https://hg.mozilla.org/integration/autoland/rev/91624c065046
Expand the layout viewport to the minimum scale size for overflow:hidden pages. r=botond,tnikkel

Comment 33

[Oana Pop-Rus]

Backed out 3 changesets (bug 1423013) for failing at reftests/transform/compound-1a.html on a CLOSED TREE

Backoutlink: https://hg.mozilla.org/integration/autoland/rev/8af061c4dfc01a8ecaff8072b70110f85f1c5060

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=221919224&revision=91624c065046140dd88b670829892f3c4e41dc2d

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221919224&repo=autoland&lineNumber=1925

Log snippet: [task 2019-01-15T06:51:01.232Z] 06:51:01 INFO - REFTEST TEST-START | http://10.0.2.2:8854/tests/layout/reftests/transform/compound-1a.html == http://10.0.2.2:8854/tests/layout/reftests/transform/compound-1-ref.html
[task 2019-01-15T06:51:01.233Z] 06:51:01 INFO - REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/transform/compound-1a.html | 106 / 273 (38%)
[task 2019-01-15T06:51:11.750Z] 06:51:11 INFO - REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/transform/compound-1-ref.html | 106 / 273 (38%)
[task 2019-01-15T06:51:11.753Z] 06:51:11 INFO - REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/transform/compound-1a.html == http://10.0.2.2:8854/tests/layout/reftests/transform/compound-1-ref.html | image comparison, max difference: 37, number of differing pixels: 479

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Hi Oana, it seems to me that the failed test passed [1] on a revision after bug 1504659 was backed out (but my patches were still there). Are you sure that my patches caused the failure?

https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=221921472

Comment 35

[Cosmin Sabou [:CosminS]]

Hi Hiro, yes this weirdly got green again after backing out Bug 1504659 here: https://hg.mozilla.org/integration/autoland/rev/73cd4b7b53ba8edbc148e40d101fd9427846ea26

but this was also green before your push (R53), and then got perma orange: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&searchStr=android%2C4.3%2Capi16%2B%2Cdebug%2Creftests%2Ctest-android-em-4.3-arm7-api-16%2Fdebug-reftest-53%2Cr%28r53%29&group_state=expanded&fromchange=749c9dcbbd7f7d8146a71d8cb2a1acb07db1b8c7&tochange=8af061c4dfc01a8ecaff8072b70110f85f1c5060&selectedJob=221919224

and this is also the case for (R27): https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=749c9dcbbd7f7d8146a71d8cb2a1acb07db1b8c7&tochange=8af061c4dfc01a8ecaff8072b70110f85f1c5060&selectedJob=221919301&searchStr=android%2C4.3%2Capi16%2B%2Copt%2Creftests%2Ctest-android-em-4.3-arm7-api-16%2Fopt-reftest-27%2Cr%28r27%29
so I think there is reason to investigate on this. Thank you.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Thanks, Cosmin. That's fair.

I did track down what change actually caused the failure. The change is the RefreshVisualViewportSize call[1] in the case where APZAllowZooming() is false in MobileViewportManager::RefreshViewportSize(). That's surprising to me because I've been thinking that apz.allow_zooming is true on Android. But it's not at least on this test case. Anyway I am going to leave a comment in bug 1504659, and land my patches as it is here.

[1] https://hg.mozilla.org/integration/autoland/rev/738e1ee854eb24b72679b35252a4889b9603c003#l1.36

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8aa60df47b04
Add a web platform test checking documentElement clientWidth is `Initial Containing Block` size even if there is visible overlow:hidden region due to minimum-scale. r=botond
https://hg.mozilla.org/integration/autoland/rev/9d1c137922e1
Set explicit minimum-scale=1 to avoid overflow:hidden area reachable and visible. r=botond
https://hg.mozilla.org/integration/autoland/rev/895cc0297ce2
Expand the layout viewport to the minimum scale size for overflow:hidden pages. r=botond,tnikkel

Comment 38

[Hiroyuki Ikezoe (:hiro)]

I found the pref is disabled on reftest. :/
https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/layout/tools/reftest/remotereftest.py#313

Comment 39

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8aa60df47b04
https://hg.mozilla.org/mozilla-central/rev/9d1c137922e1
https://hg.mozilla.org/mozilla-central/rev/895cc0297ce2

Comment 40

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #38)

I found the pref is disabled on reftest. :/
https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/layout/tools/reftest/remotereftest.py#313

Yeah, I just (re-)discovered this recently as well (bug 1504659 comment 55). It has been the case ever since reftests were first enabled on Android (see bug 1156817), because the reftest environment was missing some things needed for zooming to work properly. That may have been resolved by now, and we may be able to transition Android reftests to use apz.allow_zooming=true (although, while RDM uses apz.allow_zooming=false, some of those tests provide coverage that's useful for RDM).

Comment 41

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14880 for changes under testing/web-platform/tests

Comment 42

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14881 for changes under testing/web-platform/tests

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7232fe5e5eb
[wpt PR 14881] - [Gecko Bug 1423013] Backed out 3 changesets (bug 1423013) for failing at reftests/transform/compound-1a.html on a CLOSED TREE, a=testonly
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb606ba0c10
[wpt PR 15109] - Add a web platform test checking documentElement clientWidth is `Initial Containing Block` size even if there is visible overlow:hidden region due to minimum-scale., a=testonly

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/758d1e4dca08
[wpt PR 14881] - [Gecko Bug 1423013] Backed out 3 changesets (bug 1423013) for failing at reftests/transform/compound-1a.html on a CLOSED TREE, a=testonly
https://hg.mozilla.org/integration/mozilla-inbound/rev/410068244807
[wpt PR 15109] - Add a web platform test checking documentElement clientWidth is `Initial Containing Block` size even if there is visible overlow:hidden region due to minimum-scale., a=testonly

Comment 45

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/758d1e4dca08
https://hg.mozilla.org/mozilla-central/rev/410068244807

Comment 46

[Botond Ballo [:botond]]

Now that important follow-up bugs like bug 1520077 and bug 1519013 are fixed as well, I filed an issue against the Web Viewports Explainer doc to let the Chromium folks know about these changes and hopefully get the document updated to reflect them.

Hiro, thank you so much for taking this on and driving it to completion! It has probably been the #1 viewport-related web compatibility issue affecting Firefox, and it's now resolved.

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Moving back http://www.correios.com.br/ case from bug 1498729 since the content isn't scaled down on Chrome either in the original webcompat report and the difference is Chrome is able to scroll to the content. Now we also are able to scroll the content (probably) thanks to this bug.

Comment 48

[Sean Feng [:sefeng211]]

Hiro, is this still the case and can the change be reverted? It's quite confusing that scrollport includes the overflowed content on Android.

Comment 49

[Hiroyuki Ikezoe (:hiro)]

Sean, would you mind providing the case you are concerned? Though I am not sure what you meant by " scrollport includes the overflowed content", on mobile environments there's a concept named "minimum-scale size" which makes the contents get scaled less than 1.0 ratio, thus the visual viewport size can be larger than the ICB, thus some of overflowed contents can be inside the visual viewport. Note that the visual viewport doesn't equal to the root scrollport.

Comment 50

[Sean Feng [:sefeng211]]

I have this test file https://mozilla.seanfeng.dev/files/lcp/lcp.html that works a bit differently on Firefox Android and Chrome Android. On Firefox, you can scroll the viewport to see the full image, but you can't do that on Chrome.

Comment 51

[Hiroyuki Ikezoe (:hiro)]

It looks same both on Chrome and Firefox on my Pixel3. Probably It somewhat depends on the device screen size?

Anyways, given that Chrome doesn't allow to scroll to where the image is fully visible, it's a Chrome bug?