Closed Bug 867317 Opened 11 years ago Closed 10 years ago

Implement transition for showing and hiding toolbars

Categories

(Firefox :: Theme, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 33

People

(Reporter: jaws, Assigned: dao)

References

Details

(Whiteboard: p=13 s=33.1 [qa!])

Attachments

(2 files, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #743775 - Flags: review?(fyan)
Attachment #743775 - Flags: feedback?(mdeboer)
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.
Attachment #743775 - Flags: review?(fyan) → review-
(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.
Flags: needinfo?(fyan)
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #743775 - Attachment is obsolete: true
Attachment #743775 - Flags: feedback?(mdeboer)
Attachment #743840 - Flags: review?(fyan)
Flags: needinfo?(fyan)
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().
Attachment #743840 - Flags: review?(fyan)
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;
}
Attachment #743840 - Flags: ui-review-
(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.
Whiteboard: p=0
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
No longer blocks: fxdesktopbacklog
Whiteboard: p=0 → p=5
Unassigning as I haven't been working on this and I don't want to hold people up.
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Whiteboard: p=5 → p=5 [qa?]
Whiteboard: p=5 [qa?] → p=5 [qa+]
Whiteboard: p=5 [qa+] → p=13 [qa+]
Assignee: nobody → dao
Status: NEW → ASSIGNED
Whiteboard: p=13 [qa+] → p=13 s=33.1 [qa+]
Attached patch patchSplinter Review
Attachment #743840 - Attachment is obsolete: true
Attachment #8442870 - Flags: review?(jaws)
Component: Toolbars and Customization → Theme
Severity: normal → enhancement
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.
Attachment #8442870 - Flags: review?(jaws) → review+
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...
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.
Summary: The Bookmarks Toolbar should transition when opening and closing → Implement transition for showing and hiding toolbars
(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 ...
No longer blocks: 750212
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 :(
https://hg.mozilla.org/mozilla-central/rev/4a9dfa33774d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
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?
Flags: needinfo?(florin.mezei)
Attached video 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.
Depends on: 1028569
Flags: needinfo?(florin.mezei)
QA Contact: camelia.badau
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.
Whiteboard: p=13 s=33.1 [qa+] → p=13 s=33.1 [qa!]
(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?
Iteration: --- → 33.2
Points: --- → 13
QA Whiteboard: [qa!]
Flags: needinfo?(dao)
Whiteboard: p=13 s=33.1 [qa!]
Comment 18 was addressed by bug 1028569, so no need to reopen.
Status: RESOLVED → VERIFIED
Flags: needinfo?(dao)
Iteration: 33.2 → ---
Points: 13 → ---
QA Whiteboard: [qa!]
Whiteboard: p=13 s=33.1 [qa!]
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?
Depends on: 1028872
Depends on: 1037303
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: