851344 - Work - Extend the duration of the tab bar showing when the user opens a link in a new tab

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: App Bar, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Using Window 8 with Firefox in Metro and using just a keyboard and mouse, I ran into the following issue:

When you right click on a link and choose "open in new tab", we hide the overlay too quickly. it is near impossible to switch to that new tab without right-clicking again.

A good suggestion from mbrubeck on #windev:
<mbrubeck> For mouse users, we could even keep waiting longer if the mouse is moving in the direction of the tab bar.... (Not helpful for touch, unfortunately.)

Comment 1

[Brian R. Bondy [:bbondy]]

possibly lowish priority change story for bug 831901?

Comment 2

[Asa Dotzler [:asa]]

I think we should increase this time as well. This should probably block bug 831924.

Comment 3

[Allison Naaktgeboren :ally]

What do we want the new time to be?

Comment 4

[Allison Naaktgeboren :ally]

some digging turns up that it is either 3000(ms)
from http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-ui.js#17

or 3000 + 550 = 3550 from the above + http://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/browser.css#46

Comment 5

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Talked with Frank about the transition on Friday. 
Seems like current the duration feels much shorter than 3000(ms). Frank suggested to investigate if there is any problem with the code first. Once he's back from travel, He will have Jonathan and I work side by side to figure out the correct duration together

Comment 6

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v1

The number of tab dismisses were too damn high! http://cdn.meme.li/instances/400x/39660565.jpg

Organized the dismisses and created 3 different constants to control delay times with initial values of my random liking:
- kNewTabAnimationDelayMsec = 1000; // delay when showing the tab bar briefly after a new (empty) tab opens
- kOpenInNewTabAnimationDelayMsec = 3000; // delay when showing the tab bar after opening a link on a new tab
- kSelectTabAnimationDelayMsec = 500; // delay before closing tab bar after selecting another tab

It didn't feel right that I had to move the constants from contextUI to browser-ui so that they were visible on both files. Not sure if there's a better way to share constants or if we should consider a consts file or maybe I should have them in Util or something.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 775037 [details] [diff] [review]
Patch v1

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

::: browser/metro/base/content/ContextUI.js
@@ -201,5 @@
>      this._setIsExpanded(true);
>    },
>  
> -  // Display the app bar
> -  displayContextAppbar: function () {

Why did you remove this? We want to move all context ui control here, including the find bar. (bug 888091)

@@ -312,5 @@
>          if (aEvent.button == 0 && this.isVisible)
>            this.dismiss();
>          break;
> -      case 'URLChanged':
> -        this.dismissTabs();

Does the tab bar still hide when navigating with this change?

@@ -314,5 @@
>          break;
> -      case 'URLChanged':
> -        this.dismissTabs();
> -        break;
> -      case 'TabSelect':

I'm ok with this since I personally find it a bit annoying, but ux should probably provide input.

Comment 8

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Jim Mathies [:jimm] from comment #7)
> Comment on attachment 775037 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 775037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/ContextUI.js
> @@ -201,5 @@
> >      this._setIsExpanded(true);
> >    },
> >  
> > -  // Display the app bar
> > -  displayContextAppbar: function () {
> 
> Why did you remove this? We want to move all context ui control here,
> including the find bar. (bug 888091)

I removed it because it's not being used anywhere. Probably a left over from when we had the unified tab?

> @@ -312,5 @@
> >          if (aEvent.button == 0 && this.isVisible)
> >            this.dismiss();
> >          break;
> > -      case 'URLChanged':
> > -        this.dismissTabs();
> 
> Does the tab bar still hide when navigating with this change?

Yes, we are dismissing on mousedown and touchstart.

> @@ -314,5 @@
> >          break;
> > -      case 'URLChanged':
> > -        this.dismissTabs();
> > -        break;
> > -      case 'TabSelect':
> 
> I'm ok with this since I personally find it a bit annoying, but ux should
> probably provide input.

To be clear, the tab bar will still dismiss when you select a new tab. Instead of checking this event (which also fires on opening new tabs), I moved it to BrowserUI.selectTabAndDismiss() which is called when a tab is selected [1].

I added a short delay before dismissing on tab select, assuming the general case is that you look at the thumbnail and know which tab you want. It's not enough to comfortably iterate through different tabs and then pick one for example.

[1] https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul#185

Comment 9

[Jim Mathies [:jimm]]

(In reply to Rodrigo Silveira [:rsilveira] from comment #8)
> (In reply to Jim Mathies [:jimm] from comment #7)
> > Why did you remove this? We want to move all context ui control here,
> > including the find bar. (bug 888091)
> 
> I removed it because it's not being used anywhere. Probably a left over from
> when we had the unified tab?

Ah I see. Could have sworn this was in use a couple weeks ago. Looks like all the visibility control is now in appbar.js.

> > @@ -314,5 @@
> > >          break;
> > > -      case 'URLChanged':
> > > -        this.dismissTabs();
> > > -        break;
> > > -      case 'TabSelect':
> > 
> > I'm ok with this since I personally find it a bit annoying, but ux should
> > probably provide input.
> 
> To be clear, the tab bar will still dismiss when you select a new tab.
> Instead of checking this event (which also fires on opening new tabs), I
> moved it to BrowserUI.selectTabAndDismiss() which is called when a tab is
> selected [1].
> 
> I added a short delay before dismissing on tab select, assuming the general
> case is that you look at the thumbnail and know which tab you want. It's not
> enough to comfortably iterate through different tabs and then pick one for
> example.

Ah, too bad :) I like flipping through tabs, the auto-hide after select is annoying.

Comment 10

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Jim Mathies [:jimm] from comment #9)
> Ah, too bad :) I like flipping through tabs, the auto-hide after select is
> annoying.

Well, at least changing that will be super easy now :)

Comment 11

[Rodrigo Silveira [:rsilveira] [:rodms]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a117a4e10a0

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0a117a4e10a0