Closed
Bug 851344
Opened 12 years ago
Closed 11 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)
Firefox for Metro Graveyard
App Bar
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jaws, Assigned: rsilveira)
References
Details
(Whiteboard: feature=work)
Attachments
(1 file)
(deleted),
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
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.)
Updated•12 years ago
|
Blocks: metrov1triage
Updated•12 years ago
|
Whiteboard: p=2
Comment 1•12 years ago
|
||
possibly lowish priority change story for bug 831901?
Blocks: 831901
Flags: needinfo?(asa)
Comment 2•12 years ago
|
||
I think we should increase this time as well. This should probably block bug 831924.
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
Updated•12 years ago
|
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
Comment 4•12 years ago
|
||
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•12 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → rsilveira
Assignee | ||
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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.
Assignee | ||
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #775037 -
Flags: review?(mbrubeck) → review+
Assignee | ||
Comment 10•11 years ago
|
||
(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 :)
Assignee | ||
Comment 11•11 years ago
|
||
Comment 12•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•