Closed Bug 1423013 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Layout, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: botond, Assigned: hiro)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [webcompat:p1][layout:p2][geckoview:p2][wptsync upstream error])

Attachments

(4 files, 2 obsolete files)

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.
Depends on: 1423017
Priority: -- → P1
Priority: P1 → P3
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...
(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%).
Attached file 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
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.
(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.
Priority: P3 → P2
Whiteboard: [webcompat:p1]
Whiteboard: [webcompat:p1] → [webcompat:p1][layout:p2]
Moved a bunch of sites will be solved by 'extend-to-zoom' machinery (bug 1494422).
(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.
(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.
(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.
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).
Blocks: 1508458
Blocks: 1508459
(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.)
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
See Also: → 1513089
Blocks: 1513090
[geckoview:p2] for mobile viewport webcompat
Whiteboard: [webcompat:p1][layout:p2] → [webcompat:p1][layout:p2][geckoview:p2]
Removing https://webcompat.com/issues/15216 since I can see the problem in RDM on Chrome.
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.
Depends on: 1516368
Blocks: 1516377
No longer blocks: 1508459
No longer blocks: 1508458
(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.
Blocks: 1407582
(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.
Blocks: 1465732
(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?
(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.
Attachment #9033275 - Attachment is obsolete: true
Attachment #9033276 - Attachment is obsolete: true
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.

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.

See Also: → 1519007
Blocks: 1519013
Attachment #9033275 - Attachment is obsolete: false
Attachment #9033276 - Attachment is obsolete: false
Attachment #9033275 - Attachment is obsolete: true
Attachment #9033276 - Attachment is obsolete: true
Depends on: 1519607
Depends on: 1520077
Depends on: 1520081
Attachment #9033274 - Attachment description: Bug 1423013 - Make overflow:hidden area reachable within the minimum scale size. r=botond → Bug 1423013 - Expand the layout viewport to the minimum scale size. r=botond
Attachment #9033274 - Attachment description: Bug 1423013 - Expand the layout viewport to the minimum scale size. r=botond → Bug 1423013 - Expand the layout viewport to the minimum scale size for overflow:hidden pages. r=botond
Depends on: 1520096

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.

Pushed by hikezoe@mozilla.com:
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

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

Flags: needinfo?(hikezoe)

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

Flags: needinfo?(hikezoe) → needinfo?(opoprus)

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

Pushed by hikezoe@mozilla.com:
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

(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).

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14880 for changes under testing/web-platform/tests
Whiteboard: [webcompat:p1][layout:p2][geckoview:p2] → [webcompat:p1][layout:p2][geckoview:p2][wptsync upstream]
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14881 for changes under testing/web-platform/tests
Whiteboard: [webcompat:p1][layout:p2][geckoview:p2][wptsync upstream] → [webcompat:p1][layout:p2][geckoview:p2][wptsync upstream error]
See Also: → 1520239
Depends on: 1520455
Flags: in-testsuite+
Depends on: 1520513
No longer depends on: 1520513
Depends on: 1525075
Pushed by james@hoppipolla.co.uk:
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
Pushed by james@hoppipolla.co.uk:
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
See Also: → 1526940
Blocks: 1527187

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.

Regressions: 1536717

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

Flags: needinfo?(hikezoe.birchill)

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.

Flags: needinfo?(hikezoe.birchill) → needinfo?(sefeng)

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.

Flags: needinfo?(sefeng) → needinfo?(hikezoe.birchill)

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?

Flags: needinfo?(hikezoe.birchill)
You need to log in before you can comment on or make changes to this bug.