Closed Bug 1393565 Opened 3 years ago Closed 3 years ago

De-duplicate the JS code and CSS that sets the bookmark and pocket library animation

Categories

(Firefox :: Toolbars and Customization, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(1 file)

Follow-up from bug 1384953.

Saving a page to Pocket or bookmarking a page will both run an animation on the Library button. The code to implement this animation is duplicated for each one so that Pocket's implementation can be independent since it is installed as a system-addon and not integrated fully with the product.

(In reply to :Gijs from bug 1384953 comment #15)
> Can we factor all this duplicated code out to a utility function on (for
> want of a better place) BookmarkingUI? Like maybe
> `triggerLibraryAnimation(animation, animationendListener)`, where
> `animation` is one of `bookmark` or `pocket`, and then invoke that from the
> two callsite? It looks like the fade animation stuff could also be shared.
> 
> ::: browser/themes/shared/toolbarbutton-icons.inc.css:464
> (Diff revision 3)
> > -
> >  #library-button[animate="bookmark"] > .toolbarbutton-icon {
> >    fill: transparent;
> >  }
> >  
> > -#library-button[animate="bookmark"] > .toolbarbutton-animatable-box {
> > +.toolbarbutton-animatable-box[animate="bookmark"] {
> 
> It looks like most of the styles in this block could be shared too.
Flags: qe-verify-
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Priority: P4 → P1
Comment on attachment 8902876 [details]
Bug 1393565 - De-duplicate the JS code and CSS that sets the bookmark and pocket library animation.

https://reviewboard.mozilla.org/r/174570/#review180456

So this would be r+ for the JS part, but the commit message says "and CSS", but you're not touching the CSS... :-)

::: browser/base/content/browser-places.js:1517
(Diff revision 1)
> +    let libraryButton = document.getElementById("library-button");
> +    if (!libraryButton ||
> +        libraryButton.getAttribute("cui-areatype") == "menu-panel" ||
> +        libraryButton.getAttribute("overflowedItem") == "true" ||
> +        !libraryButton.closest("#nav-bar") ||
> +        !Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {

I wonder if this should be a lazy pref getter on this object. Doesn't really matter much as this code isn't hot, I guess...

::: browser/extensions/pocket/bootstrap.js:195
(Diff revision 1)
>      this.pageAction.remove();
>      this.pageAction = null;
>    },
>  
>    startLibraryAnimation(doc) {
> -    let libraryButton = doc.getElementById("library-button");
> +    doc.defaultView.LibraryUI.triggerLibraryAnimation("pocket");

Assuming there's only 1 caller, looks like this could move to the caller.
Attachment #8902876 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8902876 [details]
Bug 1393565 - De-duplicate the JS code and CSS that sets the bookmark and pocket library animation.

https://reviewboard.mozilla.org/r/174570/#review180720

::: browser/themes/shared/toolbarbutton-icons.inc.css:486
(Diff revision 2)
>    height: var(--toolbarbutton-height);
>    min-height: 54px; /* Minimum height must be equal to the height of the SVG sprite */

The height and min-height should probably be part of the common styling too (and can be removed from the pocket block in pocket.css) because the "half the sprite height" is now shared, too, and those two values are linked.
Attachment #8902876 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8902876 [details]
Bug 1393565 - De-duplicate the JS code and CSS that sets the bookmark and pocket library animation.

https://reviewboard.mozilla.org/r/174570/#review180722

::: browser/base/content/browser-places.js:1512
(Diff revision 2)
>  /**
> + * Handles the Library button in the toolbar.
> + */
> +var LibraryUI = {
> +  triggerLibraryAnimation(animation) {
> +    if (!Object.getOwnPropertyNames(this).includes("COSMETIC_ANIMATIONS_ENABLED")) {

Can't this just be `this.hasOwnProperty("COSMETIC_ANIMATIONS_ENABLED")` ? If not, can you add a comment about why not?
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf39016f8878
De-duplicate the JS code and CSS that sets the bookmark and pocket library animation. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/bf39016f8878
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.