Closed Bug 752434 Opened 12 years ago Closed 11 years ago

Stop hiding toolbars for about:addons & Co.

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: fb+mozdev, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-needed, ux-consistency)

Attachments

(1 file, 5 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0) Gecko/20100101 Firefox/13.0
Build ID: 20120501201020

Steps to reproduce:

Open about:addons.


Actual results:

Navigation bar (awesome bar) is hidden. 


Expected results:

Navigation bar is visible with meaningful URL. 

Chrome does this very cleverly: They provide an actual URL for any of their special pages (e. g. preferences). This helps orientate yourself where you are as well as provides direct access to certain configuration pages. With this direct access, you can send anyone a direct link to specific configuration pages thus eliminating the need to explain how to get to a configuration option when you need someone to change it. 

Also, I often find myself navigating to about:addons (e. g. enable/disable an extension or plugin), then navigating to a Website requires me to close the current tab and open another one and then typing in the address, instead of just being able to input the address directly inside the navigation bar. 

As a security consideration, you could prevent Interweb Referrers thus only allowing typing in "about:addons/extensions/Firebug" or "about:settings/advanced/encryption" et al. directly into the navigation bar (instead of allowing direct links form Websites to the special pages). 
Furthermore, the awesome bar could by painted with a Firefox-y orange/blue background to distinguish fake phishing special sites from the real special sites.
Keywords: ux-efficiency
OS: Mac OS X → All
Hardware: x86 → All
Depends on: 752447
Component: Untriaged → Location Bar
Severity: normal → enhancement
This isn't really ux-efficiency.

It isn't wontfix either. I think that, especially with the new Australis customization flow, there will be more compelling reasons to have the navigation toolbar always visible in the default UI configuration.

So, please don't spam this bug with more reasons to do this. The description covers it well, and we're aware of it.
Severity: enhancement → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: untriaged → location.bar
Whiteboard: DUPEME
Version: 13 Branch → Trunk
No longer depends on: 752447
(In reply to Frank Yan (:fryn) from comment #1)
> This isn't really ux-efficiency.
> 
> It isn't wontfix either. I think that, especially with the new Australis
> customization flow, there will be more compelling reasons to have the
> navigation toolbar always visible in the default UI configuration.

Also, the fact that Australis wants the Firefox menu button on the navigation toolbar.
Assignee: nobody → dao
Component: Location Bar → Toolbars
QA Contact: location.bar → toolbars
Whiteboard: DUPEME
Summary: always show navigation bar / awesome bar (e. g. about:addons) → Stop hiding toolbars for about:addons & Co.
Attached patch patch (obsolete) — Splinter Review
Attachment #629999 - Flags: ui-review?(shorlander)
I don't understand the reasoning behind doing this. Showing that toolbar shows a lot of extra UI cruft that's not relevant to in-content UI. And it doesn't remove the need for a toolbar in many of the in-content pages (Add-ons Manager and Preferences need a search bar, Add-ons Manager needs a menubutton and a notification area, for instance).

The navigation to a special URL as described in comment 0 is unrelated to hiding/showing the toolbar, and can be done without showing the URL bar once the page is loaded.

And it feels very wrong to show all that extra UI if it's only because the Firefox menu button is being moved there. There are other ways around that (eg, overlaying it into the page's toolbar) if that's the real issue.
Comment on attachment 629999 [details] [diff] [review]
patch

(In reply to Blair McBride (:Unfocused) from comment #4)

Not being able to navigate easily a tab away from the Add-ons Manager / Preferences, etc. without closing it is annoying and unnecessarily prevented.

Requiring a search bar, etc. in the Add-ons Manager doesn't really have anything to do with this bug.

The navigation toolbar elements, including back/forward, should be visually consistent across every page, and they aren't for the Add-ons Manager at the moment. We shouldn't do partial overlays that confuse the visual hierarchy even further.

With the new Australis customization UI flow (panel, etc.), we're going to make it easy for users to customize the region between the URL bar and the menu button, and it doesn't make sense to hide all that while the Add-ons Manager / Preferences, etc. are being displayed to the user.
Attachment #629999 - Flags: feedback+
(In reply to Frank Yan (:fryn) from comment #5)
> Requiring a search bar, etc. in the Add-ons Manager doesn't really have
> anything to do with this bug.

It does when we're adding additional noise to the UI.

> The navigation toolbar elements, including back/forward, should be visually
> consistent across every page, and they aren't for the Add-ons Manager at the
> moment.

I had been working towards fixing that in bug 630682 and Dao shot it down.

> With the new Australis customization UI flow (panel, etc.), we're going to
> make it easy for users to customize the region between the URL bar and the
> menu button, and it doesn't make sense to hide all that while the Add-ons
> Manager / Preferences, etc. are being displayed to the user.

I was under the impression that the UI for customizing that would open a separate in-content UI (ie, not the Add-ons Manager or Preferences).
Comment on attachment 629999 [details] [diff] [review]
patch

Review of attachment 629999 [details] [diff] [review]:
-----------------------------------------------------------------

We may want to eventually do this. Before we do though we need a complete design on how exactly we want to handle incontent UI.
Attachment #629999 - Flags: ui-review?(shorlander) → ui-review-
(In reply to Stephen Horlander from comment #8)
> Before we do though we need a complete
> design on how exactly we want to handle incontent UI.

What is your opinion on the way Chrome does ist (every in-content UI page has a meaningful and easy URI displayed in the address bar)? I'd like to see this solution in Fx as well. 

Or what does your question refer to?
This has not been an issue in SeaMonkey. Why can't you reimport the code? Has development on the two projects diverged too greatly?

As a second thought, can someone explain why the projects separated in the first place? It seems logical to separate the components of the suite so compiling them together would make the suite and compiling them separately would create applications branded with component names. This would permit one stream of add-on components compatible with the suite and individual applications. I understand that it is too late to go back without a major overhaul. Respond to me personally to this second issue, since it is off-topic. You may contact me through my Mozilla profile or by finding my email address(It contains my name.) in the CC list.
(In reply to Blair McBride (:Unfocused) from comment #4)

Well, in fact you want to reinvent the dialog box. Prefs & company belong to dialog boxes. Moving config UI from dialogs to pages is really *a bad idea*. Not everything Chrome does is worth copying it.
Assignee: dao → nobody
I don't think landing Australis new menu panel is possible without this since average users could for example get stuck in the add-ons manager (only way to get out would be the menu bar).
Hiding of the navbar has recently hindered or caused the following bugs:
* Bug 858089 - Australis tab stroke polish - the space for the box-shadow on the top of the nav-bar is shown on about:addons without a fill since the navbar is hidden
* Bug 851009 - Australis tab separator layering required changing the border between the tabs and navbar
* Bug 770135 - Australis app menu
** The app menu normally only exists on the navbar.  Hiding the navbar on pages like these mean that there is no app button. Moving the app button into content in this case adds complexity and another configuration to test for little benefit IMO.
* Bug 853415 - Hiding and showing the chrome causes overflow to be recalculated.
** This is likely a platform bug that should be fixed anyways but I think this is the only place we are currently affected.

It seems like the only benefit mentioned is reducing UI clutter on these few pages but I would argue that consistency for users and developers outweigh that.

As mentioned in comment 5, the new customization UI will require showing the nav-bar to allow for customizing it.  That UI was originally going to include customizing with themes and it makes sense to show the UI as it would normally be seen when previewing a theme.

For non-customization in-content pages, I care less about hiding 3rd-party toolbars if the UI cruft is a big issue, but I don't think we should hide the nav-bar.
This is the previous patch, unbitrotted, but it also removes our new addition of the overlay UI that'd show the menu button inside the screen. From a cursory glance over the code that introduced that, I believe we also adapted the CSS for some of those pages. Does that need to be undone? If so, I can add another patch to do that bit, too.
Assignee: nobody → gijskruitbosch+bugs
Attachment #629999 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #749269 - Flags: review?(jaws)
I should have said, we're picking this back up after a discussion in the Australis meeting last Thursday, where we discussed with UX and they gave us the go-ahead to do this after all.
Comment on attachment 749269 [details] [diff] [review]
Unbitrotted patch, also remove the UX overlays for the menubutton

Egh, missed some bits...
Attachment #749269 - Attachment is obsolete: true
Attachment #749269 - Flags: review?(jaws)
Attached patch Patch (obsolete) — Splinter Review
I'm still seeing unhappiness from about:addons about a missing overlay with this patch, but I don't understand why...
Comment on attachment 749282 [details] [diff] [review]
Patch

(In reply to :Gijs Kruitbosch from comment #18)
> Created attachment 749282 [details] [diff] [review]
> Patch
> 
> I'm still seeing unhappiness from about:addons about a missing overlay with
> this patch, but I don't understand why...

Unhappiness being errors in the error console. If we cared about that, we'd need to touch the clobber file and request clobbers. As it is, I don't expect this to cause any (other) tests to fail... and we clobber often enough (nightlies clobber by default) that it shouldn't matter, IMHO.
Attachment #749282 - Flags: review?(mconley)
Comment on attachment 749282 [details] [diff] [review]
Patch

Review of attachment 749282 [details] [diff] [review]:
-----------------------------------------------------------------

I guess we don't need to update OSX's inContentUI.css?

Besides the one-nit I found, this looks good to me. You might want Dao to r+ the toolkit/* stuff.

::: browser/base/content/browser.js
@@ -3749,5 @@
>          SocialMark.updateMarkState();
>        }
>  
> -      // Show or hide browser chrome based on the whitelist
> -      if (this.hideChromeForLocation(location)) {

This was the only usage of hideChromeForLocation, which is a function that relies on inContentWhitelist. We should remove that function too.
Attachment #749282 - Flags: review?(mconley) → review+
Attached patch Patch (obsolete) — Splinter Review
Good point, I missed a conflict there when unbitrotting dao's original patch and then for some reason did not find the function afterwards. I've removed it in this version of the patch.

I'm going to be honest and say I don't even know *why* we're adjusting the background gradients on in-content pages, but these changes were in the original patch that dao put up here, so I've unbitrotted them across. There is also a gradient in the OS X version of inContentUI.css, and whether that needs updating I'll leave up to Dao, indeed. Thanks! :-)
Attachment #749282 - Attachment is obsolete: true
Attachment #749297 - Flags: review?(dao)
Comment on attachment 749297 [details] [diff] [review]
Patch

You missed a spot in toolkit/themes/windows/global/inContentUI.css and you didn't remove browser_disablechrome.js and disablechrome.html.
Attachment #749297 - Flags: review?(dao) → review-
Attached patch Patch addressing review comments (obsolete) — Splinter Review
Attachment #749297 - Attachment is obsolete: true
Attachment #749367 - Flags: review?(dao)
Attachment #749367 - Flags: review?(dao) → review+
No longer blocks: 715230
I also unbitrotted this patch last night but there were some surprises:
1) Add-ons and the addon-sdk use hideChromeForLocation
2) The patch here doesn't prevent hiding the toolbars for the appOrigin case which means it doesn't unblock the bugs listed in comment 14. I didn't see how we would ever hit that case so perhaps we can remove it. This would also mean we could remove CSS which only exists for the [disablechrome] case.

(In reply to Mike Conley (:mconley) from comment #20)
> This was the only usage of hideChromeForLocation, which is a function that
> relies on inContentWhitelist. We should remove that function too.

Actually, the addon-sdk also uses it[1] and it points to https://developer.mozilla.org/en-US/docs/Hiding_browser_chrome which tells other add-on developers how to patch the function. 
[1] https://mxr.mozilla.org/mozilla-central/search?string=hideChromeForLocation
Keywords: addon-compat
Comment on attachment 749367 [details] [diff] [review]
Patch addressing review comments

3) There is also an addon-sdk test: addon-sdk/source/test/test-addon-page.js
Would this feature also be affected by some EOL promise of the SDK?
(In reply to Matthew N. [:MattN] from comment #24)
> 2) The patch here doesn't prevent hiding the toolbars for the appOrigin case
> which means it doesn't unblock the bugs listed in comment 14. I didn't see
> how we would ever hit that case so perhaps we can remove it. This would also
> mean we could remove CSS which only exists for the [disablechrome] case.

We should probably file a separate bug on this and CC some web apps people.

> (In reply to Mike Conley (:mconley) from comment #20)
> > This was the only usage of hideChromeForLocation, which is a function that
> > relies on inContentWhitelist. We should remove that function too.
> 
> Actually, the addon-sdk also uses it[1] and it points to
> https://developer.mozilla.org/en-US/docs/Hiding_browser_chrome which tells
> other add-on developers how to patch the function. 
> [1]
> https://mxr.mozilla.org/mozilla-central/search?string=hideChromeForLocation

It doesn't look like anything will fail horribly there, it's just not going to have any effect as we won't call hideChromeForLocation anymore.
Keywords: dev-doc-needed
This should keep add-ons or the SDK from breaking. We should probably still document that we're no longer actually going to hide anything, though.
Attachment #749389 - Flags: review?(dao)
Blocks: 872162
Attachment #749389 - Flags: review?(dao) → review+
Pushed the patch with dummies, http://hg.mozilla.org/projects/ux/rev/8db97b1af68c
Whiteboard: [fixed-in-ux]
Attachment #749367 - Attachment is obsolete: true
Blocks: 770135
Blocks: 873008
Blocks: 873102
No longer depends on: 738796
Fixing the dep tree, sorry for spam.
https://hg.mozilla.org/mozilla-central/rev/8db97b1af68c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-ux]
Target Milestone: --- → Firefox 28
Blocks: 655679
You need to log in before you can comment on or make changes to this bug.