Closed Bug 1155518 Opened 5 years ago Closed 5 years ago

Implement "Save to Pocket" context menu item

Categories

(Firefox :: Menus, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox38.0.5 --- verified
firefox39 --- fixed
firefox40 --- verified

People

(Reporter: Dolske, Assigned: florian)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: Pocket
Flags: qe-verify?
Flags: firefox-backlog+
(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?
Flags: needinfo?(mmaslaney)
Yes, let's hide it.
Flags: needinfo?(mmaslaney)
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Assignee: nobody → florian
Points: --- → 3
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Attached patch Patch (obsolete) — Splinter Review
Attachment #8599356 - Flags: review?(jaws)
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.
Attachment #8599356 - Flags: review?(jaws) → review-
(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.
Flags: needinfo?(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
Flags: needinfo?(dao)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8599356 - Attachment is obsolete: true
Attachment #8599814 - Flags: review?(jaws)
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?
Attachment #8599814 - Flags: review?(jaws) → review-
(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
Flags: needinfo?(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.
Flags: needinfo?(dao)
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.
(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.
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.
Attached patch Patch v3Splinter Review
Requesting review from Justin instead of Jared, as bugzilla tells me Jared isn't accepting review requests.
Attachment #8599814 - Attachment is obsolete: true
Attachment #8600910 - Flags: review?(dolske)
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
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/0c9f806d09c6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8600910 [details] [diff] [review]
Patch v3

a+ for uplift in service of 38.0.5 spring launch campaign.
Attachment #8600910 - Flags: approval-mozilla-beta+
Attachment #8600910 - Flags: approval-mozilla-aurora+
Comment on attachment 8600910 [details] [diff] [review]
Patch v3

[Triage Comment]
Fix the branch
Attachment #8600910 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
Depends on: 1162136
No longer depends on: 1162136
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.
Status: RESOLVED → VERIFIED
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.
Removing qe-verify flag as verification on Firefox 38.0.5 Beta and 40 Nightly should suffice.
Flags: qe-verify+
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".
(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.
(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.
You need to log in before you can comment on or make changes to this bug.