Closed Bug 644721 Opened 10 years ago Closed 8 years ago

Hide all toolbars for app tabs by default

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: Felipe, Assigned: Felipe)

References

(Depends on 1 open bug)

Details

(Whiteboard: [strings])

Attachments

(5 files, 8 obsolete files)

6.65 KB, patch
Gavin
: feedback+
Details | Diff | Splinter Review
40.21 KB, patch
Details | Diff | Splinter Review
5.38 KB, patch
Details | Diff | Splinter Review
13.59 KB, patch
Gavin
: review-
Details | Diff | Splinter Review
21.59 KB, image/png
Details
Things needed to be done to land this feature:

- Site specific pref to show toolbars for app tabs, tied to an entry in the tab's context menu

- Ctrl + L to bring back the navigation toolbar temporarily, until blur.

- Hang doorhangers out of the app tab icon


---
Things to keep in mind for follow-ups

- Theme polish for tabs + hidden toolbars: see http://i.imgur.com/q4iwQ.png, bug 637018

- View -> Navigation bar is currently messy as a global/per-window setting, and this featire

- "Home tab as app tab" site-pref should be "show toolbars" by default
Whiteboard: [strings]
Temporarily show navigation bar on Ctrl+L (plus focus the url bar) until focus moves out of the nav bar (by pressing Enter, tabbing out or clicking outside)
Hide toolbars for all app tabs, except about:blank and about:home. Updates the UI on pin/unpin and tab switches
Attached patch Part 3 - site-specific pref (obsolete) — Splinter Review
Part 3 - Creates a site-specific pref to show toolbars even in app-tab mode, adds a tab context menu entry for this pref and ties that into the toolbar UI updates
Still to do:
 - needs to fix a CSS glitch that shows a gray line when nav bar is temporarily on after Ctrl + L
 - understand some more edge cases of app tab -> Ctrl+L -> type new address, and about:home
 - lots o' tests

No need to do:
 - doorhangers already work for app tabs as expected (thanks Margaret!!)
Forgot two things (just a note to self):
 - make all of this kill-switchable
 - nuke openLocation.xul from orbit (if the tree rules will allow that)
Attached patch Part 1 + 2 (obsolete) — Splinter Review
Folded parts 1 and 2 together as they are closely related.

Asking feedback on the general approach and specially to see what Dao thinks of the shownavbar attribute used here. This is what Frank suggested to work nicely with the tricky border rules (the margin-bottom: -1px on the patch) for disablechrome. This avoids the CSS glitch I mentioned in comment 4.

At first we were going to use just a different value for disablechrome (disablechrome=all and disablechrome=shownavbar), but I didn't want to have the code track what the previous value was, so I went with a separate attribute instead.

Not asking review yet because I'm struggling to make the focus/blur track to work reliably enough. It seems that sometimes it misses some blurs..
Attachment #521713 - Attachment is obsolete: true
Attachment #521715 - Attachment is obsolete: true
Attachment #522562 - Flags: feedback?(dao)
Attached patch Part 3 - site-specific pref (obsolete) — Splinter Review
This part ties the feature to the site-specific prefs service.
Attachment #521716 - Attachment is obsolete: true
Attachment #522566 - Flags: review?(dao)
Attached patch Part 1 + 2 (obsolete) — Splinter Review
Various clean-ups and updates. Tied everything to an about:config pref and made the observers lazily started only on first switch to an app tab. Also added the case that we missed in the discussions about the neterror pages.

This is almost good to go for review, only problem is that navigation that results in neterror is not being caught correctly at OnLocationChange (contentDocument.documentURI still returns the previous URL before the navigation happened)
Attachment #522562 - Attachment is obsolete: true
Attachment #522562 - Flags: feedback?(dao)
Attachment #522893 - Flags: feedback?(dao)
Small updates and improvements based on some changes from the parts-1-and-2 patch.

(Sorry about updating it now; if you have already started reviewing the previous one I can provide an interdiff)
Attachment #522566 - Attachment is obsolete: true
Attachment #522566 - Flags: review?(dao)
Attachment #522896 - Flags: review?(dao)
Attached patch Part 1 + 2 (obsolete) — Splinter Review
Changed the call site from the previous patch to somewhere that catches the neterror pages, and fixed a problem with the click-and-make-it-go-away handler.

This is ready for review
Attachment #522893 - Attachment is obsolete: true
Attachment #522893 - Flags: feedback?(dao)
Attachment #523479 - Flags: review?(gavin.sharp)
Attachment #523479 - Flags: review?(dao)
Attachment #522896 - Flags: review?(gavin.sharp)
Comment on attachment 523479 [details] [diff] [review]
Part 1 + 2

> function focusAndSelectUrlBar() {
>+  let navBar = document.getElementById("nav-bar");
>   if (gURLBar && !gURLBar.readOnly) {
>     if (window.fullScreen)
>       FullScreen.mouseoverToggle(true);
>     if (isElementVisible(gURLBar)) {
>       gURLBar.focus();
>       gURLBar.select();
>       return true;
>+    } else if (!isElementVisible(navBar) && 
>+               !navBar.collapsed &&
>+               navBar.compareDocumentPosition(gURLBar) & Node.DOCUMENT_POSITION_CONTAINED_BY) {
>+
>+      ChromelessAppTabs.showNavBar();
>+
>+      if (isElementVisible(gURLBar)) {
>+        gURLBar.focus();
>+        gURLBar.select();
>+        return true;
>+      }

This seems suboptimal, given the customization possibilities... Showing a surrogate location bar in a panel would be a more complete solution, and it would avoid shifting down the content. I actually made a work-in-progress patch for this several months ago. I'll go and see if this is still usable.

>+  toggleBrowserToolbarUI: function() {

Better name needed...
90% of this fail to apply, but you get the idea...
Attached patch testsSplinter Review
(In reply to comment #11)
> This seems suboptimal, given the customization possibilities... Showing a
> surrogate location bar in a panel would be a more complete solution

I don't think inventing another widget is really what we want here. If we build one, then we'll need the same thing for the search bar, and possibly other aspects too.

If there are issues with focusing and defocusing the existing toolbar, I think we should work on resolving those instead.
Comment on attachment 523479 [details] [diff] [review]
Part 1 + 2

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

>-  hideChromeForLocation: function(aLocation) {

I think you should keep this, and call it from toggleBrowserToolbarUI

>+  toggleBrowserToolbarUI: function() {

>+    let currentState = (document.documentElement.getAttribute("disablechrome") == "true");
>+
>+    if (hideChrome == currentState)
>+      return;

This check is just as expensive (or more, given xpconnect overheard) than the internal checks done by setAttribute/removeAttribute, so just omit it.

>+var ChromelessAppTabs = {

>+  handleEvent: function(event) {

>+    switch (event.type) {
>+      case "blur":
>+        setTimeout(function() {
>+          try
>+          {
>+            let isDescendant = navBar.compareDocumentPosition(focused) & 

This looks broken, "focused" isn't defined here.

In general I tend to agree with dao, this approach is kind of fragile. Do we need to block this bug on a better solution to Cmd+L when the location bar is hidden? I.e. could we just land this and rely on the (ugly/sucky) open location dialog until we come up with something better?

>+  observe: function(aSubject, aTopic, aData) {
>+    if (aTopic == "nsPref:changed" && aData == this._globalPrefName)
>+      this.enabled = Services.prefs.getBoolPref(this._globalPrefName);
>+  },

This pref probably doesn't need to be live.

>+  _blacklist: [
>+    /^about:neterror\?/,
>+    /^about:blocked\?/,
>+    /^about:certerror\?/,

I'm not sure I understand the motivation behind these exceptions.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

Seems like it would be cleaner to avoid changes here and just use TabPinned/TabUnPinned event listeners in browser.js.
Attachment #523479 - Flags: review?(gavin.sharp)
Attachment #523479 - Flags: review?(dao)
Attachment #523479 - Flags: review-
Comment on attachment 522896 [details] [diff] [review]
Part 3 - site-specific pref

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

> var ChromelessAppTabs = {

>   shouldHideToolbars: function() {
>     return gBrowser.selectedTab.pinned &&
>            this.enabled &&
>-           !this.isBlacklisted(gBrowser.contentDocument.documentURI);
>+           !this.isBlacklisted(gBrowser.contentDocument.documentURI) &&
>+           ChromelessAppTabs.getPrefForSite(gBrowser.currentURI) != "true";

this.getPrefForSite?

>+    // Hide chrome for app tabs
>+    if (this.contextTab.pinned && !ChromelessAppTabs.isBlacklisted(this.contextTab.linkedBrowser.contentDocument.documentURI)) {
>+      document.getElementById("context_alwaysShowToolbars").hidden = false;
>+      document.getElementById("context_alwaysShowToolbars").setAttribute("checked",
>+                                                                         ChromelessAppTabs.getPrefForSite(this.contextTab.linkedBrowser.currentURI));
>+    } else {
>+      document.getElementById("context_alwaysShowToolbars").hidden = true;

add local variables for document.getElementById("context_alwaysShowToolbars") and this.contextTab.linkedBrowser.currentURI, and use currentURI.spec instead of documentURI?

This looks fine, but we need to sort out what the base patch will look like first. I don't think we need to track this patch in this bug, it can go in a followup, right?
Attachment #522896 - Flags: review?(gavin.sharp)
Attachment #522896 - Flags: review?(dao)
Attachment #522896 - Flags: feedback+
(In reply to comment #15)
> In general I tend to agree with dao, this approach is kind of fragile. Do we
> need to block this bug on a better solution to Cmd+L when the location bar
> is hidden? I.e. could we just land this and rely on the (ugly/sucky) open
> location dialog until we come up with something better?

How fragile is it, though? The old dialog doesn't behave like the URL bar at all, and very much gets in the way of normal operation. Even if it's fragile

Remember that we're already pretty deep into power-user land here, app tabs are a highly specialized feature that isn't used by a lot of our users — it's only available through right-clicking a tab.

I'd rather land it the way Felipe has it, and work on resolving the focus issues in a follow-up bug.
> >+  observe: function(aSubject, aTopic, aData) {
> >+    if (aTopic == "nsPref:changed" && aData == this._globalPrefName)
> >+      this.enabled = Services.prefs.getBoolPref(this._globalPrefName);
> >+  },
> 
> This pref probably doesn't need to be live.

This was done not so much to keep the pref live, but to avoid retrieving its value from the prefservice on every tab switch. Though it's only retrieved for pinned tabs. Let me know what you think is the best method here.

> 
> >+  _blacklist: [
> >+    /^about:neterror\?/,
> >+    /^about:blocked\?/,
> >+    /^about:certerror\?/,
> 
> I'm not sure I understand the motivation behind these exceptions.

If a site you've pinned is compromised and its certificate is changed, only showing the about page won't give easy access to the larry button to verify the cert.
Similarly if a page becomes blocked or if it can't be loaded, trying to get out of it is somewhat disconcerting without the URL bar.


> add local variables for
> document.getElementById("context_alwaysShowToolbars") and
> this.contextTab.linkedBrowser.currentURI, and use currentURI.spec instead of
> documentURI?

currentURI doesn't reflect the about:neterror and other pages (it still gives the URL that was tried to open). That's why I had to use documentURI here

> 
> This looks fine, but we need to sort out what the base patch will look like
> first. I don't think we need to track this patch in this bug, it can go in a
> followup, right?

I believe the site-specific pref is required to land the feature, so I'd prefer to track it all here. If it's not true I'll move the patch to a follow-up.


Will update the patch with the other comments
> The old dialog doesn't behave like the URL bar at all, and very much gets in
> the way of normal operation.

This is easily addressed by appropriately styling a mock URL bar in a panel.

> Remember that we're already pretty deep into power-user land here, app tabs
> are a highly specialized feature that isn't used by a lot of our users —
> it's only available through right-clicking a tab.

Power users are those who'll more likely customize the UI (to the extent that the URL bar could be outside of the nav bar), install extensions that add random widgets to the nav bar and invoke fancy actions (e.g. open a panel) that can't be caught, because it's all tied to whether focus is somewhere in the nav bar. It's unnecessary to open this can of worms when all we actually care about is allowing the user to enter a URL.
(In reply to comment #18)
> I believe the site-specific pref is required to land the feature, so I'd
> prefer to track it all here. If it's not true I'll move the patch to a
> follow-up.

Landing this change without the site-specific preference will make it feel broken, so I'd prefer that we kept them together if we can.
> Power users are those who'll more likely customize the UI (to the extent
> that the URL bar could be outside of the nav bar), install extensions that
> add random widgets to the nav bar and invoke fancy actions (e.g. open a
> panel) that can't be caught, because it's all tied to whether focus is
> somewhere in the nav bar. It's unnecessary to open this can of worms when
> all we actually care about is allowing the user to enter a URL.

I'd like to point out that the current implementation is very conservative about the way it try to shows the nav bar. If anything is not in the expected default settings it falls back to showing the openLocation.xul dialog.

Also, in my testing I didn't see panels being affected by the patch. Opening a doorhanger or other panel does not change the focus on the browser window, so for example clicking in the larry button works as expected. It seems that if there are edge cases they would be way more rare than it looks like at first.
(In reply to comment #21)
> I'd like to point out that the current implementation is very conservative
> about the way it try to shows the nav bar. If anything is not in the
> expected default settings it falls back to showing the openLocation.xul
> dialog.

"Expected default" only means the url bar being on the nav bar, right? It doesn't care other items on the nav bar at all.
Secondly, we actually don't want openLocation.xul, hence my proposal to get rid of it for good -- or not at all, if we think the whole use case is only an edge case for power users anyway.

> Also, in my testing I didn't see panels being affected by the patch. Opening
> a doorhanger or other panel does not change the focus on the browser window,
> so for example clicking in the larry button works as expected.

I'm pretty sure this isn't true. Did you fix the blur handler? Do you see the focus change if you make it a capturing listener?
Is there consideration for adding a 'Copy Page Location' after 'Send Link...' for context menus when clicking in the page vs. a link within the App Tab?
(In reply to comment #20)
> Landing this change without the site-specific preference will make it feel
> broken, so I'd prefer that we kept them together if we can.

I don't think I agree (re: the feature being "broken"), but it doesn't matter -  "keep them together" doesn't need to happen at the bug level.
> "Expected default" only means the url bar being on the nav bar, right? It
> doesn't care other items on the nav bar at all.

It also checks if the nav-bar has not been permanently hidden through the menu

> > Also, in my testing I didn't see panels being affected by the patch. Opening
> > a doorhanger or other panel does not change the focus on the browser window,
> > so for example clicking in the larry button works as expected.
> 
> I'm pretty sure this isn't true. Did you fix the blur handler? Do you see
> the focus change if you make it a capturing listener?

I fixed the the panel thing recently using the same approach as to what the fullscreen UI does, and another minor focus issue which was caused by the missing "focused" variable on the latest patch. I'll include these changes in a new patch in a moment.

The blur listener is already a capturing listener. Did you mean a bubbling listener? I can't change it to a bubbling listener because it misses one situation (when you press Enter in the url bar). This could be fixed by other means if we want to completely avoid the capturing listener, but I don't think that is necessary.
Attached patch Part 1 + 2 (obsolete) — Splinter Review
> >+  toggleBrowserToolbarUI: function() {
> 
> Better name needed...

changed it to updateToolbarVisibility


> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >-  hideChromeForLocation: function(aLocation) {
> 
> I think you should keep this, and call it from toggleBrowserToolbarUI

done


> >+  toggleBrowserToolbarUI: function() {
> 
> >+    let currentState = (document.documentElement.getAttribute("disablechrome") == "true");
> >+
> >+    if (hideChrome == currentState)
> >+      return;
> 
> This check is just as expensive (or more, given xpconnect overheard) than
> the internal checks done by setAttribute/removeAttribute, so just omit it.

done

> 
> >+var ChromelessAppTabs = {
> 
> >+  handleEvent: function(event) {
> 
> >+    switch (event.type) {
> >+      case "blur":
> >+        setTimeout(function() {
> >+          try
> >+          {
> >+            let isDescendant = navBar.compareDocumentPosition(focused) & 
> 
> This looks broken, "focused" isn't defined here.

fixed

> 
> >+  observe: function(aSubject, aTopic, aData) {
> >+    if (aTopic == "nsPref:changed" && aData == this._globalPrefName)
> >+      this.enabled = Services.prefs.getBoolPref(this._globalPrefName);
> >+  },
> 
> This pref probably doesn't need to be live.

changed it to retrieve-on-first-use and removed observer

 
> >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
> 
> Seems like it would be cleaner to avoid changes here and just use
> TabPinned/TabUnPinned event listeners in browser.js.

Done (although I think adding/removing these listeners on startup/shutdown seems more trouble than changing tabbrowser)

Also added popupshown/popuphidden listeners to keep track of panels like the fullscreen controller does
Attachment #523479 - Attachment is obsolete: true
Blocks: 657525
Attachment #532766 - Flags: review?(gavin.sharp)
Depends on: 662923
Depends on: 662926
Updated the patch to include the fact that in addition to Ctrl+L, clicking on the app tab itself should bring the toolbar back.
Attachment #532766 - Attachment is obsolete: true
Attachment #532766 - Flags: review?(gavin.sharp)
Attachment #539373 - Flags: review?(gavin.sharp)
Depends on: 664359
Comment on attachment 539373 [details] [diff] [review]
Part 1 + 2 - updated

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

>   asyncUpdateUI: function () {
>     FeedHandler.updateFeeds();
>+    // Show or hide browser chrome based on the whitelist for in-content UI
>+    // or the tab pinned status
>+    this.updateToolbarVisibility();

I think asyncUpdateUI happens too frequently for this purpose (e.g. it fires for subframes navigation changes). I would generally like to get rid of it, so would prefer not making use of it here.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>       <handler event="mousedown" button="0" phase="capturing">

>+          if (event.button == 0 &&
>+              this.selected &&
>+              ChromelessAppTabs.shouldHideToolbars(this)) {
>+            ChromelessAppTabs.showNavBar();
>+          }

Why is this needed? I don't even really understand why if (shouldHideToolbars()) showNavBar() makes sense.

I know there is some attachment to this patch and this approach in general, but I agree with Dao that this ends up being quite hacky (theme interactions with disablechrome/shownavbar, odd event listener heuristics for detecting interaction, focus issues, etc.) and inconsistent. We can get much the same experience using a panel approach with none of those downsides, so it seems best to me to pursue that approach rather than continue evaluating this one.
Attachment #539373 - Flags: review?(gavin.sharp) → review-
Seen this on UX builds. IMO toolbars should be shown when customizing.
Duplicate of this bug: 585445
Toolbars should be visible on App tabs by default and users should be able to hide it from the context menu (Hide toolbars for this site). Without this many users will find it difficult to access buttons like reload and back/forward.
Benefits:
a) Users would not complain about hiding toolbars without their consent or about broken design

b) "browser.tabs.chromelessAppTabs" is difficult to reach for the average user and may no longer be needed

c) Will be easy to discover for users who use the context menu to pin/unpin tabs
App Tabs was created for pinning web applications, not for every insignificant site user is visiting.
(In reply to Siddhartha Dugar [:sdrocking] from comment #33)
> Toolbars should be visible on App tabs by default and users should be able
> to hide it from the context menu (Hide toolbars for this site). Without this
> many users will find it difficult to access buttons like reload and
> back/forward.

Agree , it should be upto the users
wontfix as per bug 769101 comment 16
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.