Pin to Overflow Menu should have associated animation

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
2 months ago
7 days ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-animation] [p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

The overflow button is visible when the toolbar is too narrow to fit all of the buttons. This animation will only occur when a user right-clicks on a button and chooses to "Send to overflow menu" (verbage not final). If an item is moved to the overflow menu as part of resizing the window or through any other non-mouse-based activity, then the animation will not occur.

See bug 1367142 for the specs.
(Assignee)

Updated

2 months ago
Priority: -- → P2

Updated

2 months ago
Flags: qe-verify?
Priority: P2 → P3
Whiteboard: [photon-animation][triage] → [reserve-photon-animation] [p1]
(Assignee)

Updated

2 months ago
Flags: qe-verify? → qe-verify+

Updated

2 months ago
QA Contact: jwilliams
(Assignee)

Updated

2 months ago
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8881995 - Flags: review?(dao+bmo)

Updated

2 months ago
Priority: P3 → P1
Whiteboard: [reserve-photon-animation] [p1] → [photon-animation] [p1]
(Assignee)

Updated

2 months ago
Summary: Send to Overflow Menu should have associated animation → Pin to Overflow Menu should have associated animation

Comment 2

2 months ago
mozreview-review
Comment on attachment 8881995 [details]
Bug 1375152 - Implement animation for pinning items to the overflow menu.

https://reviewboard.mozilla.org/r/153070/#review158566

::: browser/themes/shared/icons/chevron-animation.svg:6
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="1278" height="24">
> +  <svg width="18" height="24">
> +    <path fill="context-fill" d="M9.707 7.293l-5 -5a1 1 0 1 0 -1.414 1.414l4.293 4.293 -4.293 4.293a1 1 0 0 0 -0.025 1.414 1 1 0 0 0 1.414 0.025l0.025 -0.025 5 -5a1 1 0 0 0 0 -1.414zm6 0l-5 -5a1 1 0 1 0 -1.414 1.414l4.293 4.293 -4.293 4.293a1 1 0 0 0 -0.025 1.414 1 1 0 0 0 1.414 0.025l0.025 -0.025 5 -5a1 1 0 0 0 0 -1.414z"/>

Please move fill="context-fill" to the root svg node. Paths that need a different fill can still specify that.

::: browser/themes/shared/toolbarbutton-icons.inc.css:222
(Diff revision 1)
> +}
> +
> +#nav-bar-overflow-button > .toolbarbutton-animatable-box {
> +  position: absolute;
> +  overflow: hidden;
> +  top: calc(50% - 7px); /* Vertically center the 24px tall image. */

There's a similar problem as what I described in bug 1355924 comment 31 (second part).

::: browser/themes/shared/toolbarbutton-icons.inc.css:222
(Diff revision 1)
> +}
> +
> +#nav-bar-overflow-button > .toolbarbutton-animatable-box {
> +  position: absolute;
> +  overflow: hidden;
> +  top: calc(50% - 7px); /* Vertically center the 24px tall image. */

Please document how you arrived at 7px. Shouldn't this be 12px, i.e. half of the height?
Attachment #8881995 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)

Comment 4

2 months ago
mozreview-review
Comment on attachment 8881995 [details]
Bug 1375152 - Implement animation for pinning items to the overflow menu.

https://reviewboard.mozilla.org/r/153070/#review158630

::: browser/themes/shared/toolbarbutton-icons.inc.css:223
(Diff revisions 1 - 2)
>  
>  #nav-bar-overflow-button > .toolbarbutton-animatable-box {
>    position: absolute;
>    overflow: hidden;
> -  top: calc(50% - 7px); /* Vertically center the 24px tall image. */
> +   /* Due to bug 1363730, position:absolute; doesn't position the box
> +      correctly relative to the closest non-staticly positioned ancestor.

Oh, does position:fixed work?
Comment hidden (mozreview-request)

Comment 6

2 months ago
mozreview-review
Comment on attachment 8881995 [details]
Bug 1375152 - Implement animation for pinning items to the overflow menu.

https://reviewboard.mozilla.org/r/153070/#review158766

::: browser/themes/shared/icons/chevron-animation.svg:72
(Diff revision 3)
> +  </svg>
> +  <svg width="18" height="24" x="378">
> +    <path d="M9.707 13.957l5 -5a1 1 0 1 0 -1.414 -1.414l-4.293 4.293 -4.293 -4.293a1 1 0 0 0 -1.414 -0.025 1 1 0 0 0 -0.025 1.414l0.025 0.025 5 5a1 1 0 0 0 1.414 0zm0 6l5 -5a1 1 0 1 0 -1.414 -1.414l-4.293 4.293 -4.293 -4.293a1 1 0 0 0 -1.414 -0.025 1 1 0 0 0 -0.025 1.414l0.025 0.025 5 5a1 1 0 0 0 1.414 0z"/>
> +  </svg>
> +  <svg width="18" height="24" x="396">
> +    <path fill="#39454F" d="M9.707 13.957l5 -5a1 1 0 1 0 -1.414 -1.414l-4.293 4.293 -4.293 -4.293a1 1 0 0 0 -1.414 -0.025 1 1 0 0 0 -0.025 1.414l0.025 0.025 5 5a1 1 0 0 0 1.414 0zm0 6l5 -5a1 1 0 1 0 -1.414 -1.414l-4.293 4.293 -4.293 -4.293a1 1 0 0 0 -1.414 -0.025 1 1 0 0 0 -0.025 1.414l0.025 0.025 5 5a1 1 0 0 0 1.414 0z"/>

These fill colors don't look right with inverted icons (i.e. dark themes), as they assume that we're transitioning from black to blue. Is it possible to mix context-fill with blue in the SVG?

The easiest way would probably be to set fill: #30A3FF; in the @keyframes 'to { }' rule. This is theoretically more expensive since the SVG needs to be updated every time the context-fill changes, but perf concerns don't seem too critical for this particular animation. Might be worth testing on a slow machine though.
Attachment #8881995 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)

Comment 8

2 months ago
mozreview-review
Comment on attachment 8881995 [details]
Bug 1375152 - Implement animation for pinning items to the overflow menu.

https://reviewboard.mozilla.org/r/153070/#review158904
Attachment #8881995 - Flags: review?(dao+bmo) → review+
(In reply to Dão Gottwald [::dao] from comment #6)
> The easiest way would probably be to set fill: #30A3FF; in the @keyframes
> 'to { }' rule. This is theoretically more expensive since the SVG needs to
> be updated every time the context-fill changes, but perf concerns don't seem
> too critical for this particular animation. Might be worth testing on a slow
> machine though.

I tested with the reference laptop and this animation didn't look bad or stutter.

Comment 10

2 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2998d8a55394
Implement animation for pinning items to the overflow menu. r=dao

Comment 11

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2998d8a55394
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1378390

Comment 12

a month ago
I have reproduced this bug with Nightly 56.0a1 (2017-06-21) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 56.0a1 !

Build   ID    20170705030206
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0

[bugday-20170705]
Depends on: 1381991

Updated

21 days ago
Depends on: 1384654
Depends on: 1385923

Updated

17 days ago
Iteration: --- → 56.2 - Jul 10
QA Contact: jwilliams → stefan.georgiev
I have verified this fix on today's nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.