Implement new animation for bookmarking

VERIFIED FIXED in Firefox 56

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Depends on 2 bugs, Blocks 1 bug)

52 Branch
Firefox 56
Points:
8
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

Posted 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.
Posted 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
Posted 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
Comment hidden (mozreview-request)
Iteration: --- → 56.3 - Jul 24
Priority: P3 → P1
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
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)
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
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.

Comment 8

2 years ago
(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 9

2 years ago
mozreview-review
Comment on attachment 8889079 [details]
Bug 1352063 - Implement new animation for bookmarking.

https://reviewboard.mozilla.org/r/160114/#review165760
Attachment #8889079 - Flags: review+
Comment hidden (mozreview-request)

Comment 11

2 years ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9202b42e8dcb
Implement new animation for bookmarking. r=Gijs
Comment hidden (mozreview-request)

Comment 15

2 years ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/767224f031ac
Implement new animation for bookmarking. r=Gijs

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/767224f031ac
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1385407

Updated

2 years ago
Depends on: 1386025

Comment 17

2 years ago
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+

Updated

2 years ago
Depends on: 1394082

Updated

2 years ago
Depends on: 1403550
No longer depends on: 1403550
You need to log in before you can comment on or make changes to this bug.