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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Attachment: 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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

This could cause extra reflow which slows down the fullscreen change.

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=738d8ee106fe&tochange=af67b5f73381

Triggered by: Bug 1157745


Regression window( with force apz enabled ):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=49ae0961591e&tochange=fee45cd9a4d9

Regressed by: Bug 1178847

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Thanks for the regression window.

Kartikaya, could you have a look at this issue?

Comment 4

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

Interesting. Yeah I can take a look.

Comment 5

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

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

Comment 6

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

Attachment: Patch

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?

Comment 7

[Botond Ballo [: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?

Comment 8

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

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.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd507392b2b3

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd507392b2b3