1508177 - Consider shrinking content even if no content area gets visible

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

In bug 1423709 we are going to shrink contents to screen size if the content doesn't specify initial-scale, BUT we are NOT going to shrink in the case where *no content area* is visible.  For example, if the screen size is 400x800 and the content size is 800x800 we don't shrink the content.

I believe there exists such sites in the wild, but I think it's not so common.  And to do that as Botond said, it seems more work would be necessary (actually I don't know what we need though).

Comment 1

[Karl Dubost💡 :karlcow]

should this be tied to Bug 1123938

Comment 2

[Botond Ballo [:botond]]

Yup.

Note that for overflow:hidden pages, bug 1423013 will affect the behaviour here as well (the "content area" that we compute will get larger on some overflow:hidden pages).

Comment 4

[Firefox Bug Husbandry Bot]

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Comment 5

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating whiteboard priority tags to program flags.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: half-height-content.html

I've started looking at this, but I still don't understand what we need to do for this. So I did dump some metrics in FrameMetrics in ComputeScrollMetadata when opening horizontal-overflow-hidden-region.html with commenting out code restricting using the minimum scale size.

Here is a dump;
horizontal-overflow-hidden-region.html
resolution: 0.500000
composition bounds: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
display port: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
critical display port:(x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
scrollable rect: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
root composition size:(w=800.000000, h=1120.000000)
layout viewport: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)

For comparison I did get another dump for a half height content of the screen height;
half-height-content.html
resolution:1.000000
composition bounds: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
display port: (x=0.000000, y=0.000000, w=400.000000, h=560.000000)
critical display port:(x=0.000000, y=0.000000, w=400.000000, h=560.000000)
scrollable rect: (x=0.000000, y=0.000000, w=400.000000, h=560.000000)
root composition size:(w=400.000000, h=560.000000)
layout viewport: (x=0.000000, y=0.000000, w=400.000000, h=560.000000)

So, I guess what we need to do here at least is adjusting the height values other than 'composition bounds'? I.e. w=800, h=560?

Botond, does this sound the right way? Or do you know who knows?

FWIW, I played a local build with commenting out the restriction on a real Android phone, I haven't found any issues there so I need a concrete test case that simply commenting out the code causes problems.

Comment 7

[Botond Ballo [:botond]]

My recollection is that trying to do this caused some graphical issues, e.g. garbage content being shown in the "no content" areas. However, that was a few years ago and the issue might have been resolved since then.

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

Here is a dump;
horizontal-overflow-hidden-region.html
resolution: 0.500000
composition bounds: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
display port: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
critical display port:(x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
scrollable rect: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
root composition size:(w=800.000000, h=1120.000000)
layout viewport: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)

These numbers don't really make sense to me. The page has a div whose width is 300%. Shouldn't the scrollable rect width be something like 2400px (for an 800px wide screen)?

Perhaps you are looking at the FrameMetrics for a different scroll frame, such as the chrome document's root scroll frame?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

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

My recollection is that trying to do this caused some graphical issues, e.g. garbage content being shown in the "no content" areas. However, that was a few years ago and the issue might have been resolved since then.

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

Here is a dump;
horizontal-overflow-hidden-region.html
resolution: 0.500000
composition bounds: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
display port: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
critical display port:(x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
scrollable rect: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
root composition size:(w=800.000000, h=1120.000000)
layout viewport: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)

These numbers don't really make sense to me. The page has a div whose width is 300%. Shouldn't the scrollable rect width be something like 2400px (for an 800px wide screen)?

The device screen width is 400px (got by document.documentElement.getBoundingClientRect) and the html has 'overflow:hidden' and 'minimum-scale=0.5' so the value looks correct to me. Sorry for the confusion.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

FWIW, https://webcompat.com/issues/26316 seems a different issue.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Here is another dump on overflow-hidden-region.html

overflow-hidden-region.html
resolution: 0.500000
composition bounds: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
display port: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
critical display port:(x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
scrollable rect: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)
root composition size:(w=800.000000, h=1120.000000)
layout viewport: (x=0.000000, y=0.000000, w=800.000000, h=1120.000000)

All metrics are pretty much same as for horizontal-overflow-hidden-region.html case. Though horizontal-overflow-hidden-region.html has actually no content on lower half part of the window.

Comment 11

[Botond Ballo [:botond]]

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

The device screen width is 400px

Ah, ok. I was confused by the composition bounds being 800px wide, but of course that's in screen pixels (includes resolution and device scale).

So then, how does the scrollable rect become 1120px tall? How tall is the ICB??

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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

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

The device screen width is 400px

Ah, ok. I was confused by the composition bounds being 800px wide, but of course that's in screen pixels (includes resolution and device scale).

So then, how does the scrollable rect become 1120px tall? How tall is the ICB??

Oh right. That's probably my fault. I did just comment out the restriction of mMinimumScaleSize so that the scrollable rect's height was expanded to over the content height. Thanks! I think I am getting understand what we need here. We have to probably make the content zoomable with scrollable rect (800, 560) and layout viewport(800, 560)? (I have no idea other metrics mean yet)

Comment 13

[Botond Ballo [:botond]]

Assuming the content size is actually (800, 560), here is my intuition:

• mLayoutViewport should be (800, 1120). This preserves the property that the visual viewport always fits into the layout viewport, and that the layout viewport is sized to the minimum scale size, which is the visual viewport's size at the minimum scale.
• mScrollableRect should probably stay at (800, 560), to reflect the actual content size.
• We have an "expanded scrollable rect", which should be (800, 1120). The existing logic for computing it might already give the right answer.
  • This is important because we use the expanded scrollable rect to determine things like zooming bounds during pinch-zooming. If we keep the expanded scrollable rect at (800, 560), we'll probably hit this assertion when pinch-zooming near the minimum scale.
• The displayport probably doesn't need to be touched. I think it will pick up the size of the expanded scrollable rect automatically, due to logic such as here and here.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Thanks for the summary. I will do with the way.

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

• mLayoutViewport should be (800, 1120). This preserves the property that the visual viewport always fits into the layout viewport, and that the layout viewport is sized to the minimum scale size, which is the visual viewport's size at the minimum scale.

I did add a position:fixed element at right bottom of horizontal-overflow-hidden-region.html, it appears at right bottom of the device screen on Chrome. That's surprising me, but yeah you are right. :)

(I did link to a wrong file, now fixed it to horizontal-overflow-hidden-region.html)

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I started thinking that we will end up replacing all FrameMetrics::GetScrollableRect() call sites (and direct use cases of mScrollableRect) in gfx.

First of all, we need to change mScrollableRect in FrameMetrics::CalculateScrollRange() because even if the range is outside of the content, it should be inside scroll range in APZ side, otherwise user can't move to the range at all.

Secondly we also need to change GetScrollableRect calls in AsyncPanZoomController::HandleDragEvent, without this change user can't do pinch zoom in/out at all. I haven't tried all APZ functionalities on real Android devices but we will probably need to replace a GetScrollableRect call in AsyncPanZoomController::GetKeyboardDestination, GetScrollableRect calls in AsyncPanZoomController::OnScale and so on.

Then I went back to the example in comment 6 again that the content height is half of the window height.

scrollable rect: (x=0.000000, y=0.000000, w=400.000000, h=560.000000)

This scrollable rect height is the window height, not the content height (280). Where the height value came from is viewport's size, it was used for a Reflow call, which is ended up being used as layoutSize in nsHTMLScrollFrame::TryLayout(if we are not using the minimum scale size) which is called from nsGfxScrollFrame::Reflow().

So in terms of scrollable in FrameMetrics both on the main thread and APZ side, we already allow the size is overflowed from the scrolled content height. So, instead of replacing GetScrollableRect calls in gfx one by one, it seems to me that setting the overflowed rect size as scrollable is a reasonable approach. (we can do it behind a pref which is enabled only on nightly by default to see what happens).

Botond, what do you think? (I don't rush so it's fine to get the answer after you come back to work)

Comment 16

[Botond Ballo [:botond]]

You're right, if the user can scroll to the "no content" areas after zooming in, then it makes sense to include them in mScrollableRect.

I initially thought the user can't scroll there, but now that I think about it more:

• We established that fixed-position content can be in the "no content" area
• We want the user to be able to look at the fixed-position content when zoomed in
• We probably don't want to move the fixed-position content (relative to the layout viewport) as we zoom

The above suggests that the user needs to be able to scroll there.

So, yeah, let's go ahead and try to change mScrollableRect as well and see if that works!

(Perhaps, as a follow-up, we can then remove the "extended scrollable rect" altogether.)

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

Secondly we also need to change GetScrollableRect calls in AsyncPanZoomController::HandleDragEvent, without this change user can't do pinch zoom in/out at all.

(I'm confused about this part. What is the connection between drag events and pinch zooming?)

Comment 17

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15006cf03620ed57d9b2ae019578cbeda4f44d74
A reftest added in this bug fails on WebRender. The reftest in quesion uses async-scroll-{x,y} to move the right bottom of the expanded layout viewport and there is a position:fixed element at the right bottom but the position:fixed element doesn't appear in the result of the reftest. (it's due to bug 1536360?). I am going to file a bug to track it and mark the reftest as FAIL for now.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1508177 - Add an explicit "initial-scale=1,minimum-scale=1" meta viewport tag to avoid expanding scrollable area. r?botond

So that the vertical scrollbar on the root element doesn't accidentally appear
because of the expanding the scrollable area.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1508177 - Tests for various web exposed metrics on the content that the content height is shorter than the layout viewport height. r?botond

The values other than window.innerHeight in this test, visualViewport.height,
documentElement.scrollHeight and documentElement.getBoundingClientRect().height
are same as values on Chrome. window.innerHeight value will be changed in
bug 1514429.

Depends on D40763

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1508177 - Add reftests to make sure position:fixed elements are attached to the layout viewport even if the layout viewport contains no visible contents area. r?botond

position-fixed-on-half-height-content.html is a test case that the document's
layout viewport contains no visible contents area without scaling.

position-fixed-on-landscape-content.html is a test case that the document's
layout viewport will contain no visible contents area if the document is scaled
down to fit the document to screen size.

position-fixed-on-square-content.html is a test case that the document's layout
viewport will contain no visible contents ares if the document is scaled up to
fit the document to screen size.

The latter two cases currently fail.

Depends on D40764

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1508177 - Add reftests to make sure we can scroll to expaned layout viewport areas where no content exists. r?botond

There needs a position:fixed element at the right bottom to make the test
failure without proper fixes.

Depends on D40765

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1508177 - Add reftests to make sure that expanded layout viewport areas that are initially out of the visual viewport are rendered by setting resolution. r?botond

Depends on D40766

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1508177 - Reftests that check scrollbars are properly appeared by the difference between visual and layout viewports. r?botond!,tnikkel!

scrollbars-in-landscape-content.html doesn't fail on environments where we don't
use overlay scrollbars because scrollbars for the visual viewport are not
rendered there.

Depends on D40767

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1508177 - Expand the minimum scale height even if the expanded area doesn't contain any contents. r?botond

As a result of the expansion, position:fixed elements are attached to the
expanded layout viewport.

The expanded value is used behind a pref which is enabled by default on nightly
initially, and the pref will be fliped in bug 1571599 on other channels.

scrollbars-in-landscape-content.html still fails since the vertical overlay
scrollbar doesn't appear since we are not yet using the expanded value during
reflow to tell whether we need overlay scrollbars or not. This will be fixed
by the next commit.

Depends on D40770

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1508177 - Use expanded layout viewport height to tell whether we need to render the vertical overlay scrollbar. r?tnikkel

In the previous commit, we expanded layout viewport height but during reflow
the expanded layout viewport size hasn't reflected in
ScrollReflowInput::mContentsOverflowAreas.ScrollableOverflow(). We explicitly
need to use the expanded height to tell whether we need vertical vertical
scrollbars or not.

Note that the expanded layout viewport height should NOT be used for non-overlay
scrollbars cases since, for example, if the content width is specified by
width: 100%, the non-overlay vertical scrollbar narrows down the content's
used width a little bit because of the vertical scrollbar width, which in turn
the minimum scale gets bigger because the content's width became bit narrower,
thus the layout viewport size calculated with the minimum scale gets smaller,
then it results the vertical scrollbar no longer needs to be rendered, thus
the minimum scale gets back to the original value, then the vertical scroll
needs to be rendered again, thus this sequence of processes happens repreatedly.

Depends on D40771

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Patches for this bug make css-ui-valid/select/select-disabled-fieldset-2.html failure, but I can see the failure locally on bare m-c both on x86/arm Android emulators even with/without E10S. So I suppose it's a pre-existing issue, I will add fuzzy-if(Android,0-9,0-1) for the test. select-disabled-fieldset-1.html has the same fuzzy-if, probably it's the same root cause.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f72a37c3a6b8
Add an explicit "initial-scale=1,minimum-scale=1" meta viewport tag to avoid expanding scrollable area. r=botond
https://hg.mozilla.org/integration/autoland/rev/55695642b57b
Tests for various web exposed metrics on the content that the content height is shorter than the layout viewport height. r=botond
https://hg.mozilla.org/integration/autoland/rev/23495a4d239e
Add reftests to make sure position:fixed elements are attached to the layout viewport even if the layout viewport contains no visible contents area. r=botond
https://hg.mozilla.org/integration/autoland/rev/64c444bacabc
Add reftests to make sure we can scroll to expaned layout viewport areas where no content exists. r=botond
https://hg.mozilla.org/integration/autoland/rev/e9a182bf1590
Add reftests to make sure that expanded layout viewport areas that are initially out of the visual viewport are rendered by setting resolution. r=botond
https://hg.mozilla.org/integration/autoland/rev/156532527ac9
Reftests that check scrollbars are properly appeared by the difference between visual and layout viewports. r=botond,tnikkel
https://hg.mozilla.org/integration/autoland/rev/eb738d246d40
Expand the minimum scale height even if the expanded area doesn't contain any contents. r=botond
https://hg.mozilla.org/integration/autoland/rev/06f8821f01d7
Use expanded layout viewport height to tell whether we need to render the vertical overlay scrollbar. r=tnikkel

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f72a37c3a6b8
https://hg.mozilla.org/mozilla-central/rev/55695642b57b
https://hg.mozilla.org/mozilla-central/rev/23495a4d239e
https://hg.mozilla.org/mozilla-central/rev/64c444bacabc
https://hg.mozilla.org/mozilla-central/rev/e9a182bf1590
https://hg.mozilla.org/mozilla-central/rev/156532527ac9
https://hg.mozilla.org/mozilla-central/rev/eb738d246d40
https://hg.mozilla.org/mozilla-central/rev/06f8821f01d7

Comment 29

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

https://searchfox.org/mozilla-central/rev/e11d919cdc8a909354eb2c3e19904d9229c55d88/layout/generic/nsGfxScrollFrame.cpp#5984

Can that hunk be removed now?

Comment 30

[Hiroyuki Ikezoe (:hiro)]

If I did precisely understand what the problem is at that time, yes. But now I am quite unsure, let me try to do double check it. I am going to build a local Fenix build to see whether it can be now removed tomorrow.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

I did check an archived version of mozilla.org/en-us/firefox on a local build Fenix without the fullscreen check. As far as I can tell it works pretty fine. Filed bug 1650686 for removing it.