Australis - Implement bookmarking animation

VERIFIED FIXED in Firefox 29

Status

()

VERIFIED FIXED
6 years ago
4 months ago

People

(Reporter: ntim, Assigned: dhenein)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified)

Details

(Whiteboard: [Australis:P5], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131024030204

Steps to reproduce:

Bookmark something


Actual results:

There's no animation


Expected results:

There should be an animation as shown here : http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html
(Reporter)

Updated

6 years ago
Blocks: 870032, 872617
Component: Untriaged → Theme
OS: Windows 8.1 → All
Hardware: x86_64 → All
Whiteboard: [Australis:M-][Australis:P5]
(Reporter)

Updated

6 years ago
Whiteboard: [Australis:M-][Australis:P5] → [Australis:M?][Australis:P5]
Please don't file bugs for random features in random mockups. We're quite capable of filing and tracking our own bugs.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
(Reporter)

Comment 2

6 years ago
(In reply to Justin Dolske [:Dolske] from comment #1)
> Please don't file bugs for random features in random mockups. We're quite
> capable of filing and tracking our own bugs.

Well, it is part of Australis' final pass etherpad, I just filled the bug since there was no bug matching the feature. Also, it's not a random mockup, I found it on Australis Meeting Notes page.
(Reporter)

Comment 3

5 years ago
Stephen Horlander, do we need this bug ?
Status: RESOLVED → UNCONFIRMED
Flags: needinfo?(shorlander)
Resolution: INVALID → ---
Yes if we could get the animation shown in the URL it would be a great way to show the link between the two conjoined items. Madhava suggested maybe the animation and also glowing the menu icon blue was overkill.
Flags: needinfo?(shorlander) → needinfo?(madhava)
To get this back on the radar, this is something we'd like to do (and would particularly show the value and rationale for combining the star and list buttons). It's (slightly over-enthusiastically) mocked up here (click the star button):

http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html

We'll need visual assets, of course.
Flags: needinfo?(madhava)
(Assignee)

Updated

5 years ago
Assignee: nobody → dhenein
(Assignee)

Comment 6

5 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Could someone try this on windows and linux? I don't have VMs setup yet.
(Reporter)

Comment 8

5 years ago
(In reply to Darrin Henein [:darrin] from comment #6)
> Created attachment 8365196 [details] [diff] [review]
> Patch v1
> 
> Could someone try this on windows and linux? I don't have VMs setup yet
I'd love too, but how can I do it ?
(In reply to Tim Nguyen [:ntim] from comment #8)
> (In reply to Darrin Henein [:darrin] from comment #6)
> > Created attachment 8365196 [details] [diff] [review]
> > Patch v1
> > 
> > Could someone try this on windows and linux? I don't have VMs setup yet
> I'd love too, but how can I do it ?

You'll need clone mozilla-central, apply the patch, and then compile Nightly on those systems.

Here are build instructions for each major platform: https://developer.mozilla.org/en/docs/Simple_Firefox_build
(Reporter)

Comment 10

5 years ago
(In reply to Mike Conley (:mconley) from comment #9)
> (In reply to Tim Nguyen [:ntim] from comment #8)
> > (In reply to Darrin Henein [:darrin] from comment #6)
> > > Created attachment 8365196 [details] [diff] [review]
> > > Patch v1
> > > 
> > > Could someone try this on windows and linux? I don't have VMs setup yet
> > I'd love too, but how can I do it ?
> 
> You'll need clone mozilla-central, apply the patch, and then compile Nightly
> on those systems.
> 
> Here are build instructions for each major platform:
> https://developer.mozilla.org/en/docs/Simple_Firefox_build

I already have Firefox cloned in my computer, but I made some local changes (preventing me from applying this patch), I don't know how to discard those.
(Assignee)

Comment 11

5 years ago
Smaller image dimensions reduce jagged edges seen in windows/linux, horizontal offset fixed in windows. Extra space removed in linux.
Attachment #8365196 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8367532 - Flags: review?(mak77)
(Assignee)

Comment 12

5 years ago
Comment on attachment 8367532 [details] [diff] [review]
bug-931343-australisBookmarkAnimation.patch

Wasn't sure if browser.css was the best place for the bookmark animation CSS, but there currently is no places/ stylesheets in themes. Most of the javascript is inspired by the download-finish animation in Australis.
Comment on attachment 8367532 [details] [diff] [review]
bug-931343-australisBookmarkAnimation.patch

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

If you need help testing this on differnt platforms please let me know, I have all tier1

::: browser/base/content/browser-places.js
@@ +1189,5 @@
>      // Ignore clicks on the star if we are updating its state.
>      if (!this._pendingStmt) {
> +
> +      // only animate star if new bookmark
> +      if(this._itemIds.length == 0) {

space after if

since we do this check multiple times, please define at the beginning of this method a
let isBookmarked = this._itemIds.length > 0;

@@ +1191,5 @@
> +
> +      // only animate star if new bookmark
> +      if(this._itemIds.length == 0) {
> +
> +        if (this._notificationTimeout) {

all of the animation code should be moved to a local helper method, so that here we just end up having
if (!isBookmarked)
  this._showStarAnimation();

then you basically don't need any comments, since this code is self-describing

@@ +1195,5 @@
> +        if (this._notificationTimeout) {
> +          clearTimeout(this._notificationTimeout);
> +        }
> +
> +        let notifier = document.getElementById("bookmarks-notification-anchor");

please add a lazy this.notifier getter for this

@@ +1196,5 @@
> +          clearTimeout(this._notificationTimeout);
> +        }
> +
> +        let notifier = document.getElementById("bookmarks-notification-anchor");
> +        let anchor = document.getElementById("bookmarks-menu-button");

this.button?

@@ +1207,5 @@
> +          let widthDiff = anchorRect.width - notifierRect.width;
> +          let translateX = (leftDiff + .5 * widthDiff) + "px";
> +          let translateY = (topDiff + .5 * heightDiff) + "px";
> +          notifier.style.transform = "translate(" +  translateX + ", " + translateY + ")";
> +        }

did you test this when the button is in the overflow panel? does it still work properly?

::: browser/base/content/browser.xul
@@ +414,5 @@
>      </hbox>
> +
> +    <hbox id="bookmarks-animation-container" mousethrough="always">
> +      <vbox id="bookmarks-notification-anchor">
> +        <vbox id="bookmarks-indicator-notification"/>

so, in one place we have: animation, notification, indicator... I suggest going with notification, so bookmarks-notification-container/anchor, and bookmarks-notification

::: browser/themes/osx/browser.css
@@ +4100,5 @@
> +}
> +
> +@media (min-resolution: 2dppx) {
> +  #bookmarks-notification-anchor[notification="finish"] > #bookmarks-indicator-notification {
> +    background-image: url("chrome://browser/skin/places/bookmarks-notification-finish@2x.png");

don't you have to set a background-size here?

::: browser/themes/windows/browser.css
@@ +2430,5 @@
>  }
>  
>  %include ../shared/UITour.inc.css
> +
> +/* ----- BOOKMARK STAR ANIMATION ----- */

please move this closer to #bookmarks-menu-button styling, we should try to never blindly append to browser.css

@@ +2441,5 @@
> +}
> +@keyframes animation-bookmarkPulse {
> +  from { transform: scale(1); }
> +  50%   { transform: scale(1.3);}
> +  to   { transform: scale(1);}

please be consistent with spacing everywhere, either you do { ... } or {...}, not a mixup

@@ +2456,5 @@
> +
> +#bookmarks-indicator-anchor {
> +  min-width: 20px;
> +  min-height: 20px;
> +}

is this rule actually needed on all 3 platforms?
At first glance looks like the style has just been copy/pasted across the 3 themes, but I think some of the rules don't apply... do you need help testing on some platforms?
Attachment #8367532 - Flags: review?(mak77)
To be even more cohere I suggest
_showBookmarkedNotfication for the method, and bookmarked-notification-...
(Assignee)

Comment 15

5 years ago
Attachment #8367532 - Attachment is obsolete: true
Attachment #8368724 - Flags: review?(mak77)
Comment on attachment 8368724 [details] [diff] [review]
bug-931343-australisBookmarkAnimation.patch v2

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

The changes in browser-places may conflict with the changes in bug 964887, since that patch is larger I suggest to queue for landing after it, you may coordinate with Gijs for that, should not be hard to solve the conflicts though, since you are touching different code pieces.

did you remember to compress the png files? Ask shorlander in case, I think he uses ImageOptim for that

Just for my future reference, I was unsure why we are using a blue star across all of the platforms, but I asked in #ux and sounds like that's by design (there's a plan to change the star colors)

r=me with the following changes:

::: browser/base/content/browser-places.js
@@ +954,5 @@
> +  get notifier()
> +  {
> +    return this._notifier ||
> +      (this._notifier = document.getElementById("bookmarked-notification-anchor"));
> +  },

please use the lazy getter form we already use in this object:

delete this.notifier;
return this.notifier = document...

@@ +1164,5 @@
>        button.removeAttribute("starred");
>        button.setAttribute("buttontooltiptext", this._unstarredTooltip);
>      }
>    },
> +  _showBookmarkedAnimation: function BUI_showBookmarkedAnimation() {

add newline before the method

And please rename to _showBookmarkedNotification to be coherent with the xul naming (remember to fix the callpoints)

@@ +1171,5 @@
> +      clearTimeout(this._notificationTimeout);
> +    }
> +
> +    let notifier = this.notifier;
> +    let button = this.button;

with the patch in bug 964887 button becomes a lazy getter, as well as notifier is, so you can just use this.button and this.notifier everywhere without additional costs, there should be no need for these temp vars (you can pass an arrow function to setTimeout to avoid a .bind(this))

@@ +1188,5 @@
> +
> +    let isInOverflowPanel = button.classList.contains("overflowedItem");
> +    let isInBookmarksBar = button.classList.contains("bookmark-item");
> +
> +    if (isInBookmarksBar) notifier.setAttribute("bookmarks-bar", true);

we don't use single line ifs in browser code, the minimum is
if (isInBookmarksBar)
  notifier...

Also, please rename to isInBookmarksToolbar and the attribute to in-bookmarks-toolbar

@@ +1190,5 @@
> +    let isInBookmarksBar = button.classList.contains("bookmark-item");
> +
> +    if (isInBookmarksBar) notifier.setAttribute("bookmarks-bar", true);
> +
> +    if (!isInOverflowPanel) {

please move the definition of isInOverflowPanel just before its first use here

::: browser/themes/windows/jar.mn
@@ +119,1 @@
>          skin/classic/browser/places/bookmarksToolbar-menuPanel.png   (places/bookmarksToolbar-menuPanel.png)

please find a better location in the jar.mn(s) files, here it's between bookmarksToolbar.png  and bookmarksToolbar-menuPanel.png, those are related and thus should stay adjacent if possible.
Attachment #8368724 - Flags: review?(mak77) → review+
(Assignee)

Comment 17

5 years ago
Images had already been compressed using ImageOptim, thanks. I had run `hg pull` and `hg update` prior to making these changes, so the patch should be more current. Thanks for all your help mak!
Attachment #8368724 - Attachment is obsolete: true
Attachment #8369495 - Flags: review?(mak77)
(Assignee)

Updated

5 years ago
Attachment #8369495 - Flags: review?(mak77)

Comment 18

5 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/25bd95560c25
Status: NEW → ASSIGNED
Whiteboard: [Australis:M?][Australis:P5] → [Australis:P5][fixed-in-fx-team]
Version: 27 Branch → Trunk

Comment 19

5 years ago
(In reply to :Gijs Kruitbosch from comment #18)
> remote:   https://hg.mozilla.org/integration/fx-team/rev/25bd95560c25

This had a missing asset, which landed as

remote:   https://hg.mozilla.org/integration/fx-team/rev/99c2448a8697
https://hg.mozilla.org/mozilla-central/rev/25bd95560c25
https://hg.mozilla.org/mozilla-central/rev/99c2448a8697
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P5][fixed-in-fx-team] → [Australis:P5]
Target Milestone: --- → Firefox 29
Comment on attachment 8369652 [details] [diff] [review]
aero skin asset reference (for m-c and aurora) **commit message needs [Australis]

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

r=me
Attachment #8369652 - Flags: review+
Comment on attachment 8369652 [details] [diff] [review]
aero skin asset reference (for m-c and aurora) **commit message needs [Australis]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): missed a line in the jar.mn file change within this bug
User impact if declined: part of the animation is missing on windows+aero
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: none
Attachment #8369652 - Attachment description: aero skin asset reference → aero skin asset reference (for m-c and aurora)
Attachment #8369652 - Flags: approval-mozilla-aurora?
Attachment #8369652 - Attachment description: aero skin asset reference (for m-c and aurora) → aero skin asset reference (for m-c and aurora) **commit message needs [Australis]
Attachment #8369652 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 25

5 years ago
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/602d894f6c73
status-firefox29: --- → fixed
status-firefox30: --- → fixed

Comment 26

5 years ago
Huh, except that patch has apparently not made it to mc yet. Leaving fx30 status as before for now, then.
status-firefox30: fixed → ---
No longer blocks: 968228
Depends on: 968228

Updated

5 years ago
Blocks: 969904

Updated

5 years ago
Depends on: 972282

Updated

5 years ago
Depends on: 970013

Updated

5 years ago
Depends on: 977554

Updated

5 years ago
Depends on: 977551
Depends on: 979232

Updated

5 years ago
QA Contact: cornel.ionce
Verified fixed on Windows 7 64bit, Mac OS X 10.9 and Ubuntu 13.10 using latest Aurora (build ID: 20140307004002).
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified

Updated

5 years ago
Depends on: 983997

Updated

5 years ago
No longer depends on: 983997
Depends on: 1025915
(Reporter)

Updated

4 months ago
No longer blocks: 967663
You need to log in before you can comment on or make changes to this bug.