Last Comment Bug 571970 - Main browser chrome should be hidden when viewing in-content UI
: Main browser chrome should be hidden when viewing in-content UI
Status: VERIFIED FIXED
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 6 votes (vote)
: Firefox 4.0b8
Assigned To: Dave Townsend [:mossop]
:
Mentors:
Depends on: 617099 597178 617076 617107 617325 617553 620064 620083 620138 625379 630951 638679
Blocks: FF2SM 617329 620963 544820 544821 616379 620956 629428 629437
  Show dependency treegraph
 
Reported: 2010-06-14 14:18 PDT by Dave Townsend [:mossop]
Modified: 2013-08-23 01:55 PDT (History)
48 users (show)
dtownsend: in‑testsuite+
dtownsend: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
patch rev 1 (4.59 KB, patch)
2010-08-16 20:45 PDT, Dave Townsend [:mossop]
no flags Details | Diff | Review
patch rev 2 (13.03 KB, patch)
2010-08-17 15:31 PDT, Dave Townsend [:mossop]
no flags Details | Diff | Review
Tabs on bottom screenshot (152.27 KB, image/png)
2010-09-20 16:44 PDT, Dave Townsend [:mossop]
no flags Details
Mockup: Current tabs-on-bottom problem and proposed solution (688.82 KB, image/png)
2010-09-21 17:24 PDT, Jennifer Morrow [:Boriss] (UX)
no flags Details
current WIP (10.80 KB, patch)
2010-09-21 17:36 PDT, Dave Townsend [:mossop]
no flags Details | Diff | Review
patch rev 3 (9.89 KB, patch)
2010-10-21 11:48 PDT, Dave Townsend [:mossop]
no flags Details | Diff | Review
patch rev 4 (9.46 KB, patch)
2010-11-24 12:55 PST, Dave Townsend [:mossop]
gavin.sharp: review+
Details | Diff | Review
fix broken test (1.46 KB, patch)
2010-12-03 16:00 PST, Dave Townsend [:mossop]
bmcbride: review+
Details | Diff | Review

Description Dave Townsend [:mossop] 2010-06-14 14:18:25 PDT
When viewing any in-content UI like the add-ons manager the main browser chrome (back/forward/stop/reload/location/search) should be hidden.
Comment 1 Brian King [:kinger] 2010-06-15 04:11:46 PDT
Why not disabled? Hiding will be somewhat confusing to users I think.
Comment 2 Peter Henkel [:Terepin] 2010-06-15 04:20:23 PDT
http://www.mozilla.sk/wp-content/uploads/2010/05/Firefox-4-Mockup-i06-Win7-Aero-InContentUI-AddonsManagerExtensionSelected.png
What's confusiong on that? Do you need nav buttons? No. Do you need adress bar? No. Do you need search bar? No. Do you need bookmraks bar? No. Do we need distinguish In-Content UI from regular tabs? Yes. Conclusion -> It's useless.
Comment 3 Henrik Skupin (:whimboo) 2010-06-15 05:13:31 PDT
While this is fine for tabs on top, how will it look like if you don't have this option set? Shouldn't be that easy then.
Comment 4 Peter Henkel [:Terepin] 2010-06-15 05:17:38 PDT
That's easy - In-Content UI is connected directly to tabs, therefore it's irelevant when tabs are.
Comment 5 Brian King [:kinger] 2010-06-15 09:32:44 PDT
(In reply to comment #2)
> http://www.mozilla.sk/wp-content/uploads/2010/05/Firefox-4-Mockup-i06-Win7-Aero-InContentUI-AddonsManagerExtensionSelected.png
> What's confusiong on that? Do you need nav buttons? No. Do you need adress bar?
> No. Do you need search bar? No. Do you need bookmraks bar? No. Do we need
> distinguish In-Content UI from regular tabs? Yes. Conclusion -> It's useless.

What is potentially confusing IMO is that the standard browser interface is changed dramatically which may give the appearance that you have been switched to another application.

What happens, for example, if someone wants to type in a url to go somewhere else when in the add-ons window?
Comment 6 Tony Mechelynck [:tonymec] 2010-06-21 19:29:34 PDT
- Do I need Back? Yes, to go to wherever I came from when I typed about:addons (or about: or about:config or whatever) in the URL bar.
- Do I need a URL bar? Yes, to go to some other page.
- Do I need bookmarks? Yes, to go to some other page.
- Do I need the Search bar? Yes, to go to some other page by means of a web search.
- Do I need Forward? Yes, if I go to some other page and come Back from there, I need Forward to return there.
- Do I need Reload? Maybe, maybe not. In about:config I need Reload to see the preference I just added - or maybe I could add one letter in the Filter bar. In about: which is essentially static, Reload is pointless but shouldn't harm. In about:addons, I hear there are bugs in the handling of the Reload button. Ideally, it should refresh the page, but until those bugs are ironed out (or if they are declared WONTFIX), I suppose greying out Reload would be OK, at least to temporarily hide said bugs...
- And in addition: Why should the whole browser chrome suddenly pop out of existence just because I want to make active the tab where I decided earlier to put about: or about:config or about:support or whatever? Just disable what becomes irrelevant.

IMHO what is proposed goes against: ux-control, ux-consistency, ux-discovery.
Comment 7 Peter Henkel [:Terepin] 2010-06-23 11:50:45 PDT
I was thinking. Why not use "AppTab" appereance? They will have their own icons and silver color, that will match in-content color. There will be no way to mistake them with normal tabs/AppTabs.
Comment 8 Dave Townsend [:mossop] 2010-08-16 20:45:27 PDT
Created attachment 466546 [details] [diff] [review]
patch rev 1

This makes browser.js look for a "disablechrome" attribute on the loaded document if it is an about: or chrome: url. When it is true and tabs are on top it hides the other toolbars and adds some styling that changes the tab background colour to match that of the in-content UI and removes the line between the selected tab and the content giving the impression that they are the same UI.
Comment 9 Justin Dolske [:Dolske] 2010-08-16 23:14:33 PDT
Comment on attachment 466546 [details] [diff] [review]
patch rev 1

So, one thing makes me a little bit wary... Some about: URIs can be accessed and modified by content (eg, opening a window for "about:blank" and then document.write()ing into it). I'm not sure how the timing of that interacts with when asyncUpdateUI() is called, though. [Eg, with a test script doing that I end up with a new tab with the URL == the invoking script's page, was it still "about:blank" at some point in-between?]

Maybe that's over paranoid? I can't think of a good way to sidestep the issue (by having a gBrowser.hideChrome() API, or setting a property on the <browser>) without adding more complexity/risk by having to watch for tab changes / navigation to explicitly reshow the chrome.
Comment 10 Dave Townsend [:mossop] 2010-08-17 15:31:40 PDT
Created attachment 466825 [details] [diff] [review]
patch rev 2

2nd version, adds checking that the page has privileges by comparing its document principal to the principal of browser.xul. Added a test verifying that the main window gets its attribute set and the navigation bar gets hidden when appropriate.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-08-21 18:27:39 PDT
Comment on attachment 466825 [details] [diff] [review]
patch rev 2

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>+#main-window[disablechrome] #navigator-toolbox[tabsontop="true"] > toolbar:not(#toolbar-menubar):not(#TabsToolbar) {
>+  display: none;
>+}

I think visibility: hidden is probably better (avoid destroying frames/bindings/etc.). Why does this only apply for tabsontop="true"?

Hiding things this way may have other implications that I'm not certain about. Dao may have some thoughts.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   asyncUpdateUI: function () {

>+    if (uri.schemeIs("about") || uri.schemeIs("chrome")) {

(["about", "chrome"].indexOf(uri.scheme) != -1), maybe? Multiple schemeIs calls actually have more xpconnect overhead for JS, not that it matters much.

>+      var docElement = gBrowser.selectedBrowser.contentDocument.documentElement;
>+      if (docElement) {
>+        if (document.nodePrincipal.equals(docElement.ownerDocument.nodePrincipal) &&
>+            docElement.getAttribute("disablechrome") == "true")

I think this is slightly clearer as:
let contentDocElem = content.document.documentElement;
let hasDisableChrome = contentDocElem && contentDocElem.getAttribute("disablechrome");
if (hasDisableChrome) {
  let systemPrincipal = document.nodePrincipal;
  let pagePrincipal = contentDocElem.nodePrincipal;
  if (pagePrincipal.equals(systemPrincipal)) {
    ...
  }
}

I wonder whether we should combine this with bug 569342 by having this code look for e.g. a disable="chrome,findbar" attribute, and setting that state on the <browser> so that e.g. the findbar can behave accordingly (without having to look for the attribute itself). Followup fodder perhaps.

>+    if (disableChrome)
>+      document.getElementById("main-window").setAttribute("disablechrome", "true");
>+    else
>+      document.getElementById("main-window").removeAttribute("disablechrome");

s/document.documentElement/document.getElementById("main-window")/ (applies to the test too).

Though after having looked at all this, I wonder whether it would be better to just set/remove the attribute from within the page itself (using pageshow/pagehide handlers).
Comment 12 Dave Townsend [:mossop] 2010-08-23 10:32:33 PDT
(In reply to comment #11)
> Comment on attachment 466825 [details] [diff] [review]
> patch rev 2
> 
> >diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css
> 
> >+#main-window[disablechrome] #navigator-toolbox[tabsontop="true"] > toolbar:not(#toolbar-menubar):not(#TabsToolbar) {
> >+  display: none;
> >+}
> 
> I think visibility: hidden is probably better (avoid destroying
> frames/bindings/etc.). Why does this only apply for tabsontop="true"?

It looked a little unnatural to me to be hiding this stuff when it is above the tab, that is easily switchable at UX's behest though.
> >+    if (disableChrome)
> >+      document.getElementById("main-window").setAttribute("disablechrome", "true");
> >+    else
> >+      document.getElementById("main-window").removeAttribute("disablechrome");
> 
> s/document.documentElement/document.getElementById("main-window")/ (applies to
> the test too).

I don't get it, the code there already uses getElementById.

> Though after having looked at all this, I wonder whether it would be better to
> just set/remove the attribute from within the page itself (using
> pageshow/pagehide handlers).

The page could set it on the browser element it is rendering in I guess, but that still means we'd have to hook up some other logic to make the toolbars show/hide since we wouldn't be able to use style rules based on the browser element.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-08-23 18:44:52 PDT
(In reply to comment #12)
> I don't get it, the code there already uses getElementById.

Er, I reversed the regex-replace. I meant "use document.documentElement".

> The page could set it on the browser element it is rendering in I guess, but
> that still means we'd have to hook up some other logic to make the toolbars
> show/hide since we wouldn't be able to use style rules based on the browser
> element.

I don't understand - the page can get the parent chrome window (using e.g. http://hg.mozilla.org/mozilla-central/annotate/f0af50ab4215/toolkit/components/console/hudservice/HUDService.jsm#l1563 ), and set the attribute on its documentElement.
Comment 14 Dave Townsend [:mossop] 2010-08-23 19:04:19 PDT
(In reply to comment #13)
> > The page could set it on the browser element it is rendering in I guess, but
> > that still means we'd have to hook up some other logic to make the toolbars
> > show/hide since we wouldn't be able to use style rules based on the browser
> > element.
> 
> I don't understand - the page can get the parent chrome window (using e.g.
> http://hg.mozilla.org/mozilla-central/annotate/f0af50ab4215/toolkit/components/console/hudservice/HUDService.jsm#l1563
> ), and set the attribute on its documentElement.

It could, but it would also have to change it whenever the tab changed, and as you say whenever the page changed. It seems pointless to copy that code into any page that we want to treat like this (and any extension pages too) when it can just be centralized. As the case I am interested in is toolkit code I also don't like the idea of it setting attributes on whatever window happens to include it.
Comment 15 Dão Gottwald [:dao] 2010-08-24 04:14:13 PDT
Rather than reaching into the documents, how about having a URI whitelist in browser?

I'd prefer if the theme changes were spun off to a separate bug.

(In reply to comment #12)
> > Why does this only apply for tabsontop="true"?
> 
> It looked a little unnatural to me to be hiding this stuff when it is above the
> tab, that is easily switchable at UX's behest though.

The tab bar jumping up and down when switching tabs would be pretty nasty, so this seems fine as is. This means that the hope that this would solve bug 553771 and bug 562797 is futile, though. Incidentally, keyboard shortcuts would still work as mentioned in these bugs and the menu bar would be accessible even with with tabs on top.

(In reply to comment #6)
> - Do I need Back? Yes, to go to wherever I came from when I typed about:addons
> (or about: or about:config or whatever) in the URL bar.

This worries me as well. Even with tabs on top, this isn't really the solution you want for bug 562797 if there was a meaningful session history before going to about:addons.
Comment 16 Dão Gottwald [:dao] 2010-08-24 05:20:11 PDT
Also need to test whether/how this affects full screen mode.
Comment 17 Dave Townsend [:mossop] 2010-08-24 08:12:01 PDT
Yeah this won't solve bug 553771 or bug 562797.
Comment 18 Justin Dolske [:Dolske] 2010-08-24 18:07:41 PDT
Comment on attachment 466825 [details] [diff] [review]
patch rev 2

I think we need a new patch here with the review comments from gavin/dao above. Perhaps gavin or dao should just do the review, too. :)
Comment 19 Csaba Kozák [:WonderCsabo] 2010-09-20 15:06:22 PDT
What's about this bug? It has a patch but no review for a month.

I wonder if it can make into Fx4. I think it would made Fx's UI much more practicable.
Comment 20 Dave Townsend [:mossop] 2010-09-20 15:12:28 PDT
(In reply to comment #19)
> What's about this bug? It has a patch but no review for a month.
> 
> I wonder if it can make into Fx4. I think it would made Fx's UI much more
> practicable.

It had a review and I'm trying to address the comments, but it is currently not a blocker so other things have priority.
Comment 21 Csaba Kozák [:WonderCsabo] 2010-09-20 15:19:22 PDT
I see. Is there any chance to make it a blocker again?

I know that Mozilla devs are working at 120% to ship Fx4... :(
Comment 22 Dave Townsend [:mossop] 2010-09-20 16:40:24 PDT
(In reply to comment #15)
> Rather than reaching into the documents, how about having a URI whitelist in
> browser?

This way allows extension provided pages to do the same easily and also saves us from needing to sync the list across different applications should they choose to go the same way.
Comment 23 Dave Townsend [:mossop] 2010-09-20 16:44:47 PDT
Created attachment 476959 [details]
Tabs on bottom screenshot

So the problem is that we either need to fix this bug or we need to fix bug 597178, but fixing this bug would really require doing it for both tabs-on-top and tabs-on-bottom cases and the tabs-on-bottom case sucks right now, firstly it looks bad (see the screenshot) and secondly changing to the add-ons manager tab makes the tab strip leap upwards so you can't click through tabs easily.
Comment 24 Csaba Kozák [:WonderCsabo] 2010-09-20 17:15:06 PDT
If you just fix the first bug, users will be confused.

They will access all Options, dialogs, etc in separate windows, except AOM. This is inconsistent. 

(Not speaking how much usable is in-content)
Comment 25 Dave Townsend [:mossop] 2010-09-20 17:17:08 PDT
(In reply to comment #24)
> If you just fix the first bug, users will be confused.
> 
> They will access all Options, dialogs, etc in separate windows, except AOM.
> This is inconsistent. 

This bug has nothing to do with moving any of those things to in-content UI. The only part of Firefox's UI that will be in-content in Firefox 4 is the add-ons manager.
Comment 26 Csaba Kozák [:WonderCsabo] 2010-09-20 17:20:55 PDT
Really? That's very, very said. :(

Peter Henkel stated in Bug 597178 comment 5 that Stephen said there will be in-content UI. This applies to AOM only?
Comment 27 Csaba Kozák [:WonderCsabo] 2010-09-20 17:22:36 PDT
Ah, i see "not in intended range". So that range is out from feature freeze. :(
Comment 28 Dão Gottwald [:dao] 2010-09-21 00:09:33 PDT
(In reply to comment #22)
> > Rather than reaching into the documents, how about having a URI whitelist in
> > browser?
> 
> This way allows extension provided pages to do the same easily and also saves
> us from needing to sync the list across different applications should they
> choose to go the same way.

How about using a pref branch? The add-on manager is toolkit, so it could set its pref in all.js for anyone to use it.

(In reply to comment #23)
> Created attachment 476959 [details]
> Tabs on bottom screenshot
> 
> So the problem is that we either need to fix this bug or we need to fix bug
> 597178, but fixing this bug would really require doing it for both tabs-on-top
> and tabs-on-bottom cases and the tabs-on-bottom case sucks right now

It really does -- I thought we agreed that we don't want this.
Comment 29 Dave Townsend [:mossop] 2010-09-21 09:32:15 PDT
(In reply to comment #28)
> (In reply to comment #22)
> > > Rather than reaching into the documents, how about having a URI whitelist in
> > > browser?
> > 
> > This way allows extension provided pages to do the same easily and also saves
> > us from needing to sync the list across different applications should they
> > choose to go the same way.
> 
> How about using a pref branch? The add-on manager is toolkit, so it could set
> its pref in all.js for anyone to use it.

Possible I guess. Is there something particularly wrong with just getting it off the document though?

> (In reply to comment #23)
> > Created attachment 476959 [details] [details]
> > Tabs on bottom screenshot
> > 
> > So the problem is that we either need to fix this bug or we need to fix bug
> > 597178, but fixing this bug would really require doing it for both tabs-on-top
> > and tabs-on-bottom cases and the tabs-on-bottom case sucks right now
> 
> It really does -- I thought we agreed that we don't want this.

UX still want to see it happen so they were going to try to figure out a solution for tabs-on-bottom. Boriss was meant to be commenting here yesterday.
Comment 30 Dão Gottwald [:dao] 2010-09-21 09:55:55 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #22)
> > > > Rather than reaching into the documents, how about having a URI whitelist in
> > > > browser?
> > > 
> > > This way allows extension provided pages to do the same easily and also saves
> > > us from needing to sync the list across different applications should they
> > > choose to go the same way.
> > 
> > How about using a pref branch? The add-on manager is toolkit, so it could set
> > its pref in all.js for anyone to use it.
> 
> Possible I guess. Is there something particularly wrong with just getting it
> off the document though?

I'm not sure how immediately available contentDocument.documentElement is really going to be on location change. I was also wondering about e10s, but then it occurred to me that in-content chrome will probably have to be in the chrome process anyway...
Comment 31 Peter Henkel [:Terepin] 2010-09-21 12:54:17 PDT
http://img830.imageshack.us/f/incontentui.jpg/
In-Content UI ROCKS!!! (Note: This can be done entirely with Stylish.)
Comment 32 Csaba Kozák [:WonderCsabo] 2010-09-21 13:20:21 PDT
With tabs on top, it's okay, but it's not decided how exactly it will look with tabs on bottom.
Comment 33 Jennifer Morrow [:Boriss] (UX) 2010-09-21 17:24:58 PDT
Created attachment 477354 [details]
Mockup: Current tabs-on-bottom problem and proposed solution

(In reply to comment #23)
> Created attachment 476959 [details]
> Tabs on bottom screenshot
>
> So the problem is that we either need to fix this bug or we need to fix bug
> 597178, but fixing this bug would really require doing it for both tabs-on-top
> and tabs-on-bottom cases and the tabs-on-bottom case sucks rght now, firstly
> it looks bad (see the screenshot) and secondly changing to the add-ons manager
> tab makes the tab strip leap upwards so you can't click through tabs easily.

Thanks for the screenshot, Dave... I'm having deja vu to first ffx 4 betas - yikes!  Not only does this look poor, but two other problems arise with tabs-on-bottom in this layout:

1. The benefit of coloring the tab in a way that's unspoofable is gone
2. The tabs actually move for the user from their place under the URL bar to the height of the navigation bar higher - extremely jarring!

The easy "fix" would be to just hide the URL bar if the user has tabs on the bottom.  This would treat in-content pages like any other page, with the added redundancy of two back/forward buttons and two search bars.  Unfortunately, this design gives us none of the benefits of having a clean, simpler in-content UI.  It also produces the strange inconsistency of making changing tab position also effect whether the URL and bookmark bar displays.

A better design would change the style of the navigation bar so that when the user has tabs on the bottom, the elements that don't display in tabs-on-top incontent UI are hidden and the background is changed to be similar to the background of the page.

Unfortunately, if the user had the bookmarks bar enabled, there would be extra vertical space at the top when the bookmarks bar was hidden in order to keep the tabs in the same position.  It's a compromise we can probably live with.

(see attached for the above proposed solution)
Comment 34 Dave Townsend [:mossop] 2010-09-21 17:36:44 PDT
Created attachment 477358 [details] [diff] [review]
current WIP

(In reply to comment #33)
It's a nice idea but I don't think that is something worth spending our time on for Firefox 4, we're going to need to revisit this after then. Attaching my current WIP that addresses all the comments so far.
Comment 35 Henrik Skupin (:whimboo) 2010-09-22 02:24:44 PDT
(In reply to comment #33)
> A better design would change the style of the navigation bar so that when the
> user has tabs on the bottom, the elements that don't display in tabs-on-top
> incontent UI are hidden and the background is changed to be similar to the
> background of the page.

How would that work if another theme or Persona is applied?

(In reply to comment #34)
> It's a nice idea but I don't think that is something worth spending our time on
> for Firefox 4, we're going to need to revisit this after then. Attaching my

Means we can set the target milestone to future and go forward with bug 597178 for Firefox 4?
Comment 36 Dave Townsend [:mossop] 2010-09-22 09:34:39 PDT
> (In reply to comment #34)
> > It's a nice idea but I don't think that is something worth spending our time on
> > for Firefox 4, we're going to need to revisit this after then. Attaching my
> 
> Means we can set the target milestone to future and go forward with bug 597178
> for Firefox 4?

I wouldn't set the target milestone to anything for now. I am working on a patch for bug 597178
Comment 37 Jennifer Morrow [:Boriss] (UX) 2010-09-22 12:58:13 PDT
(In reply to comment #34)
> Created attachment 477358 [details] [diff] [review]
> current WIP
> 
> (In reply to comment #33)
> It's a nice idea but I don't think that is something worth spending our time on
> for Firefox 4, we're going to need to revisit this after then. Attaching my
> current WIP that addresses all the comments so far.

Heh... somehow not shocking.  Revisiting later and fixing bug 597178 for now seems both wise and justice, within the Jedi code
Comment 38 Jennifer Morrow [:Boriss] (UX) 2010-10-05 16:57:37 PDT
(In reply to comment #37)
> (In reply to comment #34)
> > Created attachment 477358 [details] [diff] [review] [details]
> > current WIP
> > 
> > (In reply to comment #33)
> > It's a nice idea but I don't think that is something worth spending our time on
> > for Firefox 4, we're going to need to revisit this after then. Attaching my
> > current WIP that addresses all the comments so far.
> 
> Heh... somehow not shocking.  Revisiting later and fixing bug 597178 for now
> seems both wise and justice, within the Jedi code

I'm told that my last comment wasn't clear: I'm only referring to not moving ahead with styling *only* the tabs-on-bottom case, as seen in the attachment "Mockup: Current tabs-on-bottom problem and proposed solution."  I'm not in favor of leaving this bug unfinished for Firefox 4.
Comment 39 Dave Townsend [:mossop] 2010-10-07 15:25:34 PDT
Per discussions it has been decided that although we can only do this for the tabs-on-top case that in itself is enough of a win to make this worthwhile. Going to clean up the existing patch shortly.
Comment 40 Dave Townsend [:mossop] 2010-10-21 11:48:52 PDT
Created attachment 485093 [details] [diff] [review]
patch rev 3

This uses a simpler pref-based whitelist approach for the uris to display in-content. It doesn't include any shiny stylings, just the hiding of the toolbars for now. The changes only happen when tabs are on top.
Comment 41 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-23 14:11:10 PST
Comment on attachment 485093 [details] [diff] [review]
patch rev 3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   asyncUpdateUI: function () {

I think this is perhaps not the best place to put this code, since it runs twice for every normal page load (once off a setTimeout in onLocationChange, and once synchronously from the pageshow handler). We should probably fix that, though I'm a bit nervous about doing that now. We probably don't want to keep the pageShow invocation, but that would make the test more complicated since things would no longer happen synchronously...

I'd like to minimize the potential this has for impacting unrelated page loads. I think assigning gBrowser.currentURI.spec to a temporary variable and skipping the other checks would help with that. The restriction to about:/chrome: URIs doesn't seem particularly useful given that this can only be configured using a pref. I'm not sure the pref itself is useful either, really. Perhaps for now we should just hardcode that one URL?

>diff --git a/browser/base/content/test/browser_disablechrome.js b/browser/base/content/test/browser_disablechrome.js

>+var gOldTabsOnTop = TabsOnTop.enabled;
>+registerCleanupFunction(function() {
>+  TabsOnTop.enabled = gOldTabsOnTop;
>+});

We should generally avoid having tests that do work outside of test(), I think (seems like it could mess with error reporting).

>+function is_element_hidden(aElement) {

>+  if (aElement.ownerDocument == aElement.parentNode)
>+    return false;
>+
>+  return is_element_hidden(aElement.parentNode);

I find this much easier to understand with the logic flipped so that you have a (ownerDocument!=parentNode) check, and end the function with just |return false;|, for some reason.
Comment 42 Dão Gottwald [:dao] 2010-11-23 23:11:04 PST
Comment on attachment 485093 [details] [diff] [review]
patch rev 3

>+#main-window[disablechrome] #navigator-toolbox[tabsontop="true"] > toolbar:not(#toolbar-menubar):not(#TabsToolbar),
> #wrapper-urlbar-container #urlbar-container > #urlbar > toolbarbutton,
> #urlbar-container:not([combined]) > #urlbar > toolbarbutton,
> #urlbar-container[combined] + #reload-button + #stop-button,
> #urlbar-container[combined] + #reload-button,
> toolbar:not([mode="icons"]) > #urlbar-container > #urlbar > toolbarbutton,
> toolbar[mode="icons"] > #urlbar-container > #urlbar > #urlbar-reload-button:not([displaystop]) + #urlbar-stop-button,
> toolbar[mode="icons"] > #urlbar-container > #urlbar > #urlbar-reload-button[displaystop],
> toolbar[mode="icons"] > #reload-button:not([displaystop]) + #stop-button,

Can you add a separate rule for this? The one you're modifying is about the stop and reload buttons.
Comment 43 Robert Kaiser (not working on stability any more) 2010-11-24 07:41:10 PST
Hmm, is there a good way for an add-on to add an URL to the list of places to hide browser chrome? For example, my Tahoe Data Manager and Jökulsárlón Download Manager add-ons might want to do that, but there may be others out there. Would be good to have a plan in mind to make that work (doesn't necessarily need to be fully implemented right away though, but the current implementation should be extensible to enable that in the future).
Comment 44 Dave Townsend [:mossop] 2010-11-24 12:55:51 PST
Created attachment 493089 [details] [diff] [review]
patch rev 4

Updated from comments. I moved the check to onLocationChange so it only happens once. I also simplified it to a hardcoded array, extensions can still add themselves to this by overlaying browser.xul I guess.
Comment 45 Peter Henkel [:Terepin] 2010-11-30 02:59:45 PST
Shouldn't this block b8?
Comment 46 Dave Townsend [:mossop] 2010-11-30 10:07:38 PST
(In reply to comment #45)
> Shouldn't this block b8?

Why?
Comment 47 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-30 15:57:18 PST
Comment on attachment 493089 [details] [diff] [review]
patch rev 4

r=me with the firefox.js change omitted.
Comment 48 Dave Townsend [:mossop] 2010-12-02 16:09:42 PST
Landed: http://hg.mozilla.org/mozilla-central/rev/3f004b291c65
Comment 49 Robert Kaiser (not working on stability any more) 2010-12-02 18:00:26 PST
Is there at least a bug open on some way for an add-on to add itself to the whitelist for this (or some generic mechanism to get other in-content UI added to it)?
Comment 50 Dave Townsend [:mossop] 2010-12-02 18:23:44 PST
(In reply to comment #49)
> Is there at least a bug open on some way for an add-on to add itself to the
> whitelist for this (or some generic mechanism to get other in-content UI added
> to it)?

As I mentioned, extensions can add themselves to the whitelist, the simple script:

XULBrowserWindow.inContentWhitelist.push("about:foo");

should suffice.
Comment 51 Dave Townsend [:mossop] 2010-12-02 18:37:07 PST
Backed out due to test failures
Comment 52 Robert Kaiser (not working on stability any more) 2010-12-03 04:36:47 PST
(In reply to comment #50)
> XULBrowserWindow.inContentWhitelist.push("about:foo");
> 
> should suffice.

Cool, thanks, will try this when the main patch re-lands. I hope the MDC folks will pick this up as well (I hope it's correct to set this keyword right now). :)
Comment 53 Nochum Sossonko [:Natch] 2010-12-03 08:24:07 PST
With this patch, the status bar will suddenly show when I move from pane to pane in the Add-on Manager. Very strange interaction, STR:

1) Open Add-on Manager.
2) Open a blank tab (open, don't switch to)
3) Switch to the tab with the Add-on Manager
4) Change panes within the Add-on Manager (e.g. go from "Get Add-ons" to "Extensions").

Results in a large white bottom bar (actually larger than the statusbar) across the bottom of the page.
Comment 54 Dave Townsend [:mossop] 2010-12-03 16:00:19 PST
Created attachment 495160 [details] [diff] [review]
fix broken test

This test needs to cope with the main window having no back button visible.
Comment 55 Dave Townsend [:mossop] 2010-12-06 10:56:42 PST
Landed: http://hg.mozilla.org/mozilla-central/rev/9d0841ea7e3b
Comment 56 Nochum Sossonko [:Natch] 2010-12-06 11:06:24 PST
(In reply to comment #55)
> Landed: http://hg.mozilla.org/mozilla-central/rev/9d0841ea7e3b

Dave, did you not experience the errant behavior from comment 53?
Comment 57 Dave Townsend [:mossop] 2010-12-06 11:30:48 PST
(In reply to comment #56)
> (In reply to comment #55)
> > Landed: http://hg.mozilla.org/mozilla-central/rev/9d0841ea7e3b
> 
> Dave, did you not experience the errant behavior from comment 53?

Oh right, no I couldn't reproduce that. If you still can file a new bug for it and we'll figure it out there.
Comment 58 Nochum Sossonko [:Natch] 2010-12-06 11:39:23 PST
(In reply to comment #57)
> (In reply to comment #56)
> > (In reply to comment #55)
> > > Landed: http://hg.mozilla.org/mozilla-central/rev/9d0841ea7e3b
> > 
> > Dave, did you not experience the errant behavior from comment 53?
> 
> Oh right, no I couldn't reproduce that. If you still can file a new bug for it
> and we'll figure it out there.

Filed bug 617076 with more specific STR.
Comment 59 Reece H. Dunn 2010-12-12 03:47:20 PST
The navigation toolbar can reappear after loading the web page in the Get Add-ons pane -- see bug 618665 for STR.
Comment 60 Eric Shepherd [:sheppy] 2010-12-27 06:56:34 PST
Hm; I'm confused about what object the disablechrome attribute is on. Is it on the DOM document object? That's what I would assume, but there are bits in the code that make that not seem right (document.documentElement.getAttribute("disablechrome") in particular).
Comment 61 Dave Townsend [:mossop] 2010-12-27 08:15:23 PST
(In reply to comment #60)
> Hm; I'm confused about what object the disablechrome attribute is on. Is it on
> the DOM document object? That's what I would assume, but there are bits in the
> code that make that not seem right
> (document.documentElement.getAttribute("disablechrome") in particular).

It is on the root element in the document (<window>). The document itself can't have attributes.
Comment 63 Paul [pwd] 2010-12-27 08:59:48 PST
(In reply to comment #62)
> Now documented here:
> 
> https://developer.mozilla.org/en/XUL/Attribute/disablechrome
> 
> And mentioned:
> 
> https://developer.mozilla.org/en/XUL/window
> https://developer.mozilla.org/en/Firefox_4_for_developers#Miscellaneous_XUL_changes

Its incredibly short-winded, it has no context and thus is simply confusing. Can you add some context perhaps by adding a code example. Rather than people being forced to Google Code search. Also can you specify that it doesn't hide the Addon Bar. It would also be helpful to add that it doesn't hide anything if Tabs On Top isn't pref'd on.
Comment 64 Eric Shepherd [:sheppy] 2010-12-27 09:01:57 PST
For something that seems like almost nobody will use it, I'm not sure why this needs a ton of documentation. Feel free to add more if you think it's necessary, or correct me if I misunderstand the usefulness of this attribute. I don't think it's "confusing" though. The doc says what the attribute does.

That it doesn't hide the add-on bar is new information to me; I wasn't aware of that and will add a note to that effect.
Comment 65 Adam Kowalczyk 2010-12-27 12:09:21 PST
I think the existing description is fine. However, I think it would be useful to document how add-ons can add themselves to the whitelist of URIs for which Firefox automatically set the attribute (see comment #50). Actually, I think it it deserves a separate, more prominent note. Many extensions that use tabs for stuff other than regular web pages could take advantage of that, especially as Firefox itself moves towards in-content UI.
Comment 66 Paul [pwd] 2010-12-27 12:23:12 PST
Given that the documentation provides no more information than where it gets "mentioned". I'd say it's horribly under-documented. I'd hope our goals are to set higher standards of documentation.
Comment 67 Adam Kowalczyk 2010-12-27 12:44:58 PST
(In reply to comment #66)
> Given that the documentation provides no more information than where it gets
> "mentioned". I'd say it's horribly under-documented. I'd hope our goals are to
> set higher standards of documentation.

Looking at it from the perspective of an extension developer, I have no idea what you mean. The purpose of the attribute is clearly described - there isn't an awful lot to say about it. Perhaps you should say exactly what kind of information is missing.
Comment 68 Eric Shepherd [:sheppy] 2010-12-27 13:11:53 PST
Are you sure you actually looked at the doc?

https://developer.mozilla.org/en/XUL/Attribute/disablechrome

This page explains what the attribute means. There's not much more to it. :)
Comment 69 Robert Kaiser (not working on stability any more) 2010-12-27 16:39:44 PST
Paul, please don't more or less call doc writers stupid or otherwise just rant negatively, but point out what should be improved instead. That way, it's much easier to get what you actually want. Try to be constructive, and constructing will be easier for everyone.

Adam, thanks for pointing out in comment #65 what the doc problem is and being friendly overall.

Sheppy, please look at comment #65 and follow to comment #50 and the question I had there and its answer, that would be good to have documented, as it's precisely what add-on authors trying to do in-tab UI will want to know.
Comment 70 Paul [pwd] 2010-12-27 16:48:25 PST
(In reply to comment #69)
> Paul, please don't more or less call doc writers stupid or otherwise just rant
> negatively, but point out what should be improved instead. That way, it's much
> easier to get what you actually want. Try to be constructive, and constructing
> will be easier for everyone.
> 
> Adam, thanks for pointing out in comment #65 what the doc problem is and being
> friendly overall.
> 
> Sheppy, please look at comment #65 and follow to comment #50 and the question I
> had there and its answer, that would be good to have documented, as it's
> precisely what add-on authors trying to do in-tab UI will want to know.

Excuse me, but I did post comment 63 before posting comment 66. In comment 63 I added constructive criticism of the doc and shortly after the "NOTE" was added. I will agree that I should've left it at that.

I'd also like to state that in no way did I impart any personal criticism on anyone. I simply looked at the dev docs that were posted and posted my opinion as someone that has been forced to do a Google Code Search just to get some context as to a dev doc of similar effort in the past.

I apologise if I offended anyone. I didn't mean to be rude. I'm really sorry.
Comment 71 Henrik Skupin (:whimboo) 2011-01-09 17:07:55 PST
Marking as verified fixed, now that the code has been stabilized in the last couple of weeks. Remaining issues will be handled on follow-up bugs.

Verified fixed with builds on all platforms like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre ID:20110109030350

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