Closed Bug 1352063 Opened 7 years ago Closed 7 years ago

Implement new animation for bookmarking

Categories

(Firefox :: Theme, enhancement, P1)

52 Branch
enhancement
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.3 - Jul 24
Tracking Status
firefox56 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

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

Attachments

(2 files, 2 obsolete files)

Attached video Bookmarking.mp4 (obsolete) —
The bookmark button should be split to two separate buttons: a star inside of the URL bar, and a separate button for the "library" outside of the URL bar.

When the bookmark star is clicked, there should be an animation of the star as well as an accompanying animation of the library "adding another book".

See the attached MP4 for a demonstration of how this should look.
Attached video Bookmarking.mp4 (obsolete) —
Update to Bookmarking animation based on initial user testing results. Feedback was that users didn't know where to edit after they saved a bookmark. A confirmation/edit door hanger now appears after the user bookmarks. The door hanger will disappear on it's own after a 3 second delay (See updated MP4 for reference).
Attachment #8852901 - Attachment is obsolete: true
Depends on: 1352110
Depends on: 1352120
Whiteboard: [photon]
Depends on: 1353436
Depends on: 1353437
Whiteboard: [photon] → [photon-animation]
Points: --- → 8
Flags: qe-verify+
Priority: -- → P2
QA Contact: jwilliams
Attached video Bookmarking.mp4
Updated Bookmarking Animation based on user testing results. Star dropping into library is now triggered after bookmark door hanger is closed
Attachment #8852992 - Attachment is obsolete: true
Priority: P2 → P3
Whiteboard: [photon-animation] → [reserve-photon-animation]
Depends on: 1373479
Priority: P3 → P2
Priority: P2 → P3
Whiteboard: [reserve-photon-animation] → [reserve-photon-animation] [p1]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P3 → P1
Comment on attachment 8889079 [details]
Bug 1352063 - Implement new animation for bookmarking.

https://reviewboard.mozilla.org/r/160114/#review165690

This is almost finished, but I'm curious about the pocket reuse question and the breakage for 1374477, and if you have thoughts on that.

::: browser/base/content/browser-places.js:1953
(Diff revision 3)
> +        if (AppConstants.MOZ_PHOTON_ANIMATIONS) {
> +          this.star.setAttribute("preloadanimations", "true");

We can remove the listener immediately after this, right?

::: browser/base/content/browser.xul:913
(Diff revision 3)
>                  <image id="reader-mode-button"
>                         class="urlbar-icon"
>                         hidden="true"
>                         onclick="ReaderParent.buttonClick(event);"/>
>  #ifdef MOZ_PHOTON_THEME
> +                <hbox id="star-button-box">

This looks like it's set to break with bug 1374477. :-(

::: browser/themes/shared/toolbarbutton-icons.inc.css:488
(Diff revision 3)
> +
> +#library-button[animate="bookmark"] > .toolbarbutton-icon {
> +  fill: transparent;
> +}
> +
> +#library-button[animate="bookmark"] > .toolbarbutton-animatable-box {

Can't we reuse bits of the pocket-related rules here?
Attachment #8889079 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8889079 [details]
Bug 1352063 - Implement new animation for bookmarking.

https://reviewboard.mozilla.org/r/160114/#review165690

> We can remove the listener immediately after this, right?

Sure thing.

> This looks like it's set to break with bug 1374477. :-(

Okay, thanks for the heads up. I will have to find if there are customization events related to the moving of it and add/remove or hide these elements when necessary.

> Can't we reuse bits of the pocket-related rules here?

I intentionally didn't do this because of the way that Pocket is developed. Since Pocket is a system add-on I believe all of its functionality should be self-contained. If we removed the bookmark-animation I wouldn't want to break Pocket for example.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> > This looks like it's set to break with bug 1374477. :-(
> 
> Okay, thanks for the heads up. I will have to find if there are
> customization events related to the moving of it and add/remove or hide
> these elements when necessary.

Ah, this reminds me we will need to hide the element rather than remove it permanently, I think.

> > Can't we reuse bits of the pocket-related rules here?
> 
> I intentionally didn't do this because of the way that Pocket is developed.
> Since Pocket is a system add-on I believe all of its functionality should be
> self-contained. If we removed the bookmark-animation I wouldn't want to
> break Pocket for example.

Good point.
Comment on attachment 8889079 [details]
Bug 1352063 - Implement new animation for bookmarking.

https://reviewboard.mozilla.org/r/160114/#review165760
Attachment #8889079 - Flags: review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9202b42e8dcb
Implement new animation for bookmarking. r=Gijs
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/767224f031ac
Implement new animation for bookmarking. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/767224f031ac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1385407
Depends on: 1386025
I have seen this "animation for bookmarking" implementation with Latest Nightly  56.0a1 on Windows 8.1 (64 Bit).

Build ID : 20170731175927
User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170726]
Depends on: 1384953
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1394082
Depends on: 1403550
No longer depends on: 1403550
You need to log in before you can comment on or make changes to this bug.