Last Comment Bug 596371 - Put <browser> in a <stack> so it's easy to overlay.
: Put <browser> in a <stack> so it's easy to overlay.
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 4.0b7
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
Depends on: 624084
Blocks: 59314
  Show dependency treegraph
 
Reported: 2010-09-14 14:03 PDT by Justin Dolske [:Dolske]
Modified: 2013-11-12 00:57 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (3.67 KB, patch)
2010-09-14 14:03 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2 (10.78 KB, patch)
2010-09-15 20:01 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
gavin.sharp: approval2.0+
Details | Diff | Splinter Review
linkedTabPanel getter (2.76 KB, patch)
2010-09-16 01:12 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2010-09-14 14:03:02 PDT
Created attachment 475213 [details] [diff] [review]
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] 2010-09-15 20:01:43 PDT
Created attachment 475738 [details] [diff] [review]
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] 2010-09-15 20:02:15 PDT
Oh, actually, I think maybe we should, but instead as browser.linkedTabPanel?
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-09-15 23:43:40 PDT
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] 2010-09-15 23:59:03 PDT
(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: gavin@gavinsharp.com] 2010-09-16 01:11:20 PDT
Huh? The <notificationbox>s aren't anonymous nodes.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-09-16 01:12:07 PDT
Created attachment 475784 [details] [diff] [review]
linkedTabPanel getter

To be clear, something like this is what I was suggesting.
Comment 7 Dão Gottwald [:dao] 2010-09-16 01:18:50 PDT
The look very anonymous over here. See also bug 508819.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-09-16 01:31:32 PDT
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: gavin@gavinsharp.com] 2010-09-16 17:05:08 PDT
(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: gavin@gavinsharp.com] 2010-09-16 17:13:21 PDT
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: gavin@gavinsharp.com] 2010-09-16 18:20:09 PDT
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 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-09-17 12:53:31 PDT
*** Bug 313190 has been marked as a duplicate of this bug. ***
Comment 13 Justin Dolske [:Dolske] 2010-09-20 18:12:29 PDT
(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] 2010-09-21 00:23:10 PDT
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 [:bz] (still a bit busy) 2010-09-22 09:47:00 PDT
> 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.

Note You need to log in before you can comment on or make changes to this bug.