Closed
Bug 641188
Opened 14 years ago
Closed 11 years ago
innerHeight and innerWidth are both initially set to 10 in a framed page or iframe when it is first loaded in a new tab.
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: bugzilla, Assigned: tnikkel)
References
()
Details
(Keywords: dev-doc-complete, regression)
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
look at the docshell size or prescontext visible area depending on if the visible area is overridden
(deleted),
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110311 Firefox/4.0b13pre Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110311 Firefox/4.0b13pre innerHeight and innerWidth are both initially set to 10 in a framed page or iframe when it is first loaded into a new tab. After body has loaded, innerHeight and innerWidth report correctly. Reproducible: Always Steps to Reproduce: 1. Create a page displaying innerHeight and innerWidth in an alert box.(page1) 2. Create a page to load page 1 in a frame or iframe.(page2) 3. Create a page with a link to page 2. (page3) 4. Open page 3 in Firefox. 5. Right click the link and open it in a new tab. Actual Results: innerHeight = 10 innerWidth = 10 Expected Results: innerHeight = your viewports height innerWidth = your viewports width This bug was introduced in firefox4.0b9pre (2010-12-22). Works correctly in firefox4.0b9pre (2010-12-21) and prior versions.
Comment 1•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fb629ae54510&tochange=b0c6a324e72f Anybody else suspect bug 602580?
Blocks: 602580
![]() |
||
Updated•14 years ago
|
Keywords: regression
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → tnikkel
Status: UNCONFIRMED → NEW
Ever confirmed: true
![]() |
||
Updated•14 years ago
|
![]() |
||
Comment 2•14 years ago
|
||
Regression window: Works: http://hg.mozilla.org/mozilla-central/rev/e5ed12d16160 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101221 Firefox/4.0b9pre ID:20101221135104 Fails http://hg.mozilla.org/mozilla-central/rev/a2a3a6e8b0e0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101221 Firefox/4.0b9pre ID:20101221145009 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e5ed12d16160&tochange=a2a3a6e8b0e0 In localbuild, build from db27d37d7879 : fail build from 77956e4dd8f8 : work
![]() |
||
Comment 3•14 years ago
|
||
GAH. The patch in bug 602580 is just totally wrong. It's returning effectively uninitialized values. This affects behavior in desktop Firefox in all sorts of cases: newly-opened tabs, iframes, etc. This may well be worth respinning for. :(
blocking2.0: --- → ?
![]() |
||
Comment 4•14 years ago
|
||
In particular, nothing in GetInner* actually makes sure the _prescontext's_ size is up to date. On the other hand, it looks like we propagate that information sync in some cases, so it might only be the "delayed resize" cases that are affected... So only iframes in background tabs, not all iframes; that sort of thing.
Assignee | ||
Comment 5•14 years ago
|
||
So the problem is that nsDocShell::GetSize flushes, and 602580 removed that call with something that doesn't flush. I thought that EnsureSizeUpToDate was doing the same thing so there was no need for another flush, but EnsureSizeUpToDate flushes the parent.
![]() |
||
Comment 6•14 years ago
|
||
nsDocShell::GetSize also only flushes the parent, fwiw. The point is that flushing the parent updates the _docshell_'s size. It doesn't synchronously update the prescontext's size, for hidden things.
Comment 9•14 years ago
|
||
bz, given comment 8 and the fact that we've shipped 8 betas with this bug (the patch in question has been in Firefox 4 since Beta 4!) do you think this requires a respin, or is a high priority .x fix?
Comment 10•14 years ago
|
||
(In reply to comment #9) > bz, given comment 8 and the fact that we've shipped 8 betas with this bug (the > patch in question has been in Firefox 4 since Beta 4!) do you think this > requires a respin, or is a high priority .x fix? Minor correction - the patch has been in since *Fennec* 4.0b4, or Firefox 4.0b9. (It landed on 2010-12-21, and shipped in 4 desktop betas.)
![]() |
||
Comment 11•14 years ago
|
||
Mike, since this only affects tabs opened in the background (and any subframes in them), we can probably live with a .x fix. Worst case the layout of a page you open in a background tab is completely busted, so you reload and it looks ok... If we do respin, I think we should take this as a ridealong. Timothy, would calling FlushDelayedResize(PR_FALSE) before looking at the prescontext size do the trick here? The important part there being whether something will come along and do the resize reflow later...
Assignee | ||
Comment 12•14 years ago
|
||
I was thinking check if we are using a custom viewport on the presshell, and if so do a flush layout and then get the visible area from the prescontext. If no custom viewport is being used then get the value from the docshell like we used to do. This fails one panorama test (browser/base/content/test/tabview/browser_tabview_bug594958.js) that was checked in after bug 602580, so I'm thinking the test might be broken. I'm continuing to work on this.
![]() |
||
Comment 13•14 years ago
|
||
That gives a noticeable performance penalty on some scripts when using a custom viewport. We can do that if we have to, but it would really be better to avoid the self-flush if at all possible...
![]() |
||
Comment 14•14 years ago
|
||
Ah, and indeed if you FlushDelayedResize(PR_FALSE) that will update the prescontext size but not clear mDelayedResize on the viewmanager or resize the root view, I think, so the resize reflow will happen when the resize is flushed PR_TRUE. Worth double-checking all that, though.
Assignee | ||
Comment 15•14 years ago
|
||
Ok. Either way the difference between getting the inner properties from the docshell or the prescontext seems to be causing differences in browser/base/content/test/tabview/browser_tabview_bug594958.js that I need to understand.
Updated•14 years ago
|
blocking2.0: ? → .x+
Assignee | ||
Comment 16•14 years ago
|
||
browser_tabview_bug594958.js must be wrong because adding a flush to the test makes it fail without any other changes. So we either need to disable the failing parts of that test or fix that test (or its related patch).
blocking2.0: .x+ → ?
Assignee | ||
Comment 18•14 years ago
|
||
I added the flush just after the resizeBy call here http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/tabview/browser_tabview_bug594958.js#110 so that the size change gets flushed to the prescontext. This is a log of the failures and a dump of the canvases the test is looking at. I also made it dump the same canvases in the "passing" case. Looking at the first image it seems the "failed" case is the correct image for the document mentioned.
Assignee | ||
Updated•14 years ago
|
Attachment #521293 -
Attachment is patch: false
Assignee | ||
Comment 19•14 years ago
|
||
So four of the five failures are due to bug 625561: it changed the expected pixels values when it removed a flush in the tabview thumbnail generation code without any reason stated. So I'm concluding that this was a "just make the test pass" change. The other failure is a little more interesting.
Comment 20•14 years ago
|
||
(In reply to comment #11) > If we do respin, I think we should take this as a ridealong. That was 4 weeks ago and still no patch -- how was this ever going to make it as a ridealong? Doesn't seem realistic for Macaw at this point; please renominate if that's wrong.
blocking2.0: ? → -
Comment 21•14 years ago
|
||
I attached a patch that corrects browser_tabview_bug594958.js. WFM when running mochitests locally. The patch is extracted from bug 633096 so winner is the bug that lands first :) Included: Timothy's changes he sent me by mail. (I'm sorry that it took so long to respond.)
![]() |
||
Comment 22•12 years ago
|
||
Bug 801341 also cause by Bug 602580. And we have patch. This should fix asap.
Comment 23•12 years ago
|
||
There's no urgency here to push in a fix for something that's been in product for this long, we should confirm if bug 801341 is showing up in nightly/aurora/beta as well as 16, since we won't respin 16 for this if it's only (for some strange reason) affecting that version. I'd consider an uplift nomination once there's a patch landed on trunk that is verified to fix this, but we wouldn't block a release on this long standing bug.
Assignee | ||
Comment 24•12 years ago
|
||
This still fails the same test. I probably don't have time to look into the failure.
Attachment #528002 -
Attachment is obsolete: true
Assignee | ||
Comment 26•12 years ago
|
||
Here is the try run https://tbpl.mozilla.org/?tree=Try&rev=ef3d09293ac9
Assignee | ||
Comment 27•12 years ago
|
||
Oops, uploaded the wrong patch before.
Attachment #674127 -
Attachment is obsolete: true
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 29•12 years ago
|
||
page1 has a link to page2 page2 has svg which contains script to use innerWidth/innerHeight open page2 through the link to page1 in new tabs in fast succession, sometimes the innerWidth/innerHeight is 10/10.
Assignee | ||
Updated•11 years ago
|
Attachment #674270 -
Attachment description: unbitrotted patch → flush all pending notifications
Assignee | ||
Comment 30•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #521293 -
Attachment is obsolete: true
Assignee | ||
Comment 31•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #674270 -
Attachment is obsolete: true
Assignee | ||
Comment 32•11 years ago
|
||
We have two potential approaches here. 1) flush any delayed resizes queued in the view subsystem and get the inner width/height from the prescontext visible area 2) get the inner width/height from the prescontext visible area if the viewport is overridden (it will always be up to date if overridden, there are no delayed resizes), or the docshell if not. 1) should be green. 2) fails in test_resize_move_windows.html, but that is because of bug 871434. We can trivially modify test_resize_move_windows.html so that it is not exposed to this problem (it is a test that is intended to test general functionality and not this specific case).
Assignee | ||
Comment 33•11 years ago
|
||
Comment on attachment 761148 [details] [diff] [review] flush delayed resize Do you remember this stuff Boris? Feel free to suggest another reviewer.
Attachment #761148 -
Flags: review?(bzbarsky)
![]() |
||
Comment 34•11 years ago
|
||
Comment on attachment 761148 [details] [diff] [review] flush delayed resize If this is green, r=me
Attachment #761148 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 35•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaea6b4c8f79 This definitely needs a test, I just didn't write one yet.
Flags: in-testsuite?
Comment 36•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aaea6b4c8f79
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 37•11 years ago
|
||
Add a note about this bug on the MDN documentation: https://developer.mozilla.org/en-US/docs/Web/API/window.innerWidth https://developer.mozilla.org/en-US/docs/Web/API/window.innerHeight
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•