596371 - Put <browser> in a <stack> so it's easy to overlay.

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Now that bug 130078 is fixed, chrome can put stuff over content. For tab-modal prompts, I need to overlay the entire <browser> with a prompt that blocks interaction with the page. This would be easiest if the browser was in a <stack>, so here we go.

This is based on the work Frank did in bug 583462. Currently running through tryserver to see if any tests fail because they make assumptions about the DOM that are being changed here.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Changes to some tests and code (HUDService), which depened on the old DOM structure.

Dunno if it's worth adding an API to go from the <browser> to the <notificationbox> instead of using .parentNode.parentNode? Maybe?

Comment 2

[Justin Dolske [:Dolske]]

Oh, actually, I think maybe we should, but instead as browser.linkedTabPanel?

Comment 3

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

We should add a property to browsers that links to their tabpanels. Don't really want it to be a strong reference, though, and isn't something that we can enforce (since browsers aren't all necessarily contained within tabpanels). Perhaps a "linkedTabPanel" property that returns document.getElementById(this.getAttribute("linkedpanelid")), and have the tabbrowser code set linkedpanelid attribute on the browser when it creates it?

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> Perhaps a "linkedTabPanel" property that returns
> document.getElementById(this.getAttribute("linkedpanelid"))

Those are anonymous nodes as far as tabbrowser is concerned.

Comment 5

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

Huh? The <notificationbox>s aren't anonymous nodes.

Comment 6

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

Attachment: linkedTabPanel getter

To be clear, something like this is what I was suggesting.

Comment 7

[Dão Gottwald [:dao]]

The look very anonymous over here. See also bug 508819.

Comment 8

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

Bah, I thought they were <content>-ed into tabbrowser (and that I was looking at browser.xul). Time to sleep, I guess.

We could still set .linkedPanelId on the browser, and have a getPanelForBrowser() helper on tabbrowser, I guess. Would be nice to nicely expose the <stack> itself somehow too, but having many "weak up references" is starting to get messy...

Comment 9

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

(In reply to comment #7)> The look very anonymous over here. See also bug 508819.Actually, it looks like the patch for bug 508819 that made getElementById() fail to return anonymous nodes was backed out (see comments in bug 522030).Now there's some HUDService code that depends on that behavior: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm#4608bz: should we avoid depending on getElementById returning anonymous nodes (and fix the existing places where we do)?

Comment 10

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

Gah, bug 596752 :( Posting again:

(In reply to comment #7)
> The look very anonymous over here. See also bug 508819.

Actually, it looks like the patch for bug 508819 that made
getElementById() fail to return anonymous nodes was backed out (see comments in
bug 522030).

Now there's some HUDService code that depends on that behavior:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm#4608

bz: should we avoid depending on getElementById returning anonymous nodes (and fix the existing places where we do)?

Comment 11

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

Comment on attachment 475738 [details] [diff] [review]
Patch v.2

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>-            b.setAttribute("flex", "1");

I don't think you want to remove this?

How about adding a gBrowser.getPanelForBrowser helper (it can just call getNotificationBox, or vice-versa) and making use of it in the places you're using .parentNode.parentNode?

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>   getContentWindowFromHUDId: function HS_getContentWindowFromHUDId(aHUDId)

>+      var node = nodes[i];
>+
>+      if (node.localName == "stack" &&
>+          node.firstChild &&
>+          node.firstChild.contentWindow) {
>+        return node.firstChild.contentWindow;

so ugly :( "hud" objects should really have direct references to their associated <browser>. The whole HUD object hierarchy is horribly confusing though. File a followup?

Comment 13

[Justin Dolske [:Dolske]]

(In reply to comment #11)

> >-            b.setAttribute("flex", "1");
> 
> I don't think you want to remove this?

I did mean to, although it's harmless. I was reminded of bug 462113 comment 23.

Comment 14

[Justin Dolske [:Dolske]]

Per IRC, decided to skip adding getPanelForBrowser() for now, it only helped for these couple of cases, and overall felt kind of meh. Can always add it later if we think there's a good reason for it.

Filed bug 598270 on the HUD-<browser> linkage/cleanup.

Pushed http://hg.mozilla.org/mozilla-central/rev/e7fd6c393c98

Comment 15

[Boris Zbarsky [:bzbarsky]]

> bz: should we avoid depending on getElementById returning anonymous nodes 

Yes.  Both sicking and I have patches to make it not do that; we just didn't get to land them in time for 2.0.  This stuff _will_ break.  Don't rely on it.