1472472 - Stop calling PlacesCommandHook.bookmarkPage from NewTabUtils.jsm

Status: VERIFIED FIXED

(Firefox :: New Tab Page, enhancement)

Description

[Dão Gottwald [:dao]]

Can we remove this stuff? https://searchfox.org/mozilla-central/rev/4074ba403219b7accdf00220b20dc492bfd4d83e/toolkit/modules/NewTabUtils.jsm#1285-1304

This seems unused, i.e. from what I can tell there's no UI that would end up calling this. This is also the only call site using PlacesCommandHook.bookmarkPage's aUrl and aTitle arguments so I'd like to simplify that.

Comment 1

[Ed Lee :Mardak]

`activityStreamLinks.addBookmark` is being used by activity stream:

https://searchfox.org/mozilla-central/search?q=activityStreamLinks.addBookmark
https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/PlacesFeed.jsm#297

BOOKMARK_URL is dispatched from content from pocket and highlight cards:
https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/content-src/lib/link-menu-options.js#43-51

Is there a different API that we should use to add a bookmark with a url and title?

Comment 2

[Dão Gottwald [:dao]]

(In reply to Ed Lee :Mardak from comment #1)
> `activityStreamLinks.addBookmark` is being used by activity stream:
> 
> https://searchfox.org/mozilla-central/search?q=activityStreamLinks.
> addBookmark
> https://searchfox.org/mozilla-central/source/browser/extensions/activity-
> stream/lib/PlacesFeed.jsm#297
> 
> BOOKMARK_URL is dispatched from content from pocket and highlight cards:
> https://searchfox.org/mozilla-central/source/browser/extensions/activity-
> stream/content-src/lib/link-menu-options.js#43-51
> 
> Is there a different API that we should use to add a bookmark with a url and
> title?

There's PlacesCommandHook.bookmarkLink which opens a dialog rather than an arrow panel; the former makes more sense here since AUIU, you're not bookmarking the current page. Does this make sense?

Comment 3

[Dão Gottwald [:dao]]

Attachment: Bug 1472472 - Use PlacesCommandHook.bookmarkLink instead of PlacesCommandHook.bookmarkPage in activity stream.

Review commit: https://reviewboard.mozilla.org/r/254104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254104/

Comment 4

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #3)
> Created attachment 8989019 [details]
> Bug 1472472 - Use PlacesCommandHook.bookmarkLink instead of
> PlacesCommandHook.bookmarkPage in activity stream.
> 
> Review commit: https://reviewboard.mozilla.org/r/254104/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/254104/

This is untested since I'm not sure where to test this...

Comment 5

[Ed Lee :Mardak]

(In reply to Dão Gottwald [::dao] from comment #4)
> This is untested since I'm not sure where to test this...
I applied the patch and ./mach build browser/app (artifact) then clicked the (...) of a history highlight or pocket story (although the latter might not show up for you unless using de in DE)

Comment 6

[Ed Lee :Mardak]

Attachment: bookmark doorhanger vs modal

Amy, what's the expected behavior for bookmarking from activity stream now that bug 1460248 has an updated doorhanger? I've attached a screenshot of what it looks like without the patch (which is the current behavior in beta 62) and with the current patch making it a window modal.

A couple interaction notes is that with the doorhanger, it automatically dismisses with a followup Saved to library text and animation. With the modal, it must be interacted with and prevents switching tabs or clicking in the page, etc; and clicking save does not trigger the library text or animation.

Neither seem quite right…

Comment 7

[Ed Lee :Mardak]

Amy, I believe there are designs for other activity stream related interactions that open doorhangers from the address bar.. including saving to pocket, pinning pages, more? Not sure if they all should be consistent or the way users would trigger them are different enough to have different UIs.

Comment 8

[Dão Gottwald [:dao]]

Important things to note:
- The bookmark panel wasn't meant to support this, e.g. we have no good anchor here. The star is supposed to be tied to the current page.
- This is more broken now as we added the page screenshot and favicon to the panel.
- The dialog that my patch uses is also used for e.g. for the Bookmark This Link command that you get when right clicking a link.
- Once we use the dialog consistently, may want to look into improving that so that all consumers of the API benefit.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8989019 [details]
Bug 1472472 - Use PlacesCommandHook.bookmarkLink instead of PlacesCommandHook.bookmarkPage in activity stream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254104/diff/1-2/

Comment 10

[Amy Lee [:amylee] UX]

It should automatically time-out and save to Library if the user opens the door-hanger and does nothing (See animation reference that I attached). If the user clicks outside the door-hanger it should also have the same result. NI Aaron to confirm on this since he worked on the UX spec.  

(In reply to Ed Lee :Mardak from comment #6)
> Created attachment 8989037 [details]
> bookmark doorhanger vs modal
> 
> Amy, what's the expected behavior for bookmarking from activity stream now
> that bug 1460248 has an updated doorhanger? I've attached a screenshot of
> what it looks like without the patch (which is the current behavior in beta
> 62) and with the current patch making it a window modal.
> 
> A couple interaction notes is that with the doorhanger, it automatically
> dismisses with a followup Saved to library text and animation. With the
> modal, it must be interacted with and prevents switching tabs or clicking in
> the page, etc; and clicking save does not trigger the library text or
> animation.
> 
> Neither seem quite right…

Comment 11

[Amy Lee [:amylee] UX]

Attachment: Better Bookmarking FINAL.mp4

Animation reference - note the order of animation. Star drops in library and confirmation appears after. The library icon and confirmation message fades out at the same time in the span of 200ms.

Comment 12

[Amy Lee [:amylee] UX]

(In reply to Ed Lee :Mardak from comment #7)
> Amy, I believe there are designs for other activity stream related
> interactions that open doorhangers from the address bar.. including saving
> to pocket, pinning pages, more? Not sure if they all should be consistent or
> the way users would trigger them are different enough to have different UIs.

Save to pocket and bookmarking should have the same behavior (and Pocket should have a confirmation message as well). Will review the UI for other interactions.

Comment 13

[Amy Lee [:amylee] UX]

Also I noticed that when you bookmark/pocket an item and the library animation is triggered, you can't open the library door-hanger until the animation is finished.

Comment 14

[Ed Lee :Mardak]

Is it expected to have a preview screenshot when bookmarking from activity stream? I suppose the pocket cards will generally have an image but not necessarily highlights, so unclear how we would pass an image to the bookmark doorhanger.

Comment 15

[Marco Bonardo [:mak]]

I also concur we should use bookmarkLink here, and not the star panel, the star panel is intended to refer to the current page. If we don't like the dialog design we should rather change that one, imo.

Comment 16

[Ed Lee :Mardak]

Comment on attachment 8989019 [details]
Bug 1472472 - Use PlacesCommandHook.bookmarkLink instead of PlacesCommandHook.bookmarkPage in activity stream.

https://reviewboard.mozilla.org/r/254104/#review261930

Design says this is fine for now. Just make sure there's a bug filed to update `bookmarkLink` to be a doorhanger instead of modal more like new `bookmarkPage` behavior.

Comment 17

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s fd0cb814960c99faaba1974a15be2ddf80570494 -d 1ca992fccdc7: rebasing 471398:fd0cb814960c "Bug 1472472 - Use PlacesCommandHook.bookmarkLink instead of PlacesCommandHook.bookmarkPage in activity stream. r=Mardak" (tip)
merging toolkit/modules/NewTabUtils.jsm
warning: conflicts while merging toolkit/modules/NewTabUtils.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8989019 [details]
Bug 1472472 - Use PlacesCommandHook.bookmarkLink instead of PlacesCommandHook.bookmarkPage in activity stream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254104/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31115ed412b5
Use PlacesCommandHook.bookmarkLink instead of PlacesCommandHook.bookmarkPage in activity stream. r=Mardak

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/31115ed412b5

Comment 21

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4238

Comment 22

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/ad0e9fc33d23d94c0ca75c0726f9827294b67759
chore(mc): Port  Bug 1472472 - Use PlacesCommandHook.bookmarkLink instead of PlacesCommandHook.bookmarkPage in activity stream. r=Mardak (#4238)

Comment 23

[Cristian Fogel [:cfogel]]

Verified the implementation with Ffox 63.0b12 on win10x4, macOS 10.11, Ubuntu 16.04.

Noticed an issue with the animation order, noted down in bug 1497561.