1162713 - Implement "Save Link to Pocket" context menu item

Status: VERIFIED FIXED

(Firefox :: Pocket, defect, P1)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: save link mockup

similar to bug 1155518 which implemented the "Save Page to Pocket" context menu.

This bug is about implementing a similar context menu when a user right clicks on a link that may not have been visited yet.

As mentioned in bug 1155518 comment 17 we currently don't tell the user what link was saved which may be an issue if we start saving links that aren't the current one you are looking at.

Michael, can take the design side of things first.  Determine if we need to make changes to the Save interface to support this context menu or if we nothing needs to be done.

Comment 1

[Justin Dolske [:Dolske]]

Oops, we don't actually have a "Save Link to Pocket" string, so we'll need get one localized to do this. (I think this got lost in some confusion around what the context menu was supposed to normally say.)

Comment 2

[Nate Weiner]

We have "Save Link to Pocket" translated on our platform, you can use these strings for now (pending review from localization):

en-US: "Save Link to Pocket"
de: "Link in Pocket speichern"
ja: "Pocket にリンクを保存"
ru: "Сохранить ссылку в Pocket"
es: "Guardar enlace en Pocket"

Comment 3

[Andrei Vaida [:avaida]]

So I guess this is fixed since I'm already seeing the option in the context menu of eligible pages. Any other requirements QA should know about for this functionality?

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Andrei Vaida, QA [:avaida] from comment #3)
> So I guess this is fixed since I'm already seeing the option in the context
> menu of eligible pages. Any other requirements QA should know about for this
> functionality?

"Save Page to Pocket" should be already working but this option is for right clicking on links you may not have visited yet.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Nate, the context menu for saving the current page to Pocket works by triggering a button press on the Pocket button. In the case of a link, we can't take the same approach since the Pocket code will try to get the URL from the current page.

How should we pass the URL of the link to the Pocket code and get the panel to use the URL of the link?

Comment 6

[Nate Weiner]

Jared, call this method in main.js: 

tryToSaveUrl(url) 

Where url is the link.

--

Note, if it's helpful, there is an unused method "pocketContextSaveLinkOnCommand" from our implementation of the right-click save in the add-on. In that implementation we set the context menuitem's oncommand to call: oncommand="pktUI.pocketContextSaveLinkOnCommand(event);"

In that method the trick is to bubble up from the event target to find the <a> element, since sometimes the target may have been a child that doesn't have the href.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 8604760 [details] [diff] [review]
Patch

Review of attachment 8604760 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-pocket-es-ES.properties
@@ +11,5 @@
>  # From browser-pocket.dtd
>  saveToPocketCmd.label = Guardar página en Pocket
>  saveToPocketCmd.accesskey = k
> +saveLinkToPocketCmd.label = Guardar enlace en Pocket
> +saveLinkToPocketCmd.accesskey = k

Note that this is the same key as the one right above it?

::: browser/base/content/nsContextMenu.js
@@ +200,5 @@
>      }
> +    let targetURI = (this.onSaveableLink || this.onPlainTextLink) ? this.linkURI : this.browser.currentURI;
> +    let canSaveToPocket = canPocket && window.pktApi && window.pktApi.isUserLoggedIn() &&
> +                          (targetURI.schemeIs("http") || targetURI.schemeIs("https") ||
> +                           (targetURI.schemeIs("about") && ReaderMode.getOriginalUrl(uri.spec)));

This is a little dense, how about moving the "window.pktApi && window.pktApi.isUserLoggedIn()" above, where we check for the widget placement?

Then the previous check is "user has button and is logged in", and this check is just "uri is something we can save".

And avoid introducing |canSaveToPocket| by just doing "canPocket = canPocket && ..."?

::: browser/components/pocket/Pocket.jsm
@@ +27,1 @@
>      let document = event.target.ownerDocument;

target is a little too vague here (especially with event.target right below it).

I think |menuitem| works better here?

@@ +31,5 @@
>      // ViewShowing fires immediately before it creates the contents,
>      // in lieu of an AfterViewShowing event, just spin the event loop.
>      window.setTimeout(function() {
> +      if (target && target.id == "context-savelinktopocket") {
> +        window.pktUI.tryToSaveUrl(target.getAttribute("data-linkurl"));

We should clear this at some point, even if the menu isn't triggered, so it doesn't hang around. (Especially for Private Browsing.)

Using a data- attribute is clever but almost too gross for me to take. :)

What do you think of this. It's a little more work, but seems saner, by keeping all the gross bits wholly in Pocket.jsm, and makes things a little more explicit:

1) Cu.import Pocket.jsm into nsContentMenu
2) Create Pocket.savePage(uri) and Pocket.saveLink(uri) methods
3) Move current gContextMenu.savePageToPocket() code into #2 (or a common helper)
4) Change gContextMenu.savePageToPocket to a simple wrapper calling Pocket.savePage(this.browser.currentURI)
5) Add gContextMenu.saveLinkToPocket, as a simple wrapper calling Pocket.saveLink(this.linkURI)

Then, when the functions in #2 are called, they can stash URI argument into a private "my-uri-brb-yolo" var within Pocket.jsm, and call the doCommand helper from #3. Milliseconds later Pocket.onPanelViewShowing() will be called, and it would do:

  onPanelViewShowing(event) {
    ///
    window.setTimeout(function() {
      // NO MORE window.pktUI.pocketButtonOnCommand();
      let uriToSave, title;
      if (my-uri-brb-yolo) {
        uriToSave = my-uri-brb-yolo;
        my-uri-brb-yolo = null;
      } else {
        uriToSave = mumble.gBrowser.currentURI.spec;
        title = mumble.gBrowser.contentTitle too
      }
      window.pktUI.tryToSaveUrl(uriToSave, title);
      ...

The existing pocketButtonOnCommand() would be deadwood, and Pocket.jsm would be responsible for telling Pocket what to save explicitly.

I suppose another way of thinking about this is it's using Pocket.jsm to hold some ephemeral state, instead of an attribute inside the context menu. This also enables saving any URL from anywhere, without having to add more data attribute hacks.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: My idea (utterly untested)

One more small comment -- does your current code ensure we only show "Save Page" -or- "Save Link"? Seems like we shouldn't save both, for consistency with how the menu currently works.

Comment 11

[Justin Dolske [:Dolske]]

If this is too broken / a terrible idea, let's go with yours.

Comment 12

[Nate Weiner]

(In reply to Justin Dolske [:Dolske] from comment #10)
> Created attachment 8604955 [details] [diff] [review]
> My idea (utterly untested)
> 
> One more small comment -- does your current code ensure we only show "Save
> Page" -or- "Save Link"? Seems like we shouldn't save both, for consistency
> with how the menu currently works.

Drive by, for additional context, here is what have in our current extensions to determine when to show the options in the context menu:

if ( (gContextMenu.onSaveableLink || ( gContextMenu.inDirList && gContextMenu.onLink )) ) {
// show save link
} else if (gContextMenu.isTextSelected) {
// show no menus
} else if (!gContextMenu.onTextInput) {
// show save page
} else {
// show no menus
}

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 8604760 [details] [diff] [review]
Patch

Talked with Jared earlier - going with my WIP patch, he will test & update.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 1162713.patch

pktUI.pocketButtonOnCommand is still needed because it is used for the logged out case.

Comment 15

[Justin Dolske [:Dolske]]

Fading so I'm not going to mark r+ tonight, but all I really wanted to check in the morning is the showSaveCurrentPageToPocket logic in nsContextMenu.

Comment 16

[Justin Dolske [:Dolske]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a52f66c4e80

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 8605616 [details] [diff] [review]
1162713.patch

Review of attachment 8605616 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +187,5 @@
> +
> +  initPocketItems: function CM_initPocketItems() {
> +    var showSaveCurrentPageToPocket = !(this.onTextInput || this.onLink ||
> +                                        this.isContentSelected || this.onImage ||
> +                                        this.onCanvas || this.onVideo || this.onAudio);

Ah, I see, same as initSaveItems(). Part of me wants to make this a common helper, but we're already pretty inconsistent and often need to make subtle changes anyway.

@@ +190,5 @@
> +                                        this.isContentSelected || this.onImage ||
> +                                        this.onCanvas || this.onVideo || this.onAudio);
> +    let canPocket = CustomizableUI.getPlacementOfWidget("pocket-button") &&
> +                    window.pktApi && window.pktApi.isUserLoggedIn();
> +    let targetURI = (this.onSaveableLink || this.onPlainTextLink) ? this.linkURI : this.browser.currentURI;

Nit: moving this up before the |let canPocket ...| would help with readability because all the canPocket stuff is then grouped together.

::: browser/components/pocket/Pocket.jsm
@@ +31,5 @@
>      // in lieu of an AfterViewShowing event, just spin the event loop.
> +    let urlToSave = Pocket._urlToSave;
> +    let titleToSave = Pocket._titleToSave;
> +    Pocket._urlToSave = null;
> +    Pocket._titleToSave = null;

Nit: I'd move this before the ViewShowing comment, and maybe add a comment (here or in savePage) to help explain the weird thing we're doing here. Perhaps just refer to the bug #. ;)

Comment 18

[Justin Dolske [:Dolske]]

Hmm, this isn't actually working for me. Using the Save Page To Pocket context menu gives me:

JavaScript error: chrome://browser/content/nsContextMenu.js, line 1672: TypeError: this.currentURI is undefined

That line is is savePageToPocket() in my build.

Comment 19

[Justin Dolske [:Dolske]]

Saving a link works when the button is in the toolbar, but when it's in the menupanel I get:

JavaScript error: resource:///modules/Pocket.jsm, line 105: ReferenceError: PanelUI is not defined

That line is in the savePage() added in this patch.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/d5a378327e88

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for uplift

Approval Request Comment
[Feature/regressing bug #]: pocket
[User impact if declined]: links in pages can't be added to pocket
[Describe test coverage new/current, TreeHerder]: landed on fx-team
[Risks and why]: makes changes to context menus by adding a new item. low-risk
[String/UUID change made/needed]: strings added for the context menu items, but the strings are part of the pocket specific localization files and have been translated by Pocket

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 8605937 [details] [diff] [review]
Patch for uplift

a+ for uplift to aurora/beta/release; required for Pocket 38.0.5 launch.

(Jared clarified that comment 18 & 19 were fixed in his landing.)

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5a378327e88

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b03c5f1a77a1

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c9efcb135425

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/e13a7a312aa5

Comment 27

[Andrei Vaida [:avaida]]

Verified fixed on 38.0.5b3 (20150518141916), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

One new issue was found during the verification of this fix - Bug 1166699. I'd also like to mention that this feature is also affected by Bug 1160407.