Closed Bug 1187792 Opened 6 years ago Closed 6 years ago

[APZ] Resize event handler may get the old window inner size when there is scrollbar

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: xidorn, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached file testcase
Steps to reproduce:
1. Open the attachment
2 [review]. Maximum the window
3. Restore the window

The first size in each item is the window inner size, the second is the outer size.

Expected result:
The inner size should always match the outer size (only with the difference of the chrome), and reflect the size after the resize

Actual result:
The inner size remains the old size when the handler is called


This happens on both e10s and non-e10s. Without scrollbar (removing the super tall <div>), on e10s, this could still be reproduced when restoring the window.

It should be a recent regression.
This could cause extra reflow which slows down the fullscreen change.
Blocks: 1187769
Thanks for the regression window.

Kartikaya, could you have a look at this issue?
Flags: needinfo?(bugmail.mozilla)
Interesting. Yeah I can take a look.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(bugmail.mozilla)
I can reproduce bad window.innerWidth/innerHeight values on Linux with APZ enabled. I think the problem is that window.innerWidth/innerHeight return the size of the scroll position clamping scroll port size if one is set [1]. In the case that there is no MobileViewportManager - such as on desktop - that rect doesn't get updated until after an APZ repaint, which happens after the resize event is fired to content.

This shouldn't affect B2G since there we do have a MobileViewportManager which updates the SPCSPS before the resize event is fired to content.

Not 100% sure what the correct fix here is, but I think not setting the SPCSPS on desktop (in APZCCallbackHelper::UpdateRootFrame) is probably a good thing to do. That by itself will fix this bug but we may want to make a few other tweaks as well.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp?rev=6a28170cf9a2#4869
Component: DOM: Core & HTML → Panning and Zooming
Summary: Resize event handler may get the old window inner size when there is scrollbar → [APZ] Resize event handler may get the old window inner size when there is scrollbar
Attached patch PatchSplinter Review
I think this is probably ok for now. My reasoning for the MVM condition change is that if we have a desktop platform which supports zooming (e.g. desktop windows with pinch-zoom) then we want to have the MVM active, but MetaViewportEnabled() still false. Thoughts?
Attachment #8639385 - Flags: review?(botond)
Comment on attachment 8639385 [details] [diff] [review]
Patch

Review of attachment 8639385 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me!

Naming-wise, though, is it still a *mobile* viewport manager if it's used for desktop-with-zooming?
Attachment #8639385 - Flags: review?(botond) → review+
No I guess not.. :( Naming things is hard.

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=64366b523e6c, waiting for the results before landing.
https://hg.mozilla.org/mozilla-central/rev/bd507392b2b3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.