1155523 - Implement rough first-pass at Pocket toolbar button UI

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Justin Dolske [:Dolske]]

Firefox should ship with a Save To Pocket toolbar button by default, very similar to the existing Pocket SocialAPI button.

More detailed mocks to come shortly, but the current plan is roughly:

When clicked, the button with offer either:

* A signin panel (to create a Firefox Account if the user doesn't have one, or to use it to sign in to Pocket, or less prominently to use an existing Pocket website login. Clicking any of these actions will open a new tab with further steps (ie, signup/signin won't happen in the panel).

* A panel showing that the current web page was saved to Pocket, the open to open the item on Pocket, and the option to view all your Pocket items. This is shown when the user has already logged in to Pocket with a Firefox Account, or if they're already using it with a website login (TBD via the browser inspecting cookies.)

Content for both panels will be in-content. We'd like to have a way to do some early A/B testing, so we should think about how to do is. (Either by DOM manipulation to tweak content, or an iframe to load chrome:// variations. Sending the A/B flavor used as part of the signin/signup/save HTTP request should be sufficient for data analysis).

For version 1, there will be no "local" list of items saved to Pocket. But we will need to think about what to do when the user saves an item to Pocket, but the web service is slow/down. A temporary in-memory retry-later queue was suggested.

Lots more details to come!

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Quickie prototype

This is a super-gross prototype that I quickly cobbled together, just to start to figure out what issues might arise. Mostly for Jared and I to talk about how to start actually implementing stuff. Don't read anything in here as being definitive.

Comment 2

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

Attachment: Part 1, add toolbar button and add it to the toolbar by default

The button doesn't do anything yet, but this patch adds it to the toolbar and re-uses the OpenWebApps icon until toolbar.png and menuPanel.png are updated.

Needinfo for mmaslaney to update those sprites and attach them to this bug.

Comment 3

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

Attachment: Part 1, add toolbar button and add it to the toolbar by default

Comment 4

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

Attachment: Part 1, add toolbar button and add it to the toolbar by default

Hey Gijs, can you give some preliminary feedback on this patch and part 2 as well? We'll want to refactor how we do the layout of the panel probably because the panel view gets clipped in the sign-in view currently, http://screencast.com/t/kBnFviEgEH1

Comment 5

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

Attachment: Part 2, add panelview for pocket with a sign-in and page-saved view

Comment 6

[:Gijs (he/him)]

Comment on attachment 8594541 [details] [diff] [review]
Part 1, add toolbar button and add it to the toolbar by default

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1091,5 @@
> +  if (!locale) {
> +    try {
> +      locale = Services.prefs.getCharPref(LOCALE_PREF);
> +    } catch (ex) {}
> +  }

FWIW, this is apparently not good enough, cf. bug 1106119.

Not critical here, but we need a good, accessible library function for this somewhere. :-\

I'll also note that this pref can lie, ie won't necessarily reflect the locale that's in use if the user has set it to a locale that isn't available in their build.

I think using the snippet from the patch for the panic button will be more effective:

https://bugzilla.mozilla.org/show_bug.cgi?id=1073607

+  let chromeRegistry = Cc["@mozilla.org/chrome/chrome-registry;1"]
+                       .getService(Ci.nsIXULChromeRegistry);
+  let browserLocale = chromeRegistry.getSelectedLocale("browser");
+  let enabledLocales = [];
+  try {
+    enabledLocales = Services.prefs.getCharPref("privacy.panicButton.enabledLocales").split(' ');
+  } catch (ex) {
+    Cu.reportError(ex);
+  }
+  isPanicButtonEnabled = isPanicButtonEnabled && enabledLocales.indexOf(browserLocale) != -1;

::: browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
@@ +117,5 @@
>  devtools-webide-button2.label = WebIDE
>  devtools-webide-button2.tooltiptext = Open WebIDE (%S)
> +
> +pocket-send-button.label = Pocket
> +pocket-send-button.tooltiptext = Send this page to Pocket

How are we planning to hardcode this for uplift, if at all? For the DRM stuff we used a .properties file in the content instead of locale package, which makes the code change less radical and you'd still be using a properties file. YMMV here because we're relying on the default IDs here...

::: browser/themes/osx/browser.css
@@ +864,1 @@
>    #web-apps-button@toolbarButtonPressed@ {

Err, I'm assuming we want a proper icon?

Comment 7

[:Gijs (he/him)]

Comment on attachment 8594542 [details] [diff] [review]
Part 2, add panelview for pocket with a sign-in and page-saved view

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

Not sure what to say about this patch, it seems like the strings are unlikely to be "real", the code is also still test code... but this is generally how you'd add another subview, if that's what you wanted confirmation of, yeah. :-)

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1106,5 @@
> +        let loginView = document.getElementById("login-required");
> +        let pageSavedView = document.getElementById("page-saved");
> +        let showPageSaved = Math.random() < 0.5;
> +        loginView.hidden = !showPageSaved;
> +        pageSavedView.hidden = showPageSaved;

Test code, I presume? :-)

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +235,5 @@
> +          <label id="login-required-header" class="pocket-header" value="&pocket.loginRequired;"/>
> +          <hbox id="fxa-options">
> +            <label id="login-with-fxa" class="text-link" value="&pocket.logInWithFxa;"/>
> +            <label id="signup-for-fxa" class="text-link" value="&pocket.signUpForFxa;"/>
> +          </hbox>

So this part needs work in terms of layout, it sounds like?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +867,5 @@
>  <!ENTITY readingList.sidebar.add.label            "Add to Reading List">
>  <!ENTITY readingList.sidebar.add.tooltip          "Add this page to your Reading List">
> +
> +<!ENTITY pocket.loginRequired                     "Please log-in to continue">
> +<!ENTITY pocket.logInWithFxa                      "Log-in with Firefox Accounts">

Are these the official strings? Log-in with dashes in the middle seems weird.

Comment 8

[Francesco Lodolo [:flod]]

Is this code for trunk or something else? If it's for other branches, as Gijs already commented, we need some way to have hard-coded strings for en-US without making a ton of noise in localization files (and it's still unclear if we'll ever need them).

Comment 9

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

(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #8)
> Is this code for trunk or something else? If it's for other branches, as
> Gijs already commented, we need some way to have hard-coded strings for
> en-US without making a ton of noise in localization files (and it's still
> unclear if we'll ever need them).

We're gonna land the same patches on Nightly, Aurora, and Beta. We'll then later get Nightly using proper locale packages. I'm switching to using a separate /browser/base/content/ based .properties file so that it won't touch existing locale files.

Comment 10

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

Attachment: Part 1, add toolbar button and add it to the toolbar by default

Comment 11

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

Attachment: Part 2, add panelview for pocket with a sign-in and page-saved view (v2)

Comment 12

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

Attachment: Part 3, hook up Reading List button in Reader Mode to open Pocket panel

Comment 13

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

Attachment: Pocket_UI_Assets.zip

Comment 14

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

Hey Michael,

Thanks for the assets. Can you upload new menuPanel sprites with an inverted state for Pocket since it will open a subview if it is in the menu panel?

Thanks!

Comment 15

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

Sure thing, I realized that I also need to provide the highlight/active states as well.

Comment 16

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

Attachment: Part 1, add toolbar button and add it to the toolbar by default (v3)

I will have a follow-up patch to use the right icons when mmaslaney gets the new versions attached to this bug.

Comment 17

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

Attachment: Part 2, add panelview for pocket with a sign-in and page-saved view (v3)

Comment 18

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

Attachment: Part 3, hook up Reading List button in Reader Mode to open Pocket panel (v3)

Comment 19

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

Attachment: Part 4, placing Pocket in the palette will disable the feature

Comment 20

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

Attachment: Part 5, add UI for basic tagging functionality

Comment 21

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

Attachment: Part 6, CSS styling for Pocket

Based mostly off of the styling for the Pocket SocialAPI service, as well as some design docs that were shared with me. For review of this patch, I'm just looking to see if you find any issues with the approach (not necessarily the styling, which may still be tweaked later).

Comment 22

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

Attachment: Pocket_UI_Assets.zip

Comment 23

[:Gijs (he/him)]

Comment on attachment 8595576 [details] [diff] [review]
Part 1, add toolbar button and add it to the toolbar by default (v3)

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

r=me with the below addressed. Note that on m-c, we'll need windows hidpi imagery as well, which will need a bit of extra CSS love.

::: browser/components/customizableui/CustomizableUI.jsm
@@ +206,5 @@
>  #ifdef MOZ_DEV_EDITION
>        "developer-button",
>  #endif
>        "bookmarks-menu-button",
> +      "pocket-send-button",

Nit: maybe just pocket-button? Not sure it makes sense to use pocket-send...

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1084,5 @@
>      },
>    });
>  }
>  
> +if (Services.prefs.getBoolPref("browser.pocket.enabled")) {

Presumably we should land it without all this machinery on m-c? Or are we uplifting to m-b first and worrying about the rest later? :-(

::: browser/themes/osx/browser.css
@@ +859,5 @@
>    #panic-button@toolbarButtonPressed@ {
>      -moz-image-region: rect(18px, 702px, 36px, 684px);
>    }
>  
> +  #pocket-send-button@toolbarButtonPressed@,

Ideally, r=me with all of these updated to have the right (unique, new) coordinates.

Comment 24

[:Gijs (he/him)]

Comment on attachment 8595579 [details] [diff] [review]
Part 3, hook up Reading List button in Reader Mode to open Pocket panel (v3)

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

r=me but please mind all the comments

::: browser/modules/ReaderParent.jsm
@@ +44,5 @@
>    },
>  
>    receiveMessage: function(message) {
>      switch (message.name) {
> +      case "Reader:AddToList": {

I'm not convinced we should repurpose this message in case we do end up re-enabling and shipping reading list. Can we create a new message for this instead?

@@ +45,5 @@
>  
>    receiveMessage: function(message) {
>      switch (message.name) {
> +      case "Reader:AddToList": {
> +        let browser = message.target;

You don't use browser, so just:

let doc = message.target.ownerDocument;

@@ +49,5 @@
> +        let browser = message.target;
> +        let doc = browser.ownerDocument;
> +        let pocketWidget = doc.getElementById("pocket-send-button");
> +        let placement = CustomizableUI.getPlacementOfWidget("pocket-send-button");
> +        if (pocketWidget) {

This will fail in windows where the panel hasn't been opened yet. I would if (placement) instead, and create pocketWidget inside the if statement, after PanelUI.show() if we are in the panel.

Comment 25

[:Gijs (he/him)]

Comment on attachment 8595582 [details] [diff] [review]
Part 4, placing Pocket in the palette will disable the feature

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1105,3 @@
>      CustomizableWidgets.push({
>        id: "pocket-send-button",
> +      defaultArea: removedByUser ? null : CustomizableUI.AREA_NAVBAR,

If the user has removed it it seems OK to leave the defaultArea as it is. This also gets this out of sync with the attributes in browser.xul, and I currently don't have the bandwidth to try and dig up if/why we can('t) remove those right now.

That said, that probably means that a reset will add them back, and an undo reset may remove them again, so the listener would have to handle those cases, too...

@@ +1150,5 @@
> +
> +            CustomizableUI.removeListener(listener);
> +          }
> +        };
> +        CustomizableUI.addListener(listener);

This creates a listener for every window. You just need one, so please only create one (outside the addWidget() call, inside the if block).

::: toolkit/components/reader/AboutReader.jsm
@@ +80,5 @@
>    }
>  
> +  if (Services.prefs.getBoolPref("browser.pocket.removedByUser")) {
> +    this._doc.getElementById("toggle-button").hidden = true;
> +    this._doc.getElementById("list-button").hidden = true;

AFAICT these don't get hooked up to do anything if reading list isn't enabled, and we're disabling that pref, so we still need to fix the button to have a click handler and send a (new?) message even where it isn't hidden, in this file. (just above this block)

Comment 26

[:Gijs (he/him)]

Comment on attachment 8595583 [details] [diff] [review]
Part 5, add UI for basic tagging functionality

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

I'm not reviewing any styling/layout of the resulting panelview so far.

r=me with the code tidying below

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1130,5 @@
>          for (let elementId of elementsNeedingStrings) {
>            let el = doc.getElementById(elementId);
> +          let string = PocketBundle.GetStringFromName(elementId);
> +
> +          switch (el.localName) {

Add a default: statement. Let's go with textContent for that as it has the highest chances of either working or breaking badly enough (rather than doing nothing) that it'll get seen quickly by people changing stuff here.

@@ +1147,5 @@
> +        let eventListener = {
> +          handleEvent(event) {
> +            let doc = event.target.ownerDocument;
> +            let field = doc.getElementById("pocket-page-tags-field");
> +            let button = doc.getElementById("pocket-page-tags-add");

Stick the listener on the widget spec, please, and just add |this| as a listener, to avoid leaks from in-scope things (and because it'll make the code a little clearer). Make sure we nuke the listeners if the node is destroyed. IIRC we tell the node.

@@ +1150,5 @@
> +            let field = doc.getElementById("pocket-page-tags-field");
> +            let button = doc.getElementById("pocket-page-tags-add");
> +            switch (event.type) {
> +              case "input":
> +                button.disabled = !field.value;

Nit: field.value.trim()

Comment 27

[:Gijs (he/him)]

Comment on attachment 8595577 [details] [diff] [review]
Part 2, add panelview for pocket with a sign-in and page-saved view (v3)

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

clearing review pending me not getting when/how we're going to make the views do Stuff.

::: browser/base/content/browser-pocket.properties
@@ +14,5 @@
> +pocket-account-question  = Already have an account?
> +pocket-login-now         = Log in now
> +pocket-page-saved-header = Page Saved
> +pocket-open-pocket       = Open Pocket
> +pocket-remove-page       = Remove Page

I'm going to just assume these are the correct strings based on design documents I haven't seen...

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1132,5 @@
> +        let doc = event.target.ownerDocument;
> +
> +        let loginView = doc.getElementById("pocket-login-required");
> +        let pageSavedView = doc.getElementById("pocket-page-saved");
> +        let showPageSaved = Math.random() < 0.5;

Can you clarify what the story is in terms of making this, you know, do something sensible? :-)

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +230,5 @@
>      </panelview>
>  
> +    <panelview id="PanelUI-pocketView" flex="1">
> +      <vbox class="panel-subview-body">
> +        <vbox id="pocket-login-required">

If we don't always need either and are going to be twiddling hidden on them anyway, maybe we should set them both to hidden in the markup to avoid paying for bindings?

Comment 28

[:Gijs (he/him)]

Comment on attachment 8595586 [details] [diff] [review]
Part 6, CSS styling for Pocket

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

ERR_QUESTION_OVERLOAD.

Sorry, can we talk this over here or on IRC? Some of this is a little odd. I also don't know where the designs are which makes it hard to review this.

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1139,5 @@
>              case "button":
>                el.label = string;
>                break;
>              case "textbox":
> +              el.setAttribute("placeholder", string);

Can you ensure this goes in the previous patch?

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +230,5 @@
>      </panelview>
>  
>      <panelview id="PanelUI-pocketView" flex="1">
>        <vbox class="panel-subview-body">
> +        <label id="pocket-header"/>

This should probably have the normal class these headers have. subview-header ?

@@ +251,5 @@
> +          <hbox id="pocket-separator">
> +            <box/>
> +            <box/>
> +            <box/>
> +            <box/>

Eh?

::: browser/components/pocket/jar.mn
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +browser.jar:
> +  content/browser/pocket/img/signup_logo.png     (img/signup_logo.png)

You have skin files that depend on images which are here? Why is this in content and not in themes/ ?

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +1251,5 @@
> +  background-repeat: no-repeat;
> +  background-position: center;
> +  margin: 1rem 0;
> +  width: 116px;
> +  height: 29px;

This mixed with rems doesn't make me feel good. Can you give me some prose as to what the reasoning here is and how it *isn't* going to break? :-)

@@ +1352,5 @@
> +  margin-top: 1rem;
> +  color: #666;
> +}
> +
> +@media (min-resolution: 2dppx) {

For windows, shouldn't this be 1.25 or whatever?

@@ +1360,5 @@
> +  }
> +
> +  #pocket-signup-or {
> +    background-image: url(chrome://browser/content/pocket/img/[e-mail]);
> +    background-size: 246px 27px;

I can see this causing issues with our em-based widths/heights from a mile away.

Comment 29

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #27)
> Comment on attachment 8595577 [details] [diff] [review]

> I'm going to just assume these are the correct strings based on design
> documents I haven't seen...

The design documents aren't finished yet, I've been told we would likely have them by Thursday this week.


> ::: browser/components/customizableui/CustomizableWidgets.jsm
> @@ +1132,5 @@
> > +        let doc = event.target.ownerDocument;
> > +
> > +        let loginView = doc.getElementById("pocket-login-required");
> > +        let pageSavedView = doc.getElementById("pocket-page-saved");
> > +        let showPageSaved = Math.random() < 0.5;
> 
> Can you clarify what the story is in terms of making this, you know, do
> something sensible? :-)

I'm working on that in bug 1156878.

Comment 30

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

Attachment: Rolled-up patch with assets

I've rolled up the patch and addressed the review feedback that you had. Reader View is missing the icon for Pocket though. We'll get that later, until now it is a blank button with the right tooltip though. As the feature will be landing pref'ed off, I think it is OK to land missing this icon.

Comment 31

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

Much of the part 6 feedback was not addressed.

> You have skin files that depend on images which are here? Why is this in content and not in themes/ ?

These were placed in content because they are 3rd-party images and we don't want them changed by themes, especially the signup_logo.png image. This is the same as we did for the 3rd party logos in use by Hello. You're right though that we should have the CSS controlling this be in content and not in a skin directory. I'd like to address this in a follow-up patch.

> Eh?

This is part of the design spec for Pocket, which uses a horizontal separator that consists of four separate colors each occupying equal amounts of space.

Comment 32

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

Attachment: Rolled-up patch with assets (v1.1)

Comment 33

[Justin Dolske [:Dolske]]

Comment on attachment 8596695 [details] [diff] [review]
Rolled-up patch with assets (v1.1)

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

Driveby!

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1100,5 @@
> +    isEnabledForLocale = enabledLocales.indexOf(browserLocale) != -1;
> +  }
> +
> +  if (isEnabledForLocale) {
> +    let pocketButton = {

Nit: seems like this would be more readable (and less indented!) if pocketButton was defined outside of these two conditionals. Which would then just handle the CustomizableWidgets.push() & CustomizableWidgets.addListener().

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +228,5 @@
>                  label="&panicButton.view.forgetButton;"/>
>        </vbox>
>      </panelview>
>  
> +    <panelview id="PanelUI-pocketView" flex="1">

Late observation: I didn't use a <panelview> in my original hack, because it wasn't clear that having the panel contents in a tall&narrow subview was going to work very well here.

I guess we can see how this works out, and doesn't block an initial landing, but worth checking with UX to see if this is something we want.

Comment 34

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #29)
> (In reply to :Gijs Kruitbosch from comment #27)
> > Comment on attachment 8595577 [details] [diff] [review]
> 
> > I'm going to just assume these are the correct strings based on design
> > documents I haven't seen...
> 
> The design documents aren't finished yet, I've been told we would likely
> have them by Thursday this week.

Today is Friday... who do we need to ping about this? Please redirect as appropriate.

Comment 35

[:Gijs (he/him)]

Comment on attachment 8596695 [details] [diff] [review]
Rolled-up patch with assets (v1.1)

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

r=me but quite a lot of feedback. I should be around to do another review later today if you think that's wise.

This won't add the button on profiles with modified toolbars where we land this before flipping the pref to true. Can we leave the _introducedInVersion bit and the kVersion increment out until we enable it so that then it actually gets put on toolbars even where those have been customized?

Also, the panel always has a (vertical) scrollbar on OSX. We'll need to fix that for release, but I don't want to be holding up this initial patch much longer. :-\

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1184,5 @@
> +          addTagsButton.removeEventListener("command", this);
> +        } else {
> +          Services.prefs.clearUserPref(this.USER_REMOVED_PREF);
> +          addTagsField.addEventListener("input", this);
> +          addTagsButton.addEventListener("command", this);

Ah, this is wrong, unfortunately - if you move the widget inside its container this will be called (without being called for a removal) and so we'll just keep adding listeners.

Why are we removing listeners at all? Why don't we just add a listener when the node is created and keep it there? The view (where we add these listeners) is never removed from the DOM or something...

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +230,5 @@
>      </panelview>
>  
> +    <panelview id="PanelUI-pocketView" flex="1">
> +      <vbox class="panel-subview-body">
> +        <label id="pocket-header"/>

Still missing "normal" subview classes (where the header, fwiw, is normally outside of the body). Intentional?

::: browser/modules/ReaderParent.jsm
@@ +69,5 @@
> +        let placement = CustomizableUI.getPlacementOfWidget("pocket-button");
> +        if (placement) {
> +          if (placement.area == CustomizableUI.AREA_PANEL) {
> +            doc.defaultView.PanelUI.show().then(function() {
> +              pocketWidget = doc.getElementById("pocket-button");

Add a comment about why we're reassigning here.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +1264,5 @@
> +
> +.pocket-button {
> +  -moz-appearance: none;
> +  background-color: #d3505a;
> +  background-image: linear-gradient(to bottom, #ee5f64 0%, #d3505a 100%);

Don't think you need the color stop coordinates.

@@ +1294,5 @@
> +  margin-bottom: 3rem;
> +}
> +
> +.pocket-button:hover {
> +  background-position: 0 -15px;

This and the transition, in my admittedly very personal and unimportant opinion... ahem, it doesn't look optimal. Why are we doing this? I'm guessing it's the same design documents I've still not seen?

It looks especially silly when moving across the button and not clicking it, so you get a flash of it and then it disappears. The plain white-ish non-gradient that comes in from the bottom on the second button also doesn't look nice, IMHO.

@@ +1322,5 @@
> +#pocket-separator > box {
> +  -moz-box-flex: 1;
> +}
> +
> +#pocket-separator box:nth-child(1) {

Parent rather than descendant selector please. Also, can't we give these a class and/or ID so the selector isn't going to match boxes all over the browser UI?

@@ +1344,5 @@
> +  padding: 1rem 0;
> +}
> +
> +#pocket-page-tags-field {
> +  text-align: start;

Why is this necessary? If this is inherited from the save-page container, can we either put that style further down in the hierarchy to where we need it, or otherwise set #pocket-page-tags to be text-align: start so we don't need to correct both here and for tags-header?

::: toolkit/components/reader/AboutReader.jsm
@@ +21,5 @@
>  let gStrings = Services.strings.createBundle("chrome://global/locale/aboutReader.properties");
> +let gPocketStrings;
> +try {
> +  gPocketStrings = Services.strings.createBundle("chrome://browser/content/browser-pocket.properties");
> +} catch (e) {}

Why is this in an empty try...catch?

@@ +82,5 @@
>    } catch (e) {
>      // Pref doesn't exist.
>    }
>  
> +  if (Services.prefs.getBoolPref("browser.pocket.enabled")) {

This will enable it even for locales where it's not supposed to be? Seems like we could instead rely on:

CustomizableUI.getPlacementOfWidget("pocket-button")

which is a little hacky but would work, AIUI, and saves us duplicating all the code? (the else block for that should have the .hidden setter from the next if() block).

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/905ec194fbba

Comment 37

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

(In reply to :Gijs Kruitbosch from comment #35)
> ::: browser/components/customizableui/content/panelUI.inc.xul
> @@ +230,5 @@
> >      </panelview>
> >  
> > +    <panelview id="PanelUI-pocketView" flex="1">
> > +      <vbox class="panel-subview-body">
> > +        <label id="pocket-header"/>
> 
> Still missing "normal" subview classes (where the header, fwiw, is normally
> outside of the body). Intentional?

The design is in flux so I'll come back to this when we have a more nailed-down spec.
 
> ::: toolkit/components/reader/AboutReader.jsm
> @@ +21,5 @@
> >  let gStrings = Services.strings.createBundle("chrome://global/locale/aboutReader.properties");
> > +let gPocketStrings;
> > +try {
> > +  gPocketStrings = Services.strings.createBundle("chrome://browser/content/browser-pocket.properties");
> > +} catch (e) {}
> 
> Why is this in an empty try...catch?

Because this is a browser properties file being loaded in toolkit. I've removed it from this patch and we can tackle that in a follow-up. Until then the button won't have an icon (still need one from UX) and also won't have a tooltip. THE MYSTERY GHOST BUTTON!!!1

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7c8b99e56b16

Comment 39

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #34)

> Today is Friday... who do we need to ping about this? Please redirect as
> appropriate.

Removing needinfo as we discussed this on IRC.

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/905ec194fbba
https://hg.mozilla.org/mozilla-central/rev/7c8b99e56b16

Comment 41

[Stuart Philp :sphilp]

:florian can you summarize the IRC convo here so it's tracked in this bug

Do we have follow ups filed for the strings, icon, tooltip, and anything that may need a bug? I'm not seeing them in the tree

Comment 42

[Florian Quèze [:florian]]

(In reply to Stuart Philp :sphilp from comment #41)
> :florian can you summarize the IRC convo here so it's tracked in this bug

Summary is that I don't have any design document from our UX team (you can ping mmaslaney if you need something). I shared with Gijs a link to a draft from the Pocket team, as I didn't have anything else. I don't know if I can share it publicly.

> Do we have follow ups filed for the strings, icon, tooltip, and anything
> that may need a bug? I'm not seeing them in the tree

All the bugs we have are blocking bug 1155467. I'm not sure why we need follow-ups for the strings (or do you mean to make them localizable for Firefox 40?). There's already an icon and a tooltip in what landed here, but follow-ups may be filed if they need polishing.

Comment 43

[Stuart Philp :sphilp]

Thanks! I was referring to the previous comments discussing further work, such as https://bugzilla.mozilla.org/show_bug.cgi?id=1155523#c37

If everything is done though, then disregard, it just sounded like there was more to do :)

Comment 44

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

(In reply to Stuart Philp :sphilp from comment #43)
> Thanks! I was referring to the previous comments discussing further work,
> such as https://bugzilla.mozilla.org/show_bug.cgi?id=1155523#c37
> 
> If everything is done though, then disregard, it just sounded like there was
> more to do :)

Yeah, there is more work to do. We'll be filing more bugs as we start on the work, but we will most likely wait to file them until we know what is being requested. When they're filed they will be blocking bug 1155467.

Comment 45

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #42)
> (In reply to Stuart Philp :sphilp from comment #41)
> > :florian can you summarize the IRC convo here so it's tracked in this bug
> 
> Summary is that I don't have any design document from our UX team (you can
> ping mmaslaney if you need something). I shared with Gijs a link to a draft
> from the Pocket team, as I didn't have anything else. I don't know if I can
> share it publicly.

The document I had has been attached as attachment 8598080 [details].

Comment 46

[Andrei Vaida [:avaida]]

As far as I can tell, the only working features so far are saving, removing and tagging pages, though there's no feedback displayed for them - something to let the user know, UI/UX-wise, that the action initiated from the panel actually came through. 

I assume that everything else is coming later, in separate bugs - Jared, could you please confirm? Are these the only features expected to work in this patch?

Comment 47

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

That's right. The UX here isn't finished, and the styling will be changing significantly through other bugs (most have yet to be filed).

Comment 48

[Andrei Vaida [:avaida]]

Confirmed fixed as of Nightly 40.0a1 (2015-04-29), using Ubuntu 14.04 (x64), Windows 8.1 (x64) and Mac OS X 10.9.5, pending follow-up fixes (see Comment 45 & Comment 46). Regression testing was conducted both with and without e10s - the currently available toolbar button functionalities work fine in both cases.

Comment 49

[Justin Dolske [:Dolske]]

Comment on attachment 8596695 [details] [diff] [review]
Rolled-up patch with assets (v1.1)

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

(Some later bugs change this work significantly, but it's less painful to just replay the work bug-by-bug as it was actually developed.)

Comment 50

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8596695 [details] [diff] [review]
Rolled-up patch with assets (v1.1)

[Triage Comment]
Fix the branch

Comment 51

[Ryan VanderMeulen [:RyanVM]]

The attached roll-up patch doesn't apply to Aurora.

Comment 52

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/51090bebeac6

Comment 53

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

https://hg.mozilla.org/releases/mozilla-release/rev/3e9805c11aa3

Comment 54

[Justin Dolske [:Dolske]]

(In reply to Pulsebot from comment #38)
> https://hg.mozilla.org/integration/fx-team/rev/7c8b99e56b16

This followup got missd from the uplift, but bug 1155521 readds it, so it's all good.

Comment 55

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 38.0.5 
BUild Id: 20150511143336
OS: Win 7 x64, Ubuntu 14.04 x86, Mac Os X 10.9.5