854274 - Defect - Showing the "Tab Dock" when creating new tabs using "+" button

Status: VERIFIED FIXED

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

Description

[Kamil Jozwiak [:kjozwiak]]

When pressing the create tab button (+), I think it would be a good idea to open the Tab Dock to illustrate that a new tab has been created. The current way can be a little confusing for new users as they might not understand what happened when they pressed the "+"

Actual Results:

- The current page is replaced with the new tab and might confuse new users.

Expected Results:

- When a user selects the "+" button to create a new tab, the Tab Dock should appear, this will let a user know that a new tab has been created and their old tab is still available.

Comment 1

[Brian R. Bondy [:bbondy]]

iteration5 defect?

Comment 2

[Marco Mucci [:MarcoM]]

Moving to Iteration #5 for consideration as a defect.

Comment 3

[Brian R. Bondy [:bbondy]]

Thanks for finding a better blocking story :)

Comment 4

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

Attachment: Patch v1

Added tab closing animation too.

Comment 5

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

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

never mind, need to fix always show tabs.

Comment 6

[Asa Dotzler [:asa]]

Always show tabs is on the chopping block for v1 so if fixing that requires a bunch of extra work, I'm OK with a follow-up to remove that option from the UI prefs.

Comment 7

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

Attachment: Patch v2

Fixed tab closing in always shows tab mode.

Comment 8

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

Thanks Asa, the fix was pretty simple though.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 732891 [details] [diff] [review]
Patch v2

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

Looks good, but I'd like to re-review after the getNextTab comments below are addressed.

::: browser/metro/base/content/browser-ui.js
@@ +387,5 @@
>    },
>  
> +  setOnTabAnimationEnd: function setOnTabAnimationEnd(aCallback) {
> +    if (!aCallback)
> +      return;

Remove the "if (!aCallback)" lines, please -- this should never happen, so let's throw an exception instead of returning silently.

@@ +392,5 @@
> +
> +    Elements.tabs.addEventListener("animationend", function onAnimationEnd() {
> +      Elements.tabs.removeEventListener("animationend", onAnimationEnd);
> +      aCallback();
> +    });

At some point, maybe we should move waitForEvent from tests/head.js to Util.jsm and use it here.

@@ +665,5 @@
>  
>      Elements.urlbarState.setAttribute("mode", "edit");
>      StartUI.show();
> +    if (aShouldDismiss)
> +      ContextUI.dismissTabs();

What's the rationale for leaving the tab bar open after "New Tab" -- I guess it's because the user is interacting with it?  This seems reasonable; I just want to make sure this is the UX we want.

::: browser/metro/base/content/browser.js
@@ +541,5 @@
>  
>      let nextTab = this._selectedTab;
>      if (nextTab == aTab) {
> +      nextTab = this.getTabAtIndex(tabIndex + 1);
> +      if (!nextTab || nextTab.chromeTab.getAttribute("closing"))

nit: use hasAttribute

@@ +545,5 @@
> +      if (!nextTab || nextTab.chromeTab.getAttribute("closing"))
> +        nextTab = this.getTabAtIndex(tabIndex - 1);
> +
> +      if (!nextTab || nextTab.chromeTab.getAttribute("closing"))
> +        return null;

Should we search forward in a loop and then backward in a loop before giving up, in case the user has just closed several tabs in a row?

Could you add a few tests for the getNextTab function, please?

Comment 10

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

(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> @@ +392,5 @@
> > +
> > +    Elements.tabs.addEventListener("animationend", function onAnimationEnd() {
> > +      Elements.tabs.removeEventListener("animationend", onAnimationEnd);
> > +      aCallback();
> > +    });
> 
> At some point, maybe we should move waitForEvent from tests/head.js to
> Util.jsm and use it here.

Good idea. Created bug 858358. 
 
> @@ +665,5 @@
> >  
> >      Elements.urlbarState.setAttribute("mode", "edit");
> >      StartUI.show();
> > +    if (aShouldDismiss)
> > +      ContextUI.dismissTabs();
> 
> What's the rationale for leaving the tab bar open after "New Tab" -- I guess
> it's because the user is interacting with it?  This seems reasonable; I just
> want to make sure this is the UX we want.

At some point I actually had it always dismiss the tab bar after creating/closing a tab, then I changed to match current behavior. Current behavior for opening a tab should be to keep the tab bar open if it was already there, and peek it if it was collapsed - the catch is that peek wasn't working because _editURI was dismissing the tab bar.

In my opinion, if you have the tab bar open and you're creating a new tab, it's unlikely that you'll need the tab bar. But when you're closing more than 1 tab, I think it's better to keep the tab bar there, but it's probably rare. I ended up always keeping it for consistency and for keeping was was there before, but I'm fine either way.

I'm requesting info from Yuan to get her input.

Comment 11

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

Attachment: Patch v3

Addressed review comments and added test for getNextTab

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 733688 [details] [diff] [review]
Patch v3

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

::: browser/metro/base/tests/mochitest/Makefile.in
@@ +30,5 @@
>    browser_context_menu_tests_03.html \
>    text-block.html \
>    browser_sanitize_ui.js \
>    browser_topsites.js \
> +  browser_tabs.js \

This file is missing from the patch.

Comment 13

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

Attachment: Patch v4

Duh! Added the test file.

Comment 14

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

Interestingly enough IE will auto-dismiss the tab bar when you add a new tab, but not when you close one. This is what I think is the best approach.

Comment 15

[Asa Dotzler [:asa]]

(In reply to Rodrigo Silveira [:rsilveira] from comment #14)
> Interestingly enough IE will auto-dismiss the tab bar when you add a new
> tab, but not when you close one. This is what I think is the best approach.

The IE behavior is what we should try for. It's the right split.

Comment 16

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

Attachment: Patch v5

Just added expected peek behavior when opening/closing tabs

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 734044 [details] [diff] [review]
Patch v5

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

::: browser/metro/base/content/browser-ui.js
@@ +1197,5 @@
>        ContextUI.dismissWithDelay(kNewTabAnimationDelayMsec);
>      });
> +
> +    if (!this.isExpanded)
> +      this.displayTabs();

I'm wary of the change to peekTabs.  With this patch, accidentally calling peekTabs while the tab bar is already visible will add an event listener that could stick around until triggered by some later, possibly unrelated animation.

Perhaps it should be something like this:

  if (this.isExpanded) {
    ContextUI.dismissWithDelay(...);
  } else {
    addEventListener("animationend", ...);
    this.displayTabs();
  }

Or maybe code that wants to dismiss the tab bar whether or not it's already open should call dismissWithDelay directly instead of using peekTabs.

Comment 18

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

Thanks for catching this.
I agreed with Asa and Rodrigo.
I think consistency should not be a concern here. Our users wouldn't notice the bar behaviors differently. What they care is whether the UI can support their tasks. 

I think closing multiple tabs all at once is definitely a use case. And we should keep the tab bar open to support that.

It's worthy noting that we will also have a junior-style on-screen "+"(New tab) button in the future. "Adding a new tab" brings the user focus to the FX start. It makes sense to have the tab bar open and auto-miss itself in order to support that focus.

Comment 19

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

(In reply to Matt Brubeck (:mbrubeck) from comment #17)
> ::: browser/metro/base/content/browser-ui.js
> @@ +1197,5 @@
> >        ContextUI.dismissWithDelay(kNewTabAnimationDelayMsec);
> >      });
> > +
> > +    if (!this.isExpanded)
> > +      this.displayTabs();
> 
> I'm wary of the change to peekTabs.  With this patch, accidentally calling
> peekTabs while the tab bar is already visible will add an event listener
> that could stick around until triggered by some later, possibly unrelated
> animation.
> 
> Perhaps it should be something like this:
> 
>   if (this.isExpanded) {
>     ContextUI.dismissWithDelay(...);
>   } else {
>     addEventListener("animationend", ...);
>     this.displayTabs();
>   }
> 
> Or maybe code that wants to dismiss the tab bar whether or not it's already
> open should call dismissWithDelay directly instead of using peekTabs.

Good point. I'm doing pretty much what you mention here, the only thing I added was a setTimout(..., 0) to the case when it's already expanded. This is because the call to _editURI after addTab will eventually cancel the dismiss timeout.

Do you need a new patch?

Comment 20

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

Attachment: Patch v6

Changed as per discussion above.

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6761caea27

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b6761caea27

Comment 23

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #5 testing and everything passed without issues. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-04-14-03-10-25-mozilla-central/

- Ensured that the "Tab Bar" is auto-dismissed when adding a new tab as per Comment 15 (using both the "+" at the top and the "+" at the side of the browser)
- Ensured that the "Tab Bar" is NOT auto-dismissed when closing a tab(s) as per Comment 15
- Went through the test cases in Comment 0 and ensured that creating a new tab will display the "Tab Bar" and then auto-dismissing (as per Comment 15)
- Ensured that the animation works correctly when closing tabs from the "Tab Bar"
- Ensured the "Tab Bar" is always displayed when creating/closing using the "Always Show Tabs" feature in the "Options" charm
- Ensured that all of the above cases work while the browser is in filled view

Comment 24

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #6 testing. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-04-30-03-09-41-mozilla-central/

Went through Comment 23 and ensured all the cases worked without any issues. Also went through the comments (a recap) and ensured the behavior matches the discussion(s) throughout the comments.

Comment 25

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #7 testing. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-05-25-06-25-25-mozilla-central/

Went through Comment 23 and ensured all the cases worked without any issues. Also went through the comments (a recap) and ensured the behavior matches the discussion(s) throughout the comments.

Comment 26

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #8 testing without any issues. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-06-14-03-11-00-mozilla-central/

- Went through all the test cases listed in comment 23 without any issues ("Always Show Tabs" has been removed so not tested)
- Ensured that the "Navigation App Bar" is dismissed when opening a new tab using the right click context menu
- Ensured that creating a new tab using "CTRL + T" auto-dismisses the "Navigation App Bar" once the new tab has been created
- Ensured that all the test cases also worked in "Filled View" without any issues

Comment 27

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #9 testing without any issues. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-06-27-03-10-27-mozilla-central/

- Went through the original attached "Story" without any issues
- Went through the test cases that have been added in comment 23 without any issues
- Went through the test cases that have been added in comment 26 without any issues