358303 - window.open results in wrong innerHeight

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Robert Sayre]

First bug caught by mochitest? While writing a test for Bug 344861, I found this:

window.open("/", "testwindow", "width=200,height=200");

I get window.innerWidth==200 and window.innerHeight=197.

UNLESS I have the "Hello World" status bar extension installed. Then I get 200 for both.

http://developer.mozilla.org/en/docs/Creating_a_status_bar_extension

The height of the status bar varies with the little lock icon present in SSL sites as well.

Comment 1

[Robert Sayre]

This is present in 1.5 as well.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Testcase?

Comment 3

[Robert Sayre]

(In reply to comment #2)
> Testcase?

Checking in tests/test_bug344861.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug344861.html,v  <--  test_bug344861.html

Comment 4

[Boris Zbarsky [:bzbarsky]]

Actually, I just tried:

javascript:window.open("data:text/html,<a href='javascript:alert(window.innerHeight)'>Click Me</a>", "testwindow", "width=200,height=200");

This gives me "200" in SeaMonkey, but I do get "196" in Firefox.  Is the Firefox browser window code resizing (or showing?) some part of the chrome too late?  That's the only way I can think of for this to happen.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> > Testcase?
> Checking in tests/test_bug344861.html;

Sure, but how do I actually load that one test in a debug build?  Do I need to pull that directory and load from file:// ?  Or something else?

Comment 6

[Robert Sayre]

>
> Sure, but how do I actually load that one test in a debug build?  Do I need to
> pull that directory and load from file:// ?  Or something else?

There is a README file in the directory. :) 

Comment 7

[Boris Zbarsky [:bzbarsky]]

So in Firefox, the window is initially created with the right size.  I see the content area resize to the wrong size under reflow, with the reflow itself triggered by the following JS:

(gdb) jsstack 
0 [native frame]
1 isElementVisible(aElement = [object XULElement @ 0x8c1b218 (native @ 0x8e278d8)]) ["chrome://browser/content/utilityOverlay.js":473]
    bo = [object BoxObject @ 0x915eeb0 (native @ 0x9124478)]
    this = [object ChromeWindow @ 0x8e44ca8 (native @ 0x8ffecf4)]
2 BrowserSearch_getSearchBar() ["chrome://browser/content/browser.js":2763]
    searchBar = [object XULElement @ 0x8c1b218 (native @ 0x8e278d8)]
    this = [object Object]
3 anonymous() ["chrome://browser/content/browser.js":2685]
    searchButton = undefined
    engines = undefined
    this = [object Object]
4 anonymous() ["chrome://browser/content/browser.js":3529]
    this = [object Object]
5 pageShowEventHandlers(event = [object PageTransitionEvent @ 0x82a4210 (native @ 0x8ff4a0c)], 32) ["chrome://browser/content/browser.js":80]
    targetBrowser = undefined
    targetBrowserIndex = undefined
    this = [object ChromeWindow @ 0x8e44ca8 (native @ 0x8ffecf4)]
6 [native frame]

Not that useful....

In any case, this is pretty clearly not a Gecko bug.  And I see it on Linux too.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> with the reflow itself triggered by the following JS:
> 
> (gdb) jsstack 
> 0 [native frame]
> 1 isElementVisible(aElement = [object XULElement @ 0x8c1b218 (native @
> 0x8e278d8)]) ["chrome://browser/content/utilityOverlay.js":473]
>     bo = [object BoxObject @ 0x915eeb0 (native @ 0x9124478)]
>     this = [object ChromeWindow @ 0x8e44ca8 (native @ 0x8ffecf4)]
> 2 BrowserSearch_getSearchBar() ["chrome://browser/content/browser.js":2763]
>     searchBar = [object XULElement @ 0x8c1b218 (native @ 0x8e278d8)]
>     this = [object Object]

Bug 353071 added that code, but it was just refactoring existing code that has the same effect (trigger an early reflow), which explains why this is seen in 1.5 as well.

Comment 9

[Boris Zbarsky [:bzbarsky]]

No, no.  You misunderstand.  The actual reflow is triggered by this code.  What causes the reflow command to be posted is what matters, and I couldn't find it -- we post a HUGE number of reflow commands during browser window open...

And the problem is not "early reflow", but rather _late_ reflow.  Correct sizing of the content area depends on the browser chrome actually being in a stable state by the time the window's onload handler returns.  For Firefox, it appears that it's not.

Comment 10

[Robert Sayre]

I was able to make this pass by allowing scripts to hide the status bar and setting status=no in window.open, with and without the findbar included in browser.xul.

So, it seems we have some funky status bar XUL.

Comment 11

[Robert Sayre]

found it. taking.

Comment 12

[Robert Sayre]

Attachment: add an empty label attribute to #statusbar-display

I spotted this looking for differences between SeaMonkey's status bar and Firefox's. SeaMonkey actually has an entity in there, so maybe we want that.

I'm not going to pretend I understand why this works, but it does.

Comment 13

[Robert Sayre]

marking in-testsuite. covered by bug 344861 test.

http://lxr.mozilla.org/mozilla/source/testing/mochitest/tests/test_bug344861.html?raw=1

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 243807 [details] [diff] [review]
add an empty label attribute to #statusbar-display

(In reply to comment #12)
> I'm not going to pretend I understand why this works, but it does.

Since the statusbarpanel element essentially just wraps a label element that inherits its parent's "label" attribute, this boils down to the difference between:
<label value="" style="border:1px solid red;"/>
<label style="border:1px solid red;"/>

The anonymous label element initially has a height of 0, which is changed to a non-zero height when the progress listener first sets its value. Setting it's value to an empty string initially ensures that it always has the same height.

I don't really know whether a label with no "value" attribute really should have a height of 0 (i.e. whether the two elements above should differ), but that's certainly another bug, if it's a bug at all.

Comment 15

[Robert Sayre]

Checking in browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.325; previous revision: 1.324
done