867317 - Implement transition for showing and hiding toolbars

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement)

Description

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

Attachment: Patch

When the Bookmarks Toolbar opens and closes, we should interpolate between the two positions.

I added an effect that fades out the children of the toolbar so that it doesn't overlap the contents of the navbar much, but to do so I needed to add an inefficient CSS selector (#PersonalToolbar > *). We could forgo this change, but I think it looks nice :)

Frank, what do you think of this? It is a very similar approach to what we did for the find-in-page toolbar.

Mike, you can follow this patch as a guideline for getting the findbar to transition when it is located at the top of the browser window.

Comment 1

[Frank Yan (:fryn)]

Comment on attachment 743775 [details] [diff] [review]
Patch

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

This diff doesn't apply cleanly to mozilla-central.
Is this a diff on top of ux or jamun?
If we make a patch on top of mozilla-central, we could land it sooner, though bit-rot for ux does suck.

::: browser/themes/linux/browser.css
@@ +59,5 @@
>    background-image: linear-gradient(@toolbarHighlight@, rgba(255,255,255,0));
>  }
>  
> +#PersonalToolbar {
> +  transition-property: margin-top, visibility;

s/margin-top/transform/

@@ +61,5 @@
>  
> +#PersonalToolbar {
> +  transition-property: margin-top, visibility;
> +  transition-duration: 150ms, 0s;
> +  transition-timing-function: ease-in-out, linear;

s/ease-in-out/ease-out/

ease-out isn't great, but it's better than ease-in-out in general.

@@ +62,5 @@
> +#PersonalToolbar {
> +  transition-property: margin-top, visibility;
> +  transition-duration: 150ms, 0s;
> +  transition-timing-function: ease-in-out, linear;
> +  margin-top: 0;

Add: `z-index: 1;` after this to solve the overlapping problem.

@@ +66,5 @@
> +  margin-top: 0;
> +}
> +
> +#PersonalToolbar[collapsed="true"] {
> +  margin-top: -29px;

transform: translateY(-100%);

@@ +75,5 @@
>  #personal-bookmarks {
>    min-height: 29px;
>  }
>  
> +#PersonalToolbar > * {

Yeah, this isn't gonna fly. :|

@@ +936,5 @@
>  }
>  
> +#search-container {
> +  position: relative;
> +}

Remove this.

Comment 2

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

(In reply to Frank Yan (:fryn) from comment #1)

What's wrong with using margin-top? We used margin-bottom for the find bar (on the bottom of the browser window).

When I switch this to transform the we get a whole in the browser window where the Aero glass peaks through momentarily. This doesn't happen when we interpolate margin-top.

Comment 3

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

Attachment: Patch v2

This patch now computes the height of the toolbar in JS so that it won't get out-of-touch with the actual toolbar height.

I played with your recommendation of 'transform' but it was janky since it showed the Aero glass during the animation.

The z-index change wasn't noticeable. I think you meant to say that the navbar should have a z-index of 1? But then the navbar would also need position:relative, which breaks the selected tab bottom border.

This overlapping of the toolbars during the transition isn't that big of a deal.

Comment 4

[Frank Yan (:fryn)]

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

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

This attachment doesn't look any different from the previous one.

I think the incorrect overlapping of the toolbars during the transition is a big deal.

Instead of using JS, we could use calc().

Comment 5

[Frank Yan (:fryn)]

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

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

Re-reviewing this patch:

Thanks for filing and working on this. :)

I think we should change how we implement the bottom border of the toolbar region by setting it on #navigator-toolbox or perhaps #navigator-toolbox > toolbar:last-of-type, so that we can get the overlapping right and do something like the following:

#nav-bar {
  ...
  position: relative;
}

#PersonalToolbar {
  ...
  transition-property: margin-top, visibility;
  transition-duration: 150ms, 0s;
  transition-timing-function: ease-out;
}

#PersonalToolbar[collapsed="true"] {
  margin-top: -22px; /* -2px (margin-top) - 20px (toolbar min-height) */
  transition-timing-function: ease-in;
  transition-delay: 0s, 150ms;
}

#PersonalToolbar > toolbarbutton,
#PersonalToolbar > toolbaritem {
  transition: opacity 75ms ease-out 75ms;
}

#PersonalToolbar[collapsed="true"] > toolbarbutton,
#PersonalToolbar[collapsed="true"] > toolbaritem {
  transition-timing-function: ease-in;
  transition-delay: 0s;
  opacity: 0;
}

Comment 6

[Matthew N. [:MattN]]

(In reply to Frank Yan (:fryn) from comment #5)
> I think we should change how we implement the bottom border of the toolbar
> region by setting it on #navigator-toolbox

I'm making this change for OS X in bug 865178. It already works that way on Windows IIRC and I don't know about Linux.

Comment 7

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

Unassigning as I haven't been working on this and I don't want to hold people up.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch

Comment 9

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

Comment on attachment 8442870 [details] [diff] [review]
patch

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

r=me with the change to 150ms.

::: browser/themes/windows/browser.css
@@ +80,5 @@
>  
> +#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar):not(#addon-bar) {
> +  overflow: -moz-hidden-unscrollable;
> +  max-height: 4em;
> +  transition: min-height 250ms ease-out, max-height 250ms ease-out;

We are transitioning the height of the element, which in general is pretty short. We should reduce the duration of this transition to 150ms since there are less pixels to cover than the opening of a new tab, for example. 150ms also appears to be closer to the duration of the transition that Safari uses.

Comment 10

[Mike de Boer [:mikedeboer]]

As a drive-by comment: shouldn't this code go in browser/base/content/browser.css as this is a behavioral change and not so much theme related? It'd also have the nice effect that it'll live in only one place, instead of duplicated to all three theme sheets...

Comment 11

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/a86e80ffaf0c

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> As a drive-by comment: shouldn't this code go in
> browser/base/content/browser.css as this is a behavioral change and not so
> much theme related?

Only if you define "theme" in a narrow, static sense. This is still pretty much part of the visual presentation rather than application logic. When we do put transitions in content stylesheets, it's usually because some JS code expects those transitions to happen, so we don't want to give third-party themes the freedom to omit them. In this case there's no such dependency, so different themes should feel to implement a different transition or none at all.

(In reply to Jared Wein [:jaws] (please needinfo? me) [Away for most of July] from comment #9)
> We are transitioning the height of the element, which in general is pretty
> short. We should reduce the duration of this transition to 150ms since there
> are less pixels to cover than the opening of a new tab, for example. 150ms
> also appears to be closer to the duration of the transition that Safari uses.

I settled on 170, since 150 sometimes felt as if there was hardly any transition. I'm happy to change this again though, especially once some UX folks gave this a try since this didn't go through ui-review.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #11)
> different themes should feel to implement a different transition or none at
> all.

... should feel free to ...

Comment 14

[Dão Gottwald [:dao]]

backed out:
https://hg.mozilla.org/integration/fx-team/rev/228876ffc52c

... because browser/components/places/tests/browser/browser_drag_bookmarks_on_toolbar.js is timing out. I had sent a previous version of this patch to the try server where a different test was failing but not this one, as far as I could tell :(

Comment 15

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/4a9dfa33774d

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 17

[Marco Mucci [:MarcoM]]

Hi Florin, this bug requires a QA contact for verification.  The desktop iteration ends on Monday, is it possible to have it verified by then?

Comment 18

[Sören Hentzschel]

Attachment: transition.mov

The transition is sluggish and the items in the toolbar are jumping (see attached video). Please improve the transition or backout the change.

Comment 19

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit and Ubuntu 13.10 32bit using latest Nightly 33.0a1 (buildID: 	20140622030203). 

Verified on Mac OSX 10.7.5 using latest Nightly 33.0a1 (buildID: 20140623030201) and the items in the toolbar are jumping, like Sören Hentzschel said in comment #18.

Comment 20

[Marco Mucci [:MarcoM]]

(In reply to Camelia Badau, QA [:cbadau] from comment #19)
> Verified fixed on Windows 7 64bit and Ubuntu 13.10 32bit using latest
> Nightly 33.0a1 (buildID: 	20140622030203). 
> 
> Verified on Mac OSX 10.7.5 using latest Nightly 33.0a1 (buildID:
> 20140623030201) and the items in the toolbar are jumping, like Sören
> Hentzschel said in comment #18.

Hi Dao, does this bug need to be reopened based on comment #18?

Comment 21

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

Comment 18 was addressed by bug 1028569, so no need to reopen.

Comment 22

[jaramat]

Hm, the bug summary changed in Comment 11 implies that all toolbars should animate, but so far only the Bookmarks Toolbar does - the Menu Bar (which is also a toolbar) and the Title Bar do not.

And what about toolbars provided by add-ons? Well, taking Classic Theme Restorer as an example, the Additional Toolbar does animate, but the Add-on Bar doesn't. I think the transition should be applied universally to all XUL "toolbar" items (regardless of their position), and not just some of them. This includes even the toolbars that aren't removable by default - the Navigation Toolbar and the Tabs Bar.

And two more things regarding the transition itself:
1. 200ms feels better than 170.
2. Opacity of the toolbar items should be also animated. Though this may cause some tiny performance issues, it's still worth a shot, isn't it?