New issue
Advanced search Search tips

Issue 870008 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Dec 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

[LayoutNG] Fixed positioned objects do not re-position correctly when address bar is hidden

Project Member Reported by kojii@chromium.org, Aug 1

Issue description

Chrome Version: (copy from chrome://version)
OS: Android

What steps will reproduce the problem?
(1) Enable LayoutNG experimental flag.
(2) Open https://www.gizmodo.jp/2018/06/amazon-wobeeco-smartwatch.html
(3) Scroll down, until the bottom bar appears and until the address bar is hidden.

What is the expected result?
The bottom blue bar stays at the bottom.

What happens instead?
The bottom blue bar is up by the height of the address bar.

 
fixed-position-incorrect.jpg
353 KB View Download
fixed-position-correct.jpg
505 KB View Download
Cc: cbiesin...@chromium.org bokan@chromium.org
bokan, does address bar hiding change the viewport/icb size?
I don't get a bottom bar like that at all when I visit the page without LayoutNG. Maybe it's geo-triggered?

In general, the URL bar will affect position: fixed elements but it does not resize the ICB. So if the the bar is |position: fixed, bottom:0| it should stay stuck to the bottom. If it's positioned absolutely/statically then it wont be affected by the URL bar.
The page seems to have stopped showing the bottom bar. Found another one:
https://japanese.engadget.com/pr/j-score-ai/?ncid=egjp_pr_article_bottom
> In general, the URL bar will affect position: fixed elements but it does not resize the ICB. So if the the bar is |position: fixed, bottom:0| it should stay stuck to the bottom. If it's positioned absolutely/statically then it wont be affected by the URL bar.

Thanks for the info. Could you mind to tell us, if we're not changing the ICB size, how do we let the |position: fixed, bottom:0| elements to re-position when URL bar is hidden? If it requires additional event, probably LayoutNG is missing it.
fixed elements stick to the viewport, which is resized (once the user stops scrolling). During the scroll, the renderer isn't resized while the URL bar moves so we adjust the fixed layers on the compositor but that should be independent of LayoutNG.

Once the user lifts their finger, we resize the renderer and Blink (i.e. we get a DOM resize event). AFAIK, fixed elements use the FrameView's frame rect for position which will be resized at this time. IIRC there was code in FrameView relating to this, see LocalFrameView::AddViewportConstrainedObject. I'm not terribly familiar with how those get recalculated but if you follow the calls/reads of the viewport constrained objects it should become clear.
and FYI: there's details on how viewport resizing works (it's complicated) here: https://developers.google.com/web/updates/2016/12/url-bar-resizing
Thank you so much bokan@, this is a great article.

LayoutNG computes ICB from FrameView::GetLayoutSize() and use it everywhere AFAIU. From what you said, I guess we need to use FrameView::FrameRect() instead for fixed elements.

Is there any tests where GetLayoutSize() and FrameRect() do not match?
The most relevant would be in browser_controls_test.cc, see the 5 tests from and below DontAffectLayoutHeight.

If you want to set this up in a test, you'll want to use WebViewImpl::ResizeWithBrowserControls. Set a "top_controls_height" (bottom_controls you can set to 0) and set "browser_controls_shrink_layout" to false. Whatever size you set for "new_size" will be used as the frame_rect size while the ICB will be sized to the new_size - top_controls_height. You can do this now in layout tests too, see LayoutTests/virtual/android/rootscroller/position-fixed-in-unscrollable-document.html.
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3c4c57c94ae1cefa86c0dfb1007998e981de87b6

commit 3c4c57c94ae1cefa86c0dfb1007998e981de87b6
Author: Koji Ishii <kojii@chromium.org>
Date: Wed Sep 19 07:52:08 2018

[LayoutNG] unskip virtual/android

Unskip "virtual/android" for LayoutNG as these tests seem to
be relevant for  crbug.com/870008 . This directory was marked
to skip in r499723.

TBR=mstensho@chromium.org, atotic@chromium.org, cbiesinger@chromium.org
NOTRY=true

Bug:  870008 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I37094e5174ba1941ae9d927d5462a0985b1ade7a
Reviewed-on: https://chromium-review.googlesource.com/1233273
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592338}
[modify] https://crrev.com/3c4c57c94ae1cefa86c0dfb1007998e981de87b6/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG

This still bothers my dogfood almost everyday...I'll try if no one is going to pick this up.
Owner: atotic@chromium.org
Will take a look at this today
Possibly related are the failing webkit_unit_tests:
BrowserControlsSimTest.AffectLayoutHeightWhenConstrained (../../third_party/blink/renderer/core/frame/browser_controls_test.cc:816)
BrowserControlsTest.DontAffectLayoutHeight (../../third_party/blink/renderer/core/frame/browser_controls_test.cc:773)

(You can now run webkit_unit_test --enable-blink-features=LayoutNG; make sure your tree is up-to-date)
Cc: ikilpatrick@chromium.org
Constraint space for position:fixed Elements inside ICB has a special AvailableSize.
Regular element's AvailableSize is "largest possible viewport" size.
position:fixed Available size is the actual size of the viewport.

Example:
  <body>
    <div id="fix" style="height:100vh;position:fixed"></div>
    <div id="abs" style="height:100vh;position:absolute"></div>

 #fix.height != #abs.height when url bar is showing.

In the current NG design, sizes needed to generate a fragment
are stored on ConstraintSpace. This will increase CS size
with a record that is almost never used. 

@ikilpatrick, you've been playing with minimization of
NGConstraintSpace. Any ideas for an alternative to adding
another rarely used size to NGConstraintSpace?
Project Member

Comment 14 by bugdroid1@chromium.org, Nov 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/68a755f9b2bdc9a682158c61445fb89ba4589c5f

commit 68a755f9b2bdc9a682158c61445fb89ba4589c5f
Author: Aleks Totic <atotic@chromium.org>
Date: Tue Nov 27 08:26:32 2018

[LayoutNG] position:fixed has special ICB

There are two tests that test this, but they are currently failing
in Legacy too.
virtual/android/url-bar/bottom-and-top-fixed-sticks-to-top.html
virtual/android/url-bar/bottom-fixed-adjusted-when-showing-url-bar.html

I've confirment manually that position:fixed, bottom:0 Elements
stick to bottom on scrolling.

There is one remaining problem.
While scrolling is active, position:fixed, bottom:0 element does
not stick to the bottom of the viewport. It gets positioned correctly
after scrolling stops. Bug #906516

Bug:  870008 
Change-Id: Ibe597141869794978865809caa3991cd4c2bd8a6
Reviewed-on: https://chromium-review.googlesource.com/c/1341127
Commit-Queue: Aleks Totic <atotic@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611045}
[modify] https://crrev.com/68a755f9b2bdc9a682158c61445fb89ba4589c5f/third_party/blink/renderer/core/layout/ng/layout_ng_block_flow.cc
[modify] https://crrev.com/68a755f9b2bdc9a682158c61445fb89ba4589c5f/third_party/blink/renderer/core/layout/ng/ng_container_fragment_builder.h
[modify] https://crrev.com/68a755f9b2bdc9a682158c61445fb89ba4589c5f/third_party/blink/renderer/core/layout/ng/ng_out_of_flow_layout_part.cc
[modify] https://crrev.com/68a755f9b2bdc9a682158c61445fb89ba4589c5f/third_party/blink/renderer/core/layout/ng/ng_out_of_flow_layout_part.h

Status: Fixed (was: Available)
One small remaining issue tracked in #906516

Thank you!
Status: Verified (was: Fixed)
This issue is fixed in current M72 build 72.0.3626.7. Verified using the link in the comment #3.

Sign in to add a comment