Closed Bug 940889 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

27 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 29
Tracking Status
firefox26 --- unaffected
firefox27 --- verified
firefox28 --- verified
firefox29 --- verified
fennec 27+ ---

People

(Reporter: zzpdhr, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

Attached file 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.
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.
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.
Is this working as expected Kats?
I'll take a look tomorrow
Flags: needinfo?(bugmail.mozilla)
Yeah this appears to be a regression in 27.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Flags: needinfo?(bugmail.mozilla)
OS: Mac OS X → Android
Hardware: x86 → ARM
Version: Trunk → Firefox 27
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.
Kats - Reassign if you can.
Assignee: nobody → bugmail.mozilla
tracking-fennec: ? → 27+
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?
Flags: needinfo?(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?
Flags: needinfo?(tnikkel) → needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
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?
Yeah that should be doable. I'll need to rearrange some code in the Fennec browser.js to accomplish it.
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.
Perhaps we could dispatch a resize event only after the very first scrollPositionClampingScrollPortSize call?
I have patches this fix this now. They depend on the patch I put on bug 944479 because that simplifies some of this code.
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.
Attachment #8344850 - Flags: review?(chrislord.net)
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.
Attachment #8344851 - Flags: review?(chrislord.net)
Whoops, forgot to move a couple of lines.

Updated try push:
https://tbpl.mozilla.org/?tree=Try&rev=549b7657f485
Attachment #8344850 - Attachment is obsolete: true
Attachment #8344850 - Flags: review?(chrislord.net)
Attachment #8344859 - Flags: review?(chrislord.net)
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.
Attachment #8344859 - Flags: review?(chrislord.net) → review+
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.
Attachment #8344851 - Flags: review?(chrislord.net) → review+
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.
No longer depends on: 944479
This is the same as part 2 (plus a couple of lines from part 1) of the old patches, so carrying r+
Attachment #8344851 - Attachment is obsolete: true
Attachment #8347281 - Flags: review+
Manual testing seems indicate everything is good. Will wait on the try run before requesting review.
Attachment #8344859 - Attachment is obsolete: true
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
Attachment #8347283 - Flags: review?(chrislord.net)
My understanding is that this is an issue on Firefox OS as well.
blocking-b2g: --- → 1.3+
Same problem, but the code paths are different and the patches will be different. Let's keep them separate.
blocking-b2g: 1.3+ → ---
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?
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.
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 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.
Attachment #8347283 - Flags: review?(chrislord.net) → review+
(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.
Whiteboard: [bake on m-c until 2014]
https://hg.mozilla.org/mozilla-central/rev/c035974b266a
https://hg.mozilla.org/mozilla-central/rev/c37a88352e20
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Whiteboard: [bake on m-c until 2014] → [NO_UPLIFT][bake on m-c until 2014]
I haven't heard of any regressions from this so it should be good to uplift.
Whiteboard: [NO_UPLIFT][bake on m-c until 2014]
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
Attachment #8347281 - Flags: approval-mozilla-aurora?
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)
Attachment #8347281 - Flags: approval-mozilla-beta?
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
Attachment #8347283 - Flags: approval-mozilla-beta?
Attachment #8347283 - Flags: approval-mozilla-aurora?
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?)
Attachment #8347281 - Flags: approval-mozilla-beta?
Attachment #8347281 - Flags: approval-mozilla-beta+
Attachment #8347281 - Flags: approval-mozilla-aurora?
Attachment #8347281 - Flags: approval-mozilla-aurora+
Attachment #8347283 - Flags: approval-mozilla-beta?
Attachment #8347283 - Flags: approval-mozilla-beta+
Attachment #8347283 - Flags: approval-mozilla-aurora?
Attachment #8347283 - Flags: approval-mozilla-aurora+
I filed bug 956983 for better test coverage over this code.
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
You need to log in before you can comment on or make changes to this bug.