771575 - window.innerWidth/innerHeight return incorrect values during "load" event on page with <meta name="viewport">

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect, P1)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

On a Galaxy Nexus and other high dpi devices (720p), trying to create an element that takes up the entire body area with no scrolling or scaling doesn't work -- the body is 2x as big as it should be.  Testcase is at http://conduit.bitops.com/~vladimir/misc/device-width-test.html -- this should be a green page with an orange border, but on fennec you have twice the width and height that you should on a high dpi device.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The testcase works as expected in stock ICS Browser and in Chrome.  This I think is a pretty big deal for anyone trying to create web apps/pages that work well in fennec, so nominating.

Comment 2

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

Note setting the browser.viewport.scaleRatio pref to 100 fixes this, so it's definitely an interaction with the DPI scaling we have in fennec's browser.js

Comment 3

[Matt Brubeck (:mbrubeck)]

The problem is that window.innerWidth and .innerHeight are returning the wrong value.  Interestingly, they return the correct value if I add a delay before accessing them, for example setTimeout(..., 0).  See also this page, which is identical to Vlad's test case except for the two alert() calls, and produces the correct layout:

http://people.mozilla.com/~mbrubeck/test/device-width-test.html

It looks like the problem is that the <meta name="viewport"> settings are applied shortly *after* the "load" event fires.

Comment 4

[Matt Brubeck (:mbrubeck)]

This is a regression from bug 759678, which deliberately suppresses viewport updates during page load to prevent reflowing or painting the partially-loaded document.  Kats, any ideas for fixing this without regressing 759678?

Comment 5

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

The best idea I have for fixing this involves moving carefully-selected hunks of code from the before-first-paint event handler into the DOMContentLoaded event handler. That should avoid regressing 759678 but I'm worried that it might introduce another regression where, if you navigate to a new page, the old page will jump around while the new page loads.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I think this needs to block WebRT on Android -- as things stand, it makes it impossible to create a web app that will fit to the device screen on high dpi devices with no scrolling.  This is a pretty serious usability and quality issue for web apps.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh, I misunderstood the earlier comment -- if it has a relatively easy workaround (setTimeout 0) it may not be that big of an issue.

Comment 8

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

Kats - Do you have a plan we could start implementing?

Comment 9

[Jason Smith [:jsmith]]

Removing basecamp nom - this sounds like an Android issue (unless I'm misreading something), which isn't tied to basecamp.

Comment 10

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

Attachment: Allow setting browser size without triggering a reflow before first paint happens

This seems to fix it without regressing bug 759678. However I'm worried this may have other side effects and wouldn't really recommend uplifting this.

Comment 11

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

Attachment: Cleanup - removed unused method

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 649729 [details] [diff] [review]
Allow setting browser size without triggering a reflow before first paint happens

>+    // in this case, we should do anything that forces a reflow (see bug 759678)

"we should NOT do anything that forces a reflow", right?

Comment 13

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

(In reply to Matt Brubeck (:mbrubeck) from comment #12)
> "we should NOT do anything that forces a reflow", right?

Right, good catch. Fixed on landing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd673fa4d1d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb90deebb650

Comment 14

[Ed Morley [:emorley]]

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

Comment 15

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

Also https://hg.mozilla.org/mozilla-central/rev/fd673fa4d1d7 - I accidentally landed that patch with the wrong bug number, it should be bug 771575 (this bug) and not bug 771757.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This isn't quite fixed; the hdpi issue still remains.  I modified the testcase at http://conduit.bitops.com/~vladimir/misc/device-width-test.html to show the actual innerWidth/innerHeight that are being reported.  On a Galaxy Nexus, this comes up as 360x519, which is half of what it should be (720x1038 for this page in portait mode, where native device resolution is 720x1280).  Setting scaleRatio to 100 has it report 720x1038 as expected.

So, we're now correctly filling the viewport, but doing it by scaling content at 2x despite the viewport tag explicitly stating device-width/device-height.  We don't seem to support target-densitydpi in the viewport tag to explicitly specify this.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Then again, this is exactly what Chrome and the stock browser do (without target-desnitydpi=device-dpi), so that bit is just bug 737090.

Comment 19

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

Comment on attachment 649729 [details] [diff] [review]
Allow setting browser size without triggering a reflow before first paint happens

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Various layout properties may be incorrect when read from the page's onload function. Bug 789870 is an example of this being encountered in the wild.
Testing completed (on m-c, etc.): on m-c and aurora now
Risk to taking this patch (and alternatives if risky): mobile only
String or UUID changes made by this patch: none

Comment 20

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

https://hg.mozilla.org/releases/mozilla-beta/rev/d8f33dc727d4

Comment 21

[Cristian Nicolae (:xti)]

This issue is fixed on the latest m-a and m-c builds. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-11)
Device: Galaxy Note
OS: Android 4.0.4

Comment 22

[Cristian Nicolae (:xti)]

The orange border doesn't exceed the viewport anymore on the latest Beta build too. Setting the firefox 16 flag as verified.

--
Firefox 16.0b5 (2012-09-26)
Device: Galaxy Note
OS: Android 4.0.4

Comment 23

[Simon Werner]

I get a similar problem as this bug, although it says it's fix.  Using setTimeout to delay my init() function is my current workaround.

Firefox 43.0
Device: Galaxy Note II
OS: Android 4.0.4