Closed Bug 571970 Opened 14 years ago Closed 14 years ago

Main browser chrome should be hidden when viewing in-content UI

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mossop, Assigned: mossop)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 4 obsolete files)

When viewing any in-content UI like the add-ons manager the main browser chrome (back/forward/stop/reload/location/search) should be hidden.
blocking2.0: --- → ?
Why not disabled? Hiding will be somewhat confusing to users I think.
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.
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.
That's easy - In-Content UI is connected directly to tabs, therefore it's irelevant when tabs are.
(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?
- 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.
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.
blocking2.0: ? → -
Keywords: meta
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #466546 - Flags: review?(dolske)
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.
Attachment #466546 - Flags: feedback?(gavin.sharp)
Attached patch patch rev 2 (obsolete) — Splinter Review
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.
Attachment #466546 - Attachment is obsolete: true
Attachment #466825 - Flags: review?(dolske)
Attachment #466825 - Flags: feedback?(gavin.sharp)
Attachment #466546 - Flags: review?(dolske)
Attachment #466546 - Flags: feedback?(gavin.sharp)
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).
Attachment #466825 - Flags: feedback?(gavin.sharp)
(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.
(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.
(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.
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.
Also need to test whether/how this affects full screen mode.
Yeah this won't solve bug 553771 or bug 562797.
No longer blocks: 553771, 562797
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. :)
Attachment #466825 - Flags: review?(dolske)
Depends on: 597178
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.
(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.
I see. Is there any chance to make it a blocker again?

I know that Mozilla devs are working at 120% to ship Fx4... :(
(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.
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.
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)
(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.
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?
Ah, i see "not in intended range". So that range is out from feature freeze. :(
(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.
(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.
Keywords: uiwanted
(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...
http://img830.imageshack.us/f/incontentui.jpg/
In-Content UI ROCKS!!! (Note: This can be done entirely with Stylish.)
With tabs on top, it's okay, but it's not decided how exactly it will look with tabs on bottom.
(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)
Attached patch current WIP (obsolete) — Splinter Review
(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.
Attachment #466825 - Attachment is obsolete: true
Assignee: dtownsend → nobody
(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?
> (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
(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
(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.
blocking2.0: - → betaN+
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.
Assignee: nobody → dtownsend
Attached patch patch rev 3 (obsolete) — Splinter Review
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.
Attachment #477358 - Attachment is obsolete: true
Attachment #485093 - Flags: review?
Keywords: uiwanted
Whiteboard: [has patch][needs review gavin]
Attachment #485093 - Flags: review? → review?(gavin.sharp)
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 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.
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).
Whiteboard: [has patch][needs review gavin] → [has patch]
Attached patch patch rev 4Splinter Review
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.
Attachment #485093 - Attachment is obsolete: true
Attachment #493089 - Flags: review?(gavin.sharp)
Attachment #485093 - Flags: review?(gavin.sharp)
Whiteboard: [has patch] → [has patch][needs review gavin]
Shouldn't this block b8?
(In reply to comment #45)
> Shouldn't this block b8?

Why?
Comment on attachment 493089 [details] [diff] [review]
patch rev 4

r=me with the firefox.js change omitted.
Attachment #493089 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][needs review gavin] → [has patch]
Landed: http://hg.mozilla.org/mozilla-central/rev/3f004b291c65
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → Firefox 4.0b8
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)?
(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.
Backed out due to test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 616379
(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). :)
Keywords: dev-doc-needed
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.
Attached patch fix broken testSplinter Review
This test needs to cope with the main window having no back button visible.
Attachment #495160 - Flags: review?(bmcbride)
Attachment #495160 - Flags: review?(bmcbride) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/9d0841ea7e3b
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
(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?
(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.
Depends on: 617076
(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.
Depends on: 617099
Depends on: 617325
Depends on: 617362
No longer depends on: 617362
The navigation toolbar can reappear after loading the web page in the Get Add-ons pane -- see bug 618665 for STR.
Depends on: 620083
Blocks: 620138
No longer blocks: 620138
Depends on: 620138
Blocks: 620963
Blocks: 620956
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).
(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.
(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.
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.
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.
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.
(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.
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. :)
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.
(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.
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
Status: RESOLVED → VERIFIED
Depends on: 625379
Depends on: 617107
Blocks: 629428
Blocks: 629437
Depends on: 630951
You need to log in before you can comment on or make changes to this bug.