Closed Bug 1472472 Opened 6 years ago Closed 6 years ago

Stop calling PlacesCommandHook.bookmarkPage from NewTabUtils.jsm

Categories

(Firefox :: New Tab Page, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 --- verified

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.
Flags: needinfo?(edilee)
`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?
Flags: needinfo?(edilee)
(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?
Flags: needinfo?(edilee)
(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...
Flags: needinfo?(edilee)
Summary: Remove PlacesCommandHook.bookmarkPage call site from NewTabUtils.jsm → Stop calling PlacesCommandHook.bookmarkPage from NewTabUtils.jsm
(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)
Depends on: 1460248
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…
Flags: needinfo?(amlee)
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.
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.
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…
Flags: needinfo?(amlee) → needinfo?(abenson)
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.
(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.
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.
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.
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 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.
Attachment #8989019 - Flags: review?(edilee) → review+
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)
Depends on: 1473073
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(abenson)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31115ed412b5
Use PlacesCommandHook.bookmarkLink instead of PlacesCommandHook.bookmarkPage in activity stream. r=Mardak
Blocks: 1473861
Blocks: 1473862
https://hg.mozilla.org/mozilla-central/rev/31115ed412b5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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)
See Also: → 1497561
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1498236
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.