Closed Bug 1162713 Opened 9 years ago Closed 9 years ago

Implement "Save Link to Pocket" context menu item

Categories

(Firefox :: Pocket, defect, P1)

defect
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 41
Iteration:
41.1 - May 25
Tracking Status
firefox38.0.5 --- verified
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: clarkbw, Assigned: jaws)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached image save link mockup (obsolete) —
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.
Blocks: Pocket
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.)
Priority: -- → P3
Flags: qe-verify+
QA Contact: andrei.vaida
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"
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?
Flags: needinfo?(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.
Flags: needinfo?(clarkbw)
Priority: P3 → P1
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 41.1 - May 25
Flags: firefox-backlog+
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?
Flags: needinfo?(nate)
Points: --- → 3
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.
Flags: needinfo?(nate)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8604756 - Flags: review?(dolske)
Attachment #8604756 - Attachment is obsolete: true
Attachment #8604756 - Flags: review?(dolske)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8602970 - Attachment is obsolete: true
Attachment #8604760 - Flags: review?(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.
Attached patch My idea (utterly untested) (obsolete) — Splinter Review
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.
If this is too broken / a terrible idea, let's go with yours.
(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 on attachment 8604760 [details] [diff] [review]
Patch

Talked with Jared earlier - going with my WIP patch, he will test & update.
Attachment #8604760 - Attachment is obsolete: true
Attachment #8604760 - Flags: review?(dolske)
Attached patch 1162713.patchSplinter Review
pktUI.pocketButtonOnCommand is still needed because it is used for the logged out case.
Attachment #8604955 - Attachment is obsolete: true
Attachment #8605616 - Flags: review?(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 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 #. ;)
Attachment #8605616 - Flags: review?(dolske) → review+
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.
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.
Attached patch Patch for upliftSplinter Review
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
Attachment #8605937 - Flags: approval-mozilla-release?
Attachment #8605937 - Flags: approval-mozilla-beta?
Attachment #8605937 - Flags: approval-mozilla-aurora?
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.)
Attachment #8605937 - Flags: approval-mozilla-release?
Attachment #8605937 - Flags: approval-mozilla-release+
Attachment #8605937 - Flags: approval-mozilla-beta?
Attachment #8605937 - Flags: approval-mozilla-beta+
Attachment #8605937 - Flags: approval-mozilla-aurora?
Attachment #8605937 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/d5a378327e88
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.