1155519 - Add "View Pocket Items" menuitem to the bookmarks menu

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Justin Dolske [:Dolske]]

When the user clicks the Bookmarks toolbar button (or the toplevel Bookmarks menubar item), they should have a "View Pocket Items" (wording TBD) menuitem that takes them to their Pocket webpage.

If the user isn't signedin/signed up, I think the menuitem should just be suppressed?

Comment 1

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

(In reply to Justin Dolske [:Dolske] from comment #0)
> If the user isn't signedin/signed up, I think the menuitem should just be
> suppressed?

@mmaslaney: sound good?  hide the menu item when a user isn't signed in.

Comment 2

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

Sounds good.

Comment 3

[Florian Quèze [:florian]]

Attachment: WIP

Left to do:
- add icons
- (maybe) move the hardcoded "Pocket" string to the pocket.properties file, and set the label attribute from the updatePocketItemVisibility function of browser-places.js

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 8599953 [details] [diff] [review]
WIP

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

::: browser/base/content/browser-menubar.inc
@@ +466,5 @@
>        </menu>
>  #endif
> +      <menuseparator id="menu_pocketSeparator"/>
> +      <menuitem id="menu_pocket" label="Pocket"
> +                oncommand="openUILink('https://' + Pocket.hostname, event);"/>

Instead of construction a URL here (and the other sites that use it), it would be better add a Pocket.myListURL (or whatever, bikeshed away) getter that does it. That also gives us a single place to change it should we ever need to do so.

Comment 5

[Justin Dolske [:Dolske]]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)

> - (maybe) move the hardcoded "Pocket" string to the pocket.properties file,
> and set the label attribute from the updatePocketItemVisibility function of
> browser-places.js

Yes please, as with bug 1155518 comment 13.

Comment 6

[Florian Quèze [:florian]]

Attachment: Patch v2

We still need a better sized icon.

Comment 7

[Justin Dolske [:Dolske]]

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

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

Will ask mmaslaney for an icon, can either land it here or in a followup depending on timing.

::: browser/base/content/browser-places.js
@@ +1568,5 @@
> +  updatePocketItemVisibility: function BUI_updatePocketItemVisibility(prefix) {
> +    let hidden = !CustomizableUI.getPlacementOfWidget("pocket-button");
> +    document.getElementById(prefix + "pocket").hidden = hidden;
> +    document.getElementById(prefix + "pocketSeparator").hidden = hidden;
> +  },

Feels a little weird to just pass in a ID prefix here, but I don't have a significantly better solution so this is fine.

Passing in the full ID would be nice, but that makes the separator a pain. Could pass in a generic name/kind, but that's a bit weird too, eg:

  switch (what) {
    case "menu":
      id = "menu_pocket";
      sep = "menu_pocketSeparator";
    case "menuPanel":
      ...
  }

Comment 8

[Justin Dolske [:Dolske]]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)

> We still need a better sized icon.

Michael: can you generate the 16x16 and 32x32 icons we'll need for use here (in the bookmarks menu)?

Comment 9

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

Attachment: Pocket_Menu_Assets.zip

Comment 10

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

The label here should be: View Pocket List

Comment 11

[Pulsebot]

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

Comment 12

[Florian Quèze [:florian]]

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #10)
> The label here should be: View Pocket List

To please you I changed the wording to this before landing, but note that:
- it doesn't match the final UX spec at https://mozilla.invisionapp.com/share/4E2TPD4YD#/screens/75361624?maintainScrollPosition=false
- 'View Pocket List' is misleading if the user isn't logged in, as clicking that item will just send the user to the Pocket home page.

I also included the icons from comment 9 and tweaked the patch a little bit to ensure they are displayed correctly.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/2b0b00fb6411

Comment 14

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

(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> (In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #10)
> > The label here should be: View Pocket List
> 
> To please you I changed the wording to this before landing, but note that:
> - it doesn't match the final UX spec at
> https://mozilla.invisionapp.com/share/4E2TPD4YD#/screens/
> 75361624?maintainScrollPosition=false

Sorry, strings only just came in final yesterday.  I'll ping maslaney to get that updated to reflect the changes.

> - 'View Pocket List' is misleading if the user isn't logged in, as clicking
> that item will just send the user to the Pocket home page.

Good point.  Lets leave it for now and I'll file a follow up bug.  Might be good to check that a user is logged into Pocket and show or hide this item base on that instead of presence of the toolbar button.

> I also included the icons from comment 9 and tweaked the patch a little bit
> to ensure they are displayed correctly.

Ok.

Comment 15

[Justin Dolske [:Dolske]]

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

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

Comment 16

[Sylvestre Ledru [:Sylvestre]]

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

[Triage Comment]
Fix the branch

Comment 17

[Andrei Vaida [:avaida]]

I noticed 3 different issues on Nightly 40.0a1 (2015-05-06), all reproducible on Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS 10.9.5:

1. Bug 1162147 — the Pocket icon is missing from the associated menuitem
2. The "View Pocket List" menuitem is positioned in a different spot, compared to the UI spec
3. The menuitem has no functionality attached, it doesn't point the user to the Pocket list

Florian, are these items pending implementation?

Comment 18

[Florian Quèze [:florian]]

(In reply to Andrei Vaida, QA [:avaida] from comment #17)
> I noticed 3 different issues on Nightly 40.0a1 (2015-05-06), all
> reproducible on Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS 10.9.5:
> 
> 1. Bug 1162147 — the Pocket icon is missing from the associated menuitem
> 2. The "View Pocket List" menuitem is positioned in a different spot,
> compared to the UI spec

The spec I implemented is https://mozilla.invisionapp.com/share/4E2TPD4YD#/screens/75361624?maintainScrollPosition=false

> 3. The menuitem has no functionality attached, it doesn't point the user to
> the Pocket list

This is surprising. Last time I tried, clicking the item opened a tab with the pocket website.

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> > 3. The menuitem has no functionality attached, it doesn't point the user to
> > the Pocket list
> 
> This is surprising. Last time I tried, clicking the item opened a tab with
> the pocket website.

I tried this on four different test machines, it doesn't open the Pocket website. Same behavior in the case of this slightly different flow: Menu Bar → Bookmarks → View Pocket List.

Comment 20

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)

> > 3. The menuitem has no functionality attached, it doesn't point the user to
> > the Pocket list
> 
> This is surprising. Last time I tried, clicking the item opened a tab with
> the pocket website.

Bug 1161654 broke this by removing the listURL getter from Pocket.jsm.

Comment 21

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/902a07c1efad

Comment 22

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

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

Comment 23

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

(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> (In reply to Florian Quèze [:florian] [:flo] from comment #18)
> 
> > > 3. The menuitem has no functionality attached, it doesn't point the user to
> > > the Pocket list
> > 
> > This is surprising. Last time I tried, clicking the item opened a tab with
> > the pocket website.
> 
> Bug 1161654 broke this by removing the listURL getter from Pocket.jsm.

Filed bug 1162735 to fix this. Sorry.

Comment 24

[Andrei Vaida [:avaida]]

Since all the bugs found while verifying this fix will be treated separately -- Bug 1162735, Bug 1162147 -- I'm closing this one. As mentioned in Comment 17, regression testing was performed on Nightly 40.0a1 (2015-05-06), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Comment 25

[Catalin Varga [QA][:VarCat]]

I've verified this issue using the following environment:

FF 38.0.5
Build id: 20150510205200
OS: Win 7 x64, Ubuntu 14.04 x86, Mac Os X 10.9.5

I've noticed an inconsistency related to the icon for View Pocket List button from Bookmarks menu. The icon is being displayed on Mac but not on windows. What is the expected behavior?

Comment 26

[Florian Quèze [:florian]]

(In reply to Catalin Varga [QA][:VarCat] from comment #25)

> I've noticed an inconsistency related to the icon for View Pocket List
> button from Bookmarks menu. The icon is being displayed on Mac but not on
> windows. What is the expected behavior?

This isn't expected, I implemented this quickly and only tested on Mac. Could you please file a bug? Thanks!

Comment 27

[Catalin Varga [QA][:VarCat]]

I've logged bug bug 1163651 with the aforementioned issue.

Comment 28

[Andrei Vaida [:avaida]]

Updating flags, as the issue found during verification will be treated separately, see Comment 27.

Comment 29

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

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #14)
> > - 'View Pocket List' is misleading if the user isn't logged in, as clicking
> > that item will just send the user to the Pocket home page.
> 
> Good point.  Lets leave it for now and I'll file a follow up bug.  Might be
> good to check that a user is logged into Pocket and show or hide this item
> base on that instead of presence of the toolbar button.

Did a follow-up get filed for this?

Comment 30

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

just filed now, bug 1166717