Closed Bug 851344 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jaws, Assigned: rsilveira)

References

Details

(Whiteboard: feature=work)

Attachments

(1 file)

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.)
Whiteboard: p=2
possibly lowish priority change story for bug 831901?
Blocks: 831901
Flags: needinfo?(asa)
I think we should increase this time as well. This should probably block bug 831924.
Blocks: 831924
No longer blocks: metrov1triage, 831901
Flags: needinfo?(asa)
Summary: Extend the duration of the tab bar showing when the user opens a link in a new tab → Change - Extend the duration of the tab bar showing when the user opens a link in a new tab
Summary: Change - Extend the duration of the tab bar showing when the user opens a link in a new tab → Work - Extend the duration of the tab bar showing when the user opens a link in a new tab
Whiteboard: p=2 → feature=work
What do we want the new time to be?
Flags: needinfo?(ywang)
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
Flags: needinfo?(ywang)
Assignee: nobody → rsilveira
Attached patch Patch v1Splinter Review
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.
Attachment #775037 - Flags: review?(mbrubeck)
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.
(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
(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.
Attachment #775037 - Flags: review?(mbrubeck) → review+
(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 :)
https://hg.mozilla.org/mozilla-central/rev/0a117a4e10a0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.