Implement new animation for bookmarking

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
RESOLVED FIXED
5 months ago
15 days ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Depends on: 5 bugs, Blocks: 1 bug)

52 Branch
Firefox 56
Points:
8
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 8852901 [details]
Bookmarking.mp4

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.

Comment 1

5 months ago
Created attachment 8852992 [details]
Bookmarking.mp4

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).

Updated

5 months ago
Attachment #8852901 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Depends on: 1352110
(Assignee)

Updated

5 months ago
Depends on: 1352120

Updated

5 months ago
Whiteboard: [photon]
(Assignee)

Updated

5 months ago
Depends on: 1353436
(Assignee)

Updated

5 months ago
Depends on: 1353437
(Assignee)

Updated

4 months ago
Whiteboard: [photon] → [photon-animation]
(Assignee)

Updated

4 months ago
Points: --- → 8

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: jwilliams

Comment 2

4 months ago
Created attachment 8863486 [details]
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

Updated

3 months ago
Priority: P2 → P3
Whiteboard: [photon-animation] → [reserve-photon-animation]
(Assignee)

Updated

2 months ago
Depends on: 1373479
(Assignee)

Updated

2 months ago
Priority: P3 → P2

Updated

2 months ago
Priority: P2 → P3
Whiteboard: [reserve-photon-animation] → [reserve-photon-animation] [p1]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

26 days ago
Iteration: --- → 56.3 - Jul 24
Priority: P3 → P1
Comment hidden (mozreview-request)

Comment 6

25 days 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

25 days 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

25 days 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

25 days 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

25 days ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9202b42e8dcb
Implement new animation for bookmarking. r=Gijs
Backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=117052604&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/310ce703360639bf7e866e0e79b074cc2355e039
Flags: needinfo?(jaws)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30f651b6bb0e8e739abcaaa03ff3675d401fd849
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)

Comment 15

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

Comment 16

24 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/767224f031ac
Status: ASSIGNED → RESOLVED
Last Resolved: 24 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

24 days ago
Depends on: 1384145

Updated

23 days ago
Depends on: 1384268
Depends on: 1385262
Depends on: 1385407

Updated

17 days ago
Depends on: 1386025

Comment 17

17 days 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: 1386745
Depends on: 1384953
You need to log in before you can comment on or make changes to this bug.