test_innerWidthHeight_metaviewport.html fails on android, possible incorrect size calculations

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
9 years ago
4 months ago

People

(Reporter: jmaher, Unassigned)

Tracking

Trunk
ARM
Android

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mobile_unittests] [mobile_dev_needed])

Attachments

(2 attachments, 2 obsolete attachments)

in running the dom/tests/mochitest/dom-level0 tests, I get 2 failures:
8 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/dom-level0/test_innerWidthHeight_metaviewport.html | innerWidth is css viewport width - got 800, expected 320
9 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/dom-level0/test_innerWidthHeight_metaviewport.html | innerHeight is css viewport height - got 580, expected 320

This is on a tegra with 1024x768 screen resolution.  I see that this test was added fairly recently in bug 602580 which dealt with some of these issues on fennec, so I am assuming we should fix this test to work on the tegras.
Whiteboard: [mobile_unittests] [mobile_dev_needed]
Assignee: nobody → wjohnston
Posted patch Patch v1 (obsolete) — Splinter Review
This fixes using setTimeout. We also can't expect innerHeight to be 320 if the page's aspect ratio is > 1 (portrait mode). This just throws away the test in that case.

It doesn't appear that the viewport is actually updated until after this test has run. I am sure this worked on desktop when I checked it in, so maybe we regressed something. We update the viewport on DOMContentLoaded and pageshow:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/content.js#637

however those are Async messages, so I'm not sure that callers can always expect innerWidth and Height to be correct when onload fires.
Attachment #521542 - Flags: review?(mark.finkle)
The setTimeout approach won't fly here. We need a better way to make it work.
Attachment #521542 - Flags: review?(mark.finkle) → review-
Posted patch Patch v2 (obsolete) — Splinter Review
Alternatively, we can watch for MozAfterPaint which usually fires later for me on desktop. Still not a real assurance without us sending a "MozMetadataLoaded" event.
Attachment #521542 - Attachment is obsolete: true
Attachment #521611 - Flags: review?(mark.finkle)
Comment on attachment 521611 [details] [diff] [review]
Patch v2

We do fire messages when we get meta data. Have you tried those?

r- until we test the metadata messages
Attachment #521611 - Flags: review?(mark.finkle) → review-
We can write the same test in mobile-browser and key off of SetCSSViewport calls until we find the correct metaviewport has been set (within some timeout). Because those messages are proprietary to m-b, will re-implement this test there (coming in a minute). This deletes the test from mozilla-central.
Attachment #521611 - Attachment is obsolete: true
Attachment #523115 - Flags: review?(mark.finkle)
Posted patch MB TestSplinter Review
Moves test to mobile browser.
Attachment #523161 - Flags: review?(mark.finkle)
Comment on attachment 523115 [details] [diff] [review]
Remove tests from MC

Yes, this is a mobile specific feature, so let's test it there
Attachment #523115 - Flags: review?(mark.finkle) → review+
Comment on attachment 523161 [details] [diff] [review]
MB Test

Looks good to me. Got some feedback Matt?
Attachment #523161 - Flags: review?(mark.finkle)
Attachment #523161 - Flags: review+
Attachment #523161 - Flags: feedback?(mbrubeck)
Attachment #523161 - Flags: feedback?(mbrubeck) → feedback+
Assignee: wjohnston → nobody
Closing all opened bug in a graveyard component
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.