528406 - XUL files opened via Browser.addTab do not display

Status: VERIFIED FIXED

Description

[Christopher Finke]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5
Build Identifier: 

I've found that XUL files opened via Browser.addTab do not display, although opening them by manually typing the URL in the location bar displays them properly.

Reproducible: Always

Steps to Reproduce:
1. Add a button via an extension (or whatever method).
2. Have it run "Browser.addTab('about:config');" oncommand.
3. Click it.
Actual Results:  
A blank tab opens, albeit with the correct title.

Expected Results:  
The XUL file should open in the tab.

Comment 1

[Christopher Finke]

Created attachment 412122 [details]
An extension that adds a button to the right toolbar that exhibits the bug behavior.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Not sure why viewportRect.width && viewportRect.height stay both at 1 pixel.

Others people has confirmed it too.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Ok. we are broken since changeset 1060:3d8f7d7b2fc7

Comment 4

[Antoine Turmel [:GeekShadow]]

Got this bug too.

Comment 5

[testit]

This problem seems not to be specific for XUL files. It also occurs, if you open a new tab using an URI pointing to a simple HTML file.

Comment 6

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

I'm getting reports from add-on developers that this bug is a serious problem

Comment 7

[Antoine Turmel [:GeekShadow]]

Yep it's a blocking bug for my SamplerBox add-on :/

Comment 8

[Christopher Finke]

(In reply to comment #7)
> Yep it's a blocking bug for my SamplerBox add-on :/

Ditto for ScribeFire.

Comment 9

[Benjamin Stover (:stechz)]

1 pixel viewport makes me think the browser is never getting the proper scroll size events (viewport starts out as 0,0,1,1).  The background patch listens for all size change events, but Vivian and I have confirmed this is still occurring.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Apparently this come because MozScrolledAreaChanged is not fired for Xul documents.

froystig is working on that (don't have a bug number yet)

Comment 11

[Fabrice Desré]

Here's a workaround : 

function loadXUL(uri) {
  Browser.addTab('about:blank', true);
  window.setTimeout(function() {
         Browser.selectedBrowser.loadURI(uri, null, null)}, 0);
}

Comment 12

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

What if we use BrowserUI.newTab? Does that work better?

(In reply to comment #11)
> Here's a workaround : 
> 
> function loadXUL(uri) {
>   Browser.addTab('about:blank', true);
>   window.setTimeout(function() {
>          Browser.selectedBrowser.loadURI(uri, null, null)}, 0);
> }

This implies a timing issue of some kind.

Comment 13

[Christopher Finke]

(In reply to comment #12)
> What if we use BrowserUI.newTab? Does that work better?

No; BrowserUI.newTab has the same results as Browser.addTab for the test case.

Comment 14

[Benjamin Stover (:stechz)]

(In reply to comment #11)
> Here's a workaround : 
> 
> function loadXUL(uri) {
>   Browser.addTab('about:blank', true);
>   window.setTimeout(function() {
>          Browser.selectedBrowser.loadURI(uri, null, null)}, 0);
> }

This works because about:blank receives a scrollarea change event.  about:config or apparently any XUL files do not get the event.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Created attachment 415867 [details] [diff] [review]
Patch

Patch for handling xul document by creating a fake MozScrolledAreaChanged event and by using our old getBrowserDimensions method.

Why do we need this?

Because the bounds of a xul doc didn't change during reflow and so we don't fire the MozScrolledAreaChanged event (http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#896).

Also this code (for fire the event) belongs to nsHTMLScrollFrame not a nsXULScrollFrame for xul documents, I'm not sure why but i've fall in the trap and lose some times on that. (if someone know the answer?)

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Created attachment 415868 [details] [diff] [review]
Patch

The same without my changes to browser.xul

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Created attachment 415884 [details] [diff] [review]
Patch v0.2

Comment 18

[Benjamin Stover (:stechz)]

Comment on attachment 415884 [details] [diff] [review]
Patch v0.2

>   createBrowserViewportState: function createBrowserViewportState() {
>-    return new BrowserView.BrowserViewportState(new Rect(0, 0, 1, 1), 0, 0, 1);
>+    return new BrowserView.BrowserViewportState(new Rect(0, 0, kDefaultBrowserWidth, window.innerHeight), 0, 0, 1);
>   },

Can't just use window.innerHeight, calculate the height like resize handler does.  Also, innerWidth and innerHeight are 0 when Fennec loads.  There's some assumptions in TileManager about the width and height being at least 1, so check for this.

>   getViewportStateFromBrowser: function getViewportStateFromBrowser(browser) {
>     return browser.__BrowserView__vps;
>   },

Forgot about this function.  Its purpose is long gone, let's get rid of it sometime.

>+
>+      if (browser.contentDocument instanceof XULDocument && Browser.selectedBrowser == browser)
>+        Browser._browserView.zoomToPage();

Is this to fix the zoom problem for XUL pages?  It didn't do the job for me.  In fact, I can still scroll around as if the page has the dimensions of the old page.

It looks like MozScrollAreaChange is not fired even when moving from an HTML page to a XUL. :( If this is true, maybe your secondmost recent patch is the only way to go.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

> >+
> >+      if (browser.contentDocument instanceof XULDocument && Browser.selectedBrowser == browser)
> >+        Browser._browserView.zoomToPage();
> 
> Is this to fix the zoom problem for XUL pages?  It didn't do the job for me. 
> In fact, I can still scroll around as if the page has the dimensions of the old
> page.

Weird, i should have done something wrong between my test and the patch :/

> 
> It looks like MozScrollAreaChange is not fired even when moving from an HTML
> page to a XUL. :( If this is true, maybe your secondmost recent patch is the
> only way to go.

This is true. MozScrolledAreaChanged is never fired for xul docs (or at least for all the cases i've test.). but my second patch was supposed to made the stuff works - I'll do another iteration tomorrow to see what's wrong

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 415868 [details] [diff] [review]
Patch

I'm not sure how the previous patch i have made could work since i didn't updating the viewport, i'm not sure why it has work during my test, i've probably do some weird stuff...

So, yeah, this patch is probably the way to go. It has also an other benefits which is addons developers can listen for MozScrolledAreadChanged vent and react to this - so finally doing that is probably not so bad.

Comment 21

[Benjamin Stover (:stechz)]

Comment on attachment 415868 [details] [diff] [review]
Patch

Considering the above, looks good to me.

Comment 22

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mobile-browser/rev/03cfe0e88a8d

Comment 23

[Antoine Turmel [:GeekShadow]]

great :)

Comment 24

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091214 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091214 Firefox/3.7a1pre Fennec/1.0b5