940889 - window.innerWidth is not up-to-date in window.onresize listener

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[zzpdhr]

Attachment: test.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/536.30.1 (KHTML, like Gecko) Version/6.0.5 Safari/536.30.1

Steps to reproduce:

1. Use Firefox for Android nightly open the attached test.html in portrait mode.
2. Use logcat to watch console output.
3. Rotate device to landscape mode.


Actual results:

window.innerWidth is not up-to-date in window.onresize listener as indicated by the log:

"inner size 360 567"


Expected results:

We should get updated window.innerWidth.

Note: It is a regression issue since I could get correct window.innerWidth (640) in Firefox for Android 25.

Comment 1

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

What device are you using? If it's a high dpi device this is probably expected behavior since we are now correctly scaling the content using window.devicePixelRatio.

Comment 2

[zzpdhr]

It is a high dpi device (HTC One). 

But I am not talking about scaling, the issue is I get innerWidth < innerHeight in landscape mode but get innerWidth > innerHeight in portrait mode. I got the opposite result with Firefox for Android 25.

Comment 3

[Aaron Train [:aaronmt]]

Is this working as expected Kats?

Comment 4

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

I'll take a look tomorrow

Comment 5

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

Yeah this appears to be a regression in 27.

Comment 6

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

Actually this probably regressed in bug 919437. I haven't confirmed it but it makes sense because the scrollPositionClampingScrollPortSize isn't set until after the resize happens, and that's what will determine the innerWidth.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Kats - Reassign if you can.

Comment 8

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

I thought about this and I think the right answer is to fire another resize event after we set the scrollPositionClampingScrollPortSize. :tn, do you think that's reasonable?

Comment 9

[Timothy Nikkel (:tnikkel)]

That seems reasonable, but is it what other mobile UA's are doing? ie firing a resize event when innerWidth/Height changes but the document size or window size doesn't change?

Comment 10

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

Unfortunately no, neither Opera nor Chrome on my Nexus 4 fire a resize event in those scenarios. I tested by loading http://people.mozilla.org/~kgupta/tmp/resize.html and pinching in/out - this changes the innerWidth/height but doesn't fire resize events.

I guess for compatibility then we probably don't want do that either. Another option is to somehow delay the resize event that is dispatched until after we have set the scrollPositionClampingScrollPortSize. I'm not sure how to do that.

Comment 11

[Timothy Nikkel (:tnikkel)]

We fire the resize event from PresShell::ResizeReflowOverride, which is called from setCSSViewport (as well as the normal desktop only path). Can we set the scrollPositionClampingScrollPortSize before we call setCSSViewport?

Comment 12

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

Yeah that should be doable. I'll need to rearrange some code in the Fennec browser.js to accomplish it.

Comment 13

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

For the record this turned out to be more complicated than I thought because the resize event is dispatched to content when we try to read the page size, which we (probably) need to do before being able to call scrollPositionClampingScrollPortSize. I'm still trying to figure out a good way to fix this.

Comment 14

[Timothy Nikkel (:tnikkel)]

Perhaps we could dispatch a resize event only after the very first scrollPositionClampingScrollPortSize call?

Comment 15

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

I have patches this fix this now. They depend on the patch I put on bug 944479 because that simplifies some of this code.

Comment 16

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

Attachment: Part 1 - Move a call to getPageSize later

There are two calls to getPageSize between setting the CSS viewport and setting the scroll-clamping size. These calls will trigger the resize event to content if allowed to run. Because they occur before setting the scroll-clamping size, content will get the old innerWidth/innerHeight during the resize event handler.

This patch moves one of the calls to occur after the scroll-clamping size is set. The code block being moved should be independent of the stuff around it.

Comment 17

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

Attachment: Part 2 - Avoid calling getPageSize when setting the scroll clamping size

And this removes the second call. Instead of recomputing whether or not the margins are shown based on the latest page size, this just uses the last known value of whether or not the margins are shown. I believe this is safe because if, as part of the resize, the content changes and the margins are shown/hidden, we will get a MozScrolledAreaChanged event, which will trigger a viewport remeasure and run updateViewportSize all over again anyway.

Comment 18

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

https://tbpl.mozilla.org/?tree=Try&rev=ce29939d16d9

Comment 19

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

Attachment: Part 1 - Move a call to getPageSize later

Whoops, forgot to move a couple of lines.

Updated try push:
https://tbpl.mozilla.org/?tree=Try&rev=549b7657f485

Comment 20

[Chris Lord [:cwiiis]]

Comment on attachment 8344859 [details] [diff] [review]
Part 1 - Move a call to getPageSize later

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

This looks reasonable to me.

Comment 21

[Chris Lord [:cwiiis]]

Comment on attachment 8344851 [details] [diff] [review]
Part 2 - Avoid calling getPageSize when setting the scroll clamping size

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

This also looks reasonable, but I'd test as thoroughly as possible before committing this, to make sure the expected/reasonable behaviour happens on <screenheight pages, ==screenheight pages, >screenheight pages, pages that change size and rotating.

Comment 22

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

I have new patches for this bug that don't depend on bug 944479 since that turned out to be nontrivial to fix. I've pushed them to try at https://tbpl.mozilla.org/?tree=Try&rev=e8f9dcf2174e but I will do some more manual testing on them before putting them up.

Comment 23

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

Attachment: Part 1 - Avoid calling getPageSize when setting the scroll clamping size

This is the same as part 2 (plus a couple of lines from part 1) of the old patches, so carrying r+

Comment 24

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

Attachment: Part 2 - Set the scrollclamping port size along with the CSS viewport size to keep them in sync

Manual testing seems indicate everything is good. Will wait on the try run before requesting review.

Comment 25

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

Comment on attachment 8347283 [details] [diff] [review]
Part 2 - Set the scrollclamping port size along with the CSS viewport size to keep them in sync

Try looks good

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

My understanding is that this is an issue on Firefox OS as well.

Comment 27

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

Same problem, but the code paths are different and the patches will be different. Let's keep them separate.

Comment 28

[Chris Lord [:cwiiis]]

Comment on attachment 8347283 [details] [diff] [review]
Part 2 - Set the scrollclamping port size along with the CSS viewport size to keep them in sync

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

I don't have the time to review this properly before Monday, but is it intentional that setScrollClampingSize is called twice with this patch?

Comment 29

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

Yes. We need to have one call of setScrollClampingSize for every call to setBrowserSize. Note that in the common case the second call to both setBrowserSize and setScrollClampingSize will effectively be no-ops, but until we fix bug 944479 we need them there for the uncommon case.

Comment 30

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

I also just realized I meant to move the second call to setScrollClampingSize up a few lines to just after the call to setBrowserSize but forgot to. I can do that on landing (or leave it as-is if you prefer).

Comment 31

[Chris Lord [:cwiiis]]

Comment on attachment 8347283 [details] [diff] [review]
Part 2 - Set the scrollclamping port size along with the CSS viewport size to keep them in sync

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

Looks fine - I think I'd prefer grouping the setScrollClampingScrollPortSize with the setBrowserSize, along with a comment explaining how they need to be in sync, so that we don't accidentally separate them in the future, but do whatever you think is best.

Comment 32

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

(In reply to Chris Lord [:cwiiis] from comment #31)
> Looks fine - I think I'd prefer grouping the setScrollClampingScrollPortSize
> with the setBrowserSize, along with a comment explaining how they need to be
> in sync, so that we don't accidentally separate them in the future, but do
> whatever you think is best.

I moved the call and added a comment. Landed on fx-team:

remote:   https://hg.mozilla.org/integration/fx-team/rev/c035974b266a
remote:   https://hg.mozilla.org/integration/fx-team/rev/c37a88352e20

This should probably bake on m-c for a while before getting uplifted.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c035974b266a
https://hg.mozilla.org/mozilla-central/rev/c37a88352e20

Comment 34

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

I haven't heard of any regressions from this so it should be good to uplift.

Comment 35

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

Comment on attachment 8347281 [details] [diff] [review]
Part 1 - Avoid calling getPageSize when setting the scroll clamping size

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 919437
User impact if declined: window.innerWidth and window.innerHeight are sometimes wrong when read from content after rotation
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): it's been baking on m-c for a while now so it should be reasonably safe, but this code has poor test coverage and is known to be a bit brittle. Affects fennec only, but it might result in some pages laying out differently (like not taking into account the URL bar).
String or IDL/UUID changes made by this patch: none

Comment 36

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

Comment on attachment 8347281 [details] [diff] [review]
Part 1 - Avoid calling getPageSize when setting the scroll clamping size

(Same as above; this affects beta too)

Comment 37

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

Comment on attachment 8347283 [details] [diff] [review]
Part 2 - Set the scrollclamping port size along with the CSS viewport size to keep them in sync

Ditto

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8347281 [details] [diff] [review]
Part 1 - Avoid calling getPageSize when setting the scroll clamping size

since we haven't yet shipped this issue let's take it to Beta and hope that wider coverage with users helps make up for lack of test coverage (though if there's any way to get more tests...can there be a follow up bug or whiteboard tag for that?)

Comment 39

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/7bb1fe039670
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/155e55d2d1f2

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/daaec3c99758
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/9c6bb821eec0

Comment 40

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

I filed bug 956983 for better test coverage over this code.

Comment 42

[u421692]

Verified as fixed. 
The window.innerWidth is updated:
01-15 00:59:54.711: E/GeckoConsole(4531): inner size 360 567
01-15 01:00:41.751: E/GeckoConsole(4531): inner size 640 287

Also following ths STR from bug 951172 , the issue is not reproducing
1. Go to google.com and choose to search for images
2. Write something in the search field: e.g.: "image"
3. Tap on any image
4. Change the orientation of the screen

Expected result:
The picture is correctly resized after changing orientation of the device

Actual result:
The picture is incorrectly resized after changing orientation of the device, the picture does not fit the screen, there is a white margin