Consider shrinking content even if no content area gets visible
Categories
(Core :: Panning and Zooming, enhancement, P3)
Tracking
()
People
(Reporter: hiro, Assigned: hiro)
References
(Blocks 2 open bugs)
Details
(Keywords: correctness, Whiteboard: [webcompat])
Attachments
(9 files)
(deleted),
text/html
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 2•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 4•6 years ago
|
||
Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.
Comment 5•6 years ago
|
||
See bug 1547409. Migrating whiteboard priority tags to program flags.
Updated•6 years ago
|
Assignee | ||
Comment 6•5 years ago
|
||
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•5 years ago
|
||
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?
Assignee | ||
Comment 8•5 years ago
|
||
(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.
Assignee | ||
Comment 9•5 years ago
|
||
FWIW, https://webcompat.com/issues/26316 seems a different issue.
Assignee | ||
Comment 10•5 years ago
|
||
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•5 years ago
|
||
(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??
Assignee | ||
Comment 12•5 years ago
|
||
(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•5 years ago
|
||
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.
Assignee | ||
Comment 14•5 years ago
|
||
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)
Assignee | ||
Comment 15•5 years ago
|
||
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•5 years ago
|
||
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?)
Assignee | ||
Comment 17•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 18•5 years ago
|
||
So that the vertical scrollbar on the root element doesn't accidentally appear
because of the expanding the scrollable area.
Assignee | ||
Comment 19•5 years ago
|
||
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
Assignee | ||
Comment 20•5 years ago
|
||
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
Assignee | ||
Comment 21•5 years ago
|
||
There needs a position:fixed element at the right bottom to make the test
failure without proper fixes.
Depends on D40765
Assignee | ||
Comment 22•5 years ago
|
||
Depends on D40766
Assignee | ||
Comment 23•5 years ago
|
||
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
Assignee | ||
Comment 24•5 years ago
|
||
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
Assignee | ||
Comment 25•5 years ago
|
||
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
Assignee | ||
Comment 26•5 years ago
|
||
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•5 years ago
|
||
Comment 28•5 years ago
|
||
bugherder |
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
Updated•5 years ago
|
Assignee | ||
Comment 30•4 years ago
|
||
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.
Assignee | ||
Comment 31•4 years ago
|
||
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.
Description
•