1155518 - Implement "Save to Pocket" context menu item

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Justin Dolske [:Dolske]]

When opening the context menu on a web page, the user should see a "Save Page to Pocket" (wording TBD) context menu entry, so that they can add the page to the Pocket service. 

If the user isn't signed in to the Pocket service, we should (probably) just suppress this context menu. The toolbar button and Reader View buttons will offer a signin/signup flow via a panel, but that doesn't seem like a good fit for a content menu. And the context menu isn't the primary UI for this feature, anyway.

Comment 1

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

(In reply to Justin Dolske [:Dolske] from comment #0)
> If the user isn't signed in to the Pocket service, we should (probably) just
> suppress this context menu. The toolbar button and Reader View buttons will
> offer a signin/signup flow via a panel, but that doesn't seem like a good
> fit for a content menu. And the context menu isn't the primary UI for this
> feature, anyway.

@mmaslaney: similarly again, hide this menu item when the user isn't signed in?

Comment 2

[Michael Maslaney [:mmaslaney] ([e-mail])]

Yes, let's hide it.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch

Comment 4

[Dão Gottwald [:dao]]

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

>+                accesskey="P"

already taken by "Save Page As..."

>--- a/browser/base/content/nsContextMenu.js
>+++ b/browser/base/content/nsContextMenu.js
>@@ -224,16 +224,23 @@
>     // SimpleServiceDiscovery.services), but SimpleServiceDiscovery is guaranteed
>     // to be already loaded, since we load it on startup in nsBrowserGlue,
>     // and CastingApps isn't, so check SimpleServiceDiscovery.services first
>     // to avoid needing to load CastingApps.jsm if we don't need to.
>     shouldShowCast = shouldShowCast && this.mediaURL &&
>                      SimpleServiceDiscovery.services.length > 0 &&
>                      CastingApps.getServicesForVideo(this.target).length > 0;
>     this.setItemAttr("context-castvideo", "disabled", !shouldShowCast);
>+
>+    let uri = gBrowser.currentURI;

nsContextMenu is also used outside of tabbrowser, so this doesn't seem correct.

Comment 5

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [:dao] from comment #4)

> >+
> >+    let uri = gBrowser.currentURI;
> 
> nsContextMenu is also used outside of tabbrowser, so this doesn't seem
> correct.

Do you have a specific example of how I can test this? We probably don't want this item to appear if we aren't in the tabbrowser case, as the Pocket toolbar button just uses URI of the current tab.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #4)
> 
> > >+
> > >+    let uri = gBrowser.currentURI;
> > 
> > nsContextMenu is also used outside of tabbrowser, so this doesn't seem
> > correct.
> 
> Do you have a specific example of how I can test this?

chatWindow.xul and web-panels.xul (for bookmarks opening in the side bar) use it:
http://mxr.mozilla.org/mozilla-central/search?string=new+nsContextMenu

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch v2

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8599814 [details] [diff] [review]
Patch v2

>+    if (shouldShow && window.gBrowser) {

AIUI, this is still not correct, since nsContextMenu can be used in browser.xul for a browser independent of tabbrowser.

>+      let uri = gBrowser.currentURI;
>+      canPocket =
>+        CustomizableUI.getPlacementOfWidget("pocket-button") &&
>+        (uri.schemeIs("http") || uri.schemeIs("https") ||
>+         (uri.schemeIs("about") && ReaderMode.getOriginalUrl(uri.spec)));
>+    }

You should use this.browser.currentURI

>+    this.showItem("context-pocket", canPocket && Pocket.isLoggedIn);

Where is Pocket defined?

Comment 9

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 8599814 [details] [diff] [review]
> Patch v2
> 
> >+    if (shouldShow && window.gBrowser) {
> 
> AIUI, this is still not correct, since nsContextMenu can be used in
> browser.xul for a browser independent of tabbrowser.

How do I detect that?

> >+      let uri = gBrowser.currentURI;
> >+      canPocket =
> >+        CustomizableUI.getPlacementOfWidget("pocket-button") &&
> >+        (uri.schemeIs("http") || uri.schemeIs("https") ||
> >+         (uri.schemeIs("about") && ReaderMode.getOriginalUrl(uri.spec)));
> >+    }
> 
> You should use this.browser.currentURI

I don't want to display this context menu item for a browser that wouldn't be associated with the urlbar, as the panel saying "the page has been saved" doesn't show which URL/page has been saved.

> >+    this.showItem("context-pocket", canPocket && Pocket.isLoggedIn);
> 
> Where is Pocket defined?

http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.js#9 which is included at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/global-scripts.inc#12

Comment 10

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Comment on attachment 8599814 [details] [diff] [review]
> > Patch v2
> > 
> > >+    if (shouldShow && window.gBrowser) {
> > 
> > AIUI, this is still not correct, since nsContextMenu can be used in
> > browser.xul for a browser independent of tabbrowser.
> 
> How do I detect that?

maybe check if window.gBrowser && this.browser.getTabBrowser() == window.gBrowser? Should probably check window.Pocket too since global-scripts.inc isn't actually global.

> > >+      let uri = gBrowser.currentURI;
> > >+      canPocket =
> > >+        CustomizableUI.getPlacementOfWidget("pocket-button") &&
> > >+        (uri.schemeIs("http") || uri.schemeIs("https") ||
> > >+         (uri.schemeIs("about") && ReaderMode.getOriginalUrl(uri.spec)));
> > >+    }
> > 
> > You should use this.browser.currentURI
> 
> I don't want to display this context menu item for a browser that wouldn't
> be associated with the urlbar, as the panel saying "the page has been saved"
> doesn't show which URL/page has been saved.

Yet there's no reason for using gBrowser when you already have the browser in a local property.

Comment 11

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

Comment on attachment 8599814 [details] [diff] [review]
Patch v2

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

::: browser/base/content/browser-context.inc
@@ +267,5 @@
>                  accesskey="&savePageCmd.accesskey2;"
>                  oncommand="gContextMenu.savePageAs();"/>
> +      <menuitem id="context-pocket"
> +                label="Save to Pocket"
> +                accesskey="k"

These strings need to be added to the browser-pocket.properties file.

::: browser/base/content/nsContextMenu.js
@@ +231,5 @@
>      this.setItemAttr("context-castvideo", "disabled", !shouldShowCast);
> +
> +    let canPocket = false;
> +    if (shouldShow && window.gBrowser) {
> +      let uri = gBrowser.currentURI;

This needs to also work to allow people to "pocket" a link, in which case we check if the href is valid.

Comment 12

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> Comment on attachment 8599814 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8599814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-context.inc
> @@ +267,5 @@
> >                  accesskey="&savePageCmd.accesskey2;"
> >                  oncommand="gContextMenu.savePageAs();"/>
> > +      <menuitem id="context-pocket"
> > +                label="Save to Pocket"
> > +                accesskey="k"
> 
> These strings need to be added to the browser-pocket.properties file.

We could leave this in here for 38.1, but I remember either Dolske or Gijs asking that strings be placed in a dtd/properties file.

Comment 13

[Justin Dolske [:Dolske]]

Yeah, even if we're not actually localizing for 38.0.5 (aka 38.1), we should still have things split out. I'd suggest creating a browser/base/content/browser-pocket.dtd ala the browser-pocket.properties from bug 1155523.

Comment 14

[Florian Quèze [:florian]]

Attachment: Patch v3

Requesting review from Justin instead of Jared, as bugzilla tells me Jared isn't accepting review requests.

Comment 15

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

To be consistent with the rest of the menus the final version of two strings I have for this are:

Save Link to Pocket
Save Page to Pocket

Comment 16

[Pulsebot]

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

Comment 17

[Florian Quèze [:florian]]

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #15)
> To be consistent with the rest of the menus the final version of two strings
> I have for this are:
> 
> Save Link to Pocket

Neither comment 0 nor any UX spec I've seen include this item. Also, given that the Pocket panel doesn't give any feedback about which URL has been saved, I don't think we should use it for URLs different from the one the user can see in the URL bar.

> Save Page to Pocket

I changed the wording to this before landing.

Comment 18

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

(In reply to Florian Quèze [:florian] [:flo] from comment #17)
> (In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #15)
> > To be consistent with the rest of the menus the final version of two strings
> > I have for this are:
> > 
> > Save Link to Pocket
> 
> Neither comment 0 nor any UX spec I've seen include this item. Also, given
> that the Pocket panel doesn't give any feedback about which URL has been
> saved, I don't think we should use it for URLs different from the one the
> user can see in the URL bar.

Ok, we can revisit this later in a separate bug.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c9f806d09c6

Comment 20

[Justin Dolske [:Dolske]]

Comment on attachment 8600910 [details] [diff] [review]
Patch v3

a+ for uplift in service of 38.0.5 spring launch campaign.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8600910 [details] [diff] [review]
Patch v3

[Triage Comment]
Fix the branch

Comment 22

[Andrei Vaida [:avaida]]

Confirmed fixed as of Nightly 40.0a1 (2015-05-06), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

* Clicking the context menu option saves the page in question to Pocket.
* If the user is not logged into Pocket, the "Save to Pocket" option is NOT displayed in the context menu.

Comment 23

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

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

Comment 24

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

https://hg.mozilla.org/releases/mozilla-release/rev/0a18ef5ab9b7

Comment 25

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed using Firefox 38.0.5 beta 1 build 2 under Ubuntu 14.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.

Comment 26

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing qe-verify flag as verification on Firefox 38.0.5 Beta and 40 Nightly should suffice.

Comment 27

[Josh Triplett]

The "Save to Pocket" context menu item does not seem to respect the "browser.pocket.enabled" config option; with "browser.pocket.enabled" set to false, the context menu still includes "Save to Pocket".

Comment 28

[Florian Quèze [:florian]]

(In reply to Josh Triplett from comment #27)
> The "Save to Pocket" context menu item does not seem to respect the
> "browser.pocket.enabled" config option; with "browser.pocket.enabled" set to
> false, the context menu still includes "Save to Pocket".

Please file a new bug for this; it's not the behavior we implemented here, so this may be a regression.

Comment 29

[Josh Triplett]

(In reply to Florian Quèze [:florian] [:flo] from comment #28)
> (In reply to Josh Triplett from comment #27)
> > The "Save to Pocket" context menu item does not seem to respect the
> > "browser.pocket.enabled" config option; with "browser.pocket.enabled" set to
> > false, the context menu still includes "Save to Pocket".
> 
> Please file a new bug for this; it's not the behavior we implemented here,
> so this may be a regression.

Done: just filed bug 1275924.  Please let me know if you need any more information.