Closed Bug 964262 Opened 6 years ago Closed 6 years ago

Fade in icons in customization mode

Categories

(Firefox :: Toolbars and Customization, defect)

28 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: phlsa, Assigned: bwinton)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P2])

Attachments

(1 file, 3 obsolete files)

Currently, the items that are not yet in the panel or the toolbars appear once the transition into customization mode is finished.
We should fade them in instead for ~0.3s (lengthening the overall transition time if that is necessary) in order to make the entire transition look smoother.
Whiteboard: [Australis:P?] → [Australis:P-]
Assignee: nobody → bwinton
Attachment #8384680 - Flags: ui-review?(philipp)
Attachment #8384680 - Flags: review?(jaws)
Comment on attachment 8384680 [details] [diff] [review]
The first version of the patch…

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +258,5 @@
>        this._customizing = true;
>        this._transitioning = false;
>  
>        // Show the palette now that the transition has finished.
> +      this.visiblePalette.setAttribute("showing", "true");

Drive-by - I think this is going to hurt our CART results.

I think we should still apply hidden=false/hidden=true to the palette. Apply hidden=false, and then set the showing attribute, and then *maybe* we'll want to have a transitionend listener on the palette, and yield until that transition completes.

Same for the exit transition - remove the showing attribute, yield until the transition completes, and then hide the palette for better performance.

We get better performance because hidden removes the palette from the flow of the document, meaning layout doesn't have to worry about it anymore.
Comment on attachment 8384680 [details] [diff] [review]
The first version of the patch…

mconley's feedback is good, looking forward to a revised patch :)
Attachment #8384680 - Flags: review?(jaws)
Attached patch The second version of the patch. (obsolete) — Splinter Review
Let's try this instead.  :)

(I'm not using requestAnimationFrame because that doesn't give the dom enough time to be updated, and so the opacity doesn't transition.)
Attachment #8384680 - Attachment is obsolete: true
Attachment #8384873 - Flags: review?(jaws)
Whiteboard: [Australis:P-] → [Australis:P2]
Attached patch Patch (obsolete) — Splinter Review
Mike, what do you think about this? I couldn't get Blake's patch to work for me.
Attachment #8384873 - Attachment is obsolete: true
Attachment #8384873 - Flags: review?(jaws)
Attachment #8384954 - Flags: review?(mconley)
Comment on attachment 8384954 [details] [diff] [review]
Patch

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

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +177,5 @@
> +  transition: opacity .3s ease-in-out 1s,
> +              visibility 0s linear;
> +}
> +
> +#main-window[customize-entered] #customization-palette {

The customize-entered attribute will be set on #main-window as soon as the CSS transition is completed, but _not_ before we populate the palette. I wonder if this is why you're requiring the longer transition time.

Perhaps instead of using the customize-entered attribute on the #main-window, perhaps we should select based on an attribute on the #customization-palette, which gets set after the palette is populated? And removed when depopulated?

Also, what did phlsa think of this?
Attachment #8384954 - Flags: review?(mconley) → feedback+
phlsa gave it a ui-r+.

And I'll work on a new patch that adds a new attribute…
(Re-requesting ui-review from Philip, just to make sure this still works the way he wants.  Also to give him a chance to check it out on Jared's Windows machine.  ;)
Attachment #8384954 - Attachment is obsolete: true
Attachment #8385369 - Flags: ui-review?(philipp)
Attachment #8385369 - Flags: review?(jaws)
Comment on attachment 8385369 [details] [diff] [review]
The fourth(?) version of the patch.  ;)

ui-r+, assuming that this fixes the issues of the animation not triggering on Windows under certain circumstances.
Attachment #8385369 - Flags: ui-review?(philipp) → ui-review+
Comment on attachment 8385369 [details] [diff] [review]
The fourth(?) version of the patch.  ;)

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

Perfect, this works great. Thanks :)
Attachment #8385369 - Flags: review?(jaws) → review+
Keywords: checkin-needed
remote:   https://hg.mozilla.org/integration/fx-team/rev/89011a82f5f4
Keywords: checkin-needed
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/89011a82f5f4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
Comment on attachment 8385369 [details] [diff] [review]
The fourth(?) version of the patch.  ;)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis!


User impact if declined: 

A less pleasant customize mode transition.


Testing completed (on m-c, etc.): 

Lots of baking on m-c.


Risk to taking this patch (and alternatives if risky): 

Very low.


String or IDL/UUID changes made by this patch:

None.
Attachment #8385369 - Flags: approval-mozilla-aurora?
Attachment #8385369 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(huh, I only just noticed that this having my name on is a lie... but oh well :-\ )
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.