Closed Bug 596371 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b7

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v.1 (obsolete) — Splinter Review
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.
Attachment #475213 - Flags: review?(gavin.sharp)
Attached patch Patch v.2Splinter Review
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?
Attachment #475213 - Attachment is obsolete: true
Attachment #475738 - Flags: review?(gavin.sharp)
Attachment #475213 - Flags: review?(gavin.sharp)
Oh, actually, I think maybe we should, but instead as browser.linkedTabPanel?
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?
(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.
Huh? The <notificationbox>s aren't anonymous nodes.
Attached patch linkedTabPanel getter (obsolete) — Splinter Review
To be clear, something like this is what I was suggesting.
The look very anonymous over here. See also bug 508819.
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...
Attachment #475784 - Attachment is obsolete: true
(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)?
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 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?
Attachment #475738 - Flags: review?(gavin.sharp) → review+
Duplicate of this bug: 313190
(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.
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
> 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.
Depends on: 624084
You need to log in before you can comment on or make changes to this bug.