New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner:
Closed: Oct 4
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment
visualViewport resize not firing during URL bar movement
Project Member Reported by jakearchibald@chromium.org, Sep 6 Back to list
https://visual-viewport.glitch.me/?debug

Open in a Chrome view where the URL bar is at the top. Avoid Canary due to https://bugs.chromium.org/p/chromium/issues/detail?id=762465.

Scroll downwards slowly from the top of the page (with the URL bar showing).

visualViewport.height updates once the URL bar has fully scrolled out of view, whereas window.innerHeight updates once you lift your finger off the screen.

It feels like window.innerHeight should update at the same time as visualViewport.height. But ideally, both should continually update during scroll to reflect what's actually happening.

In Chrome versions where the URL bar is at the bottom, both visualViewport.height and window.innerHeight update once you lift your finger off the screen. At least this is consistent, but it feels like it's out of sync with other visualViewport properties.
 
Cc: -bokan@chromium.org
Labels: -Pri-3 Hotlist-Input-Dev M-62 Pri-1
Owner: bokan@chromium.org
Status: Assigned
There seems to be two separate issues here:

1) The `resize` event isn't fired against the visualViewport as the URL bar hides. This, even though the value is changing when queried. Here's a more specific test pages for this bug: http://bokand.github.io/vvHeight.html. Querying visualViewport.height in the rAF shows that it changes as the URL bar moves so I would expect we should be firing `resize` at visualViewport.

2) Chrome Home differs in that it doesn't update visualViewport.height as the URL bar moves. I've filed issue 765033 to track that separately.

Lets use this issue to only track #1.

> It feels like window.innerHeight should update at the same time as visualViewport.height. But ideally, both should continually update during scroll to reflect what's actually happening.

I agree for visualViewport.height but I'm not sure that's a good idea for window.innerHeight. I added an UMA to track the amount of time spent executing resize handlers on window (See Blink.EventListenerDuration.Resize uma). About 50% of the time there's no handler. When there is one, about 10% take longer than an entire frame (16ms). The situation is, of course, even worse when we look just at low memory/performance devices (about double the %). Degenerate cases taking >100ms are not uncommon (>2% for low-memory devices).

So I don't think we can fire the window `resize` event as the URL bar moves. Since the resize event should be tied to changes in window.innerHeight it would be irrational to change it without firing `resize` so I think we should keep the current behavior for window (i.e. layout viewport) dimensions. This is consistent since position: fixed Elements don't resize until after the finger lifts so the layout viewport really isn't changing size (admittedly we do nudge position: fixed, bottom fixed elements - it's a bit of a hack...).

Since we don't have the legacy of long running visualViewport resize handlers, I think updating that in real time is fine. I don't think innerHeight and visualViewport.height need to update in sync since they refer to different viewports. So the only AI for this bug is to just make sure the `resize` event fires at visualViewport when the height value actually changes. Does that make sense to you?
Summary: visualViewport resize not firing during URL bar movement (was: visualViewport/innerHeight can update out of sync)
Sounds good to me. If it becomes a problem that innerHeight etc are very much out of sync, we could look at the window.layoutViewport idea again.
Project Member Comment 5 by bugdroid1@chromium.org, Sep 26
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d3055f490aee644987b78da71e338590a71e4b7

commit 9d3055f490aee644987b78da71e338590a71e4b7
Author: David Bokan <bokan@chromium.org>
Date: Tue Sep 26 00:24:12 2017

Moving URL bar fires visualViewport resize event

This CL makes it so that hiding or showing the URL bar continually fires
visualViewport resize events. Previously, the resize event would be
fired only once the user lifts their finger.

Bug:  762518 
Change-Id: I09470b980d09b16d5cd7b60e98e7ec3653fdaa40
Reviewed-on: https://chromium-review.googlesource.com/666600
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504230}
[modify] https://crrev.com/9d3055f490aee644987b78da71e338590a71e4b7/third_party/WebKit/LayoutTests/NeverFixTests
[add] https://crrev.com/9d3055f490aee644987b78da71e338590a71e4b7/third_party/WebKit/LayoutTests/external/wpt/viewport/viewport-url-bar-changes-height-manual.html
[modify] https://crrev.com/9d3055f490aee644987b78da71e338590a71e4b7/third_party/WebKit/Source/core/frame/VisualViewport.cpp

Labels: Merge-Request-62
Requesting merge, confirmed fix in Canary. Realizing it's late in the game for 62, this is a minor change to fix a new API so it should be low risk. It's also been baking in Canary for a week now.
Project Member Comment 7 by sheriffbot@chromium.org, Oct 3
Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: We are only 13 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Does this have to be in M62? How bad could this cl impact Chrome if something goes wrong 
It'd be nice to have since the API just shipped in 61 so authors will be building on the current behavior, the longer it stays like this the more disruptive changing it will become. It's also a very small change and it's isolated to the feature so I think it's low risk.

That said, it's not an especially high usage API so if you feel the risk is too high it's fine to wait until M63.
Labels: -Hotlist-Merge-Review -Merge-Review-62 Merge-Rejected-63
We are definitely thriving to limit the number of CLs/amount of code that get cherry-picked into the release branch. If this change does not fix something critical, then let's have it in 63 since it is not a regression. Thank you!
Status: Fixed
Ok, thanks for the consideration!
Sign in to add a comment