Closed Bug 1155519 Opened 9 years ago Closed 9 years ago

Add "View Pocket Items" menuitem to the bookmarks menu

Categories

(Firefox :: Menus, defect)

defect
Not set
normal
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

(2 files, 1 obsolete file)

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?
Blocks: Pocket
Flags: qe-verify?
Flags: firefox-backlog+
(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.
Flags: needinfo?(mmaslaney)
Sounds good.
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 WIP (obsolete) — Splinter Review
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 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.
(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.
Attached patch Patch v2Splinter Review
We still need a better sized icon.
Attachment #8599953 - Attachment is obsolete: true
Attachment #8600932 - Flags: review?(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":
      ...
  }
Attachment #8600932 - Flags: review?(dolske) → review+
(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)?
Flags: needinfo?(mmaslaney)
Attached file Pocket_Menu_Assets.zip
Flags: needinfo?(mmaslaney)
The label here should be: View Pocket List
(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.
https://hg.mozilla.org/mozilla-central/rev/2b0b00fb6411
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
(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 on attachment 8600932 [details] [diff] [review]
Patch v2

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

[Triage Comment]
Fix the branch
Attachment #8600932 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
Depends on: 1162147
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?
Flags: needinfo?(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.
Flags: needinfo?(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.

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.
(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.
Flags: needinfo?(jaws)
Flags: needinfo?(jaws)
(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.
Depends on: 1162735
Flags: needinfo?(jaws)
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.
Status: RESOLVED → VERIFIED
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?
Flags: needinfo?(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!
Flags: needinfo?(florian)
I've logged bug bug 1163651 with the aforementioned issue.
Updating flags, as the issue found during verification will be treated separately, see Comment 27.
Depends on: 1163651
Flags: qe-verify+
(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?
Flags: needinfo?(clarkbw)
Depends on: 1165353
Flags: needinfo?(clarkbw)
You need to log in before you can comment on or make changes to this bug.