1388170 - Page Action Menu > Pocket needs to read "Save to Pocket"

Status: VERIFIED FIXED

(Firefox :: Pocket, defect, P1)

Description

[Aaron Benson]

Attachment: save-to-pocket.png

Open the Page Action Menu (•••), the item that reads "Pocket" should read "Save to Pocket"

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1388170 - Page Action Menu > Pocket needs to read "Save to Pocket".

Review commit: https://reviewboard.mozilla.org/r/169218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169218/

Comment 2

[Drew Willcoxon :adw]

Aaron, we currently have a few similar strings for saving:

* pocket-button.tooltiptext = Save to Pocket
* saveToPocketCmd.label = Save Page to Pocket
* saveLinkToPocketCmd.label = Save Link to Pocket

My patch uses the first one for this label, but it seems kind of sketchy to use "tooltiptext" for the button label.

So I point this out to ask, do we really want all three of these, and for this bug should we maybe use the command label (saveToPocketCmd.label = Save Page to Pocket) instead?

Comment 3

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #2)
> but it seems kind of sketchy to use "tooltiptext" for the button label.

Of course we could just rename this string or duplicate it.

Comment 4

[Drew Willcoxon :adw]

(If it's a big problem, which I'm not sure it is)

Comment 5

[:Gijs (he/him)]

I'm redirecting the review to :flod because I'm not sure what to do here, and if reusing a label from elsewhere is fine. I *think* that reusing the command label is probably OK, but I'm less sure about the tooltip one. I also don't know how adding new strings for pocket works because last I checked we shipped all the locales in-product ( cf. https://dxr.mozilla.org/mozilla-central/search?q=path%3Alocale%20path%3Apocket&=mozilla-central ) and I don't know how those get updated.

Comment 6

[Francesco Lodolo [:flod]]

Comment on attachment 8897932 [details]
Bug 1388170 - Page Action Menu > Pocket needs to read "Save to Pocket".

https://reviewboard.mozilla.org/r/169218/#review174624

Using a tooltip for a label is definitely a bad idea. 

Button = action, tooltip = description of what the element does. As a consequence, in some languages the button label is imperative, while the tooltip uses a present tense verb (third person).

If you need to add strings to the Pocket add-on, Shane (mixedpuppy) is your best source of information.
https://github.com/mozilla-partners/pocket
https://github.com/mozilla-l10n/pocket-l10n

Strings are exposed in a repository outside of mozilla-central, then translated in Pontoon and dumped back into mozilla-central

Comment 7

[Drew Willcoxon :adw]

OK, thanks Francesco.  IMO we should use the existing command label (saveToPocketCmd.label = Save Page to Pocket), but I'll wait to see what Aaron says.

Comment 8

[Aaron Benson]

I'm not exactly sure where those strings show up but I can imagine the context between 'page' and 'link' might be useful. Since this particular issue is about the menu item in the Page Action Menu, I'd be okay if it read "Save Page to Pocket".

Comment 9

[Aaron Benson]

Ack, however there's another item that reads "Send to Device" so lets be consistent and use "Save to Pocket". Final answer. :)

Comment 10

[Drew Willcoxon :adw]

We'd have to go through a whole localization process, described in comment 6, for that.  Also, the bookmark menu item is labeled "Bookmark This Page."  Are you sure?  It'd be way easier to change "Send to Device" to "Send Page to Device" than to add a new Pocket string.

Comment 11

[Aaron Benson]

Well if it's easier that way then I say go for it. :) I was mostly shooting for consistency in the labels.

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8897932 [details]
Bug 1388170 - Page Action Menu > Pocket needs to read "Save to Pocket".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169218/diff/1-2/

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8897932 [details]
Bug 1388170 - Page Action Menu > Pocket needs to read "Save to Pocket".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169218/diff/2-3/

Comment 14

[:Gijs (he/him)]

Comment on attachment 8897932 [details]
Bug 1388170 - Page Action Menu > Pocket needs to read "Save to Pocket".

https://reviewboard.mozilla.org/r/169218/#review174664

Then let's just update the send to device string to also be "Send Page to Device?"

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs from comment #14)
> Comment on attachment 8897932 [details]
> Bug 1388170 - Page Action Menu > Pocket needs to read "Save to Pocket".
> 
> https://reviewboard.mozilla.org/r/169218/#review174664
> 
> Then let's just update the send to device string to also be "Send Page to
> Device?"

Err, ignore this comment. The mozreview header was collapsed when I refreshed, and I didn't realize I'd be sending this when hitting publish.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b094de6a8d3b
Page Action Menu > Pocket needs to read "Save to Pocket". r=Gijs

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b094de6a8d3b

Comment 18

[Sajedul Islam]

I have reproduced this bug with Nightly 57.0a1 (2017-08-07) on Windows 8.1 !

This bug's fix is verified with latest Nightly!

Build Id   : 20170820100343
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170816]

Comment 19

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 57.0a1 (2017-08-07) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Nightly!

Build  ID  : 20170823100553 
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0