1131303 - Implement strings for Reader Mode toolbar controls

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

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

The following strings need to be implemented for Readermode.

Readermode Controls

(X) – "Close Readermode"
(Aa) – "Type Controls"
(+) – "Add to Reading List"
(-) – "Delete from Reading List"
(=) – "Open Reading List" / "Close Reading List"

Readermode Footer

(-) "Delete This Article"

Comment 1

[:Margaret Leibovic]

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from comment #0)

> Readermode Footer
> 
> (-) "Delete This Article"

In bug 1120735 I punted on implementing this footer because I wasn't sure what it's supposed to mean. How is this different than deleting the article from the reading list?

I can file another bug to add this footer, but I want to be clear about what it's supposed to be doing.

Comment 2

[:Margaret Leibovic]

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from comment #0)

> Readermode Controls
> 
> (X) – "Close Readermode"

Also, we should do an audit to make sure we're consistent with either "Readermode" or "Reader Mode". I think that we've generally been using "Reader Mode" (two words).

(NI to antlam to chime in on the consistency aspect)

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #2)
> (In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from
> comment #0)
> 
> > Readermode Controls
> > 
> > (X) – "Close Readermode"
> 
> Also, we should do an audit to make sure we're consistent with either
> "Readermode" or "Reader Mode". I think that we've generally been using
> "Reader Mode" (two words).
> 
> (NI to antlam to chime in on the consistency aspect)

YES for consistency! Thanks for NI-ing me to this.

I've been following Bugzilla and referring it to Reader Mode (two words) everywhere. :)

Comment 4

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

How do we feel about "Reading View", as suggested by Matej. I feel that it creates a nice association with it's buddy feature: "Reading List".

Comment 5

[Matej Novak [:matej]]

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from comment #4)
> How do we feel about "Reading View", as suggested by Matej. I feel that it
> creates a nice association with it's buddy feature: "Reading List".

Just added it as an option in bug 1129537.

Comment 6

[:Margaret Leibovic]

Attachment: WIP - Add title text to reader mode toolbar buttons

I have a WIP here to add these strings, and to actually include them with the buttons.

I'll finish this up and request review once we decide on the final strings we're going to use.

Comment 7

[Matej Novak [:matej]]

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from comment #0)
> The following strings need to be implemented for Readermode.

Is there a UI mockup of these strings that I could reference?

> Readermode Controls
> 
> (X) – "Close Readermode"

Sounds like we already have this one covered, but just in case, it should be "Close Reading View"

It could also be "Exit Reading View"

> (Aa) – "Type Controls"

I'm not sure what this one means or how it would be used.

> (+) – "Add to Reading List"

Looks good.

> (-) – "Delete from Reading List"

Maybe "Remove from Reading List" instead?

> (=) – "Open Reading List" / "Close Reading List"

Great.

> Readermode Footer
> 
> (-) "Delete This Article"

To consistent with the above capitalization, this should be "Delete this Article"

Comment 8

[:Margaret Leibovic]

(In reply to Matej Novak [:matej] from comment #7)
> (In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from
> comment #0)
> > The following strings need to be implemented for Readermode.
> 
> Is there a UI mockup of these strings that I could reference?

These are the strings for the tooltip/accessibility text for the reader mode toolbar buttons.

Here's a mock-up:
https://projects.invisionapp.com/share/XA217X8SY#/screens/57292604

Comment 9

[Matej Novak [:matej]]

(In reply to :Margaret Leibovic from comment #8)
> (In reply to Matej Novak [:matej] from comment #7)
> > (In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from
> > comment #0)
> > > The following strings need to be implemented for Readermode.
> > 
> > Is there a UI mockup of these strings that I could reference?
> 
> These are the strings for the tooltip/accessibility text for the reader mode
> toolbar buttons.
> 
> Here's a mock-up:
> https://projects.invisionapp.com/share/XA217X8SY#/screens/57292604

Thanks.

I just noticed an inconsistency in what I put in comment 7. Instead of "Delete this Article," let's make it "Remove this Article" so it matches with "Remove from Reading List."

Comment 10

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

(In reply to Matej Novak [:matej] from comment #7)
> Sounds like we already have this one covered, but just in case, it should be
> "Close Reading View"
> 
> It could also be "Exit Reading View"

bug 1129537 is almost closed on using "Reading View" or "Reader View".

Comment 11

[:Margaret Leibovic]

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #10)
> (In reply to Matej Novak [:matej] from comment #7)
> > Sounds like we already have this one covered, but just in case, it should be
> > "Close Reading View"
> > 
> > It could also be "Exit Reading View"
> 
> bug 1129537 is almost closed on using "Reading View" or "Reader View".

Sounds good. I'll update my patch and post for review.

Most of the strings are shared between desktop and Android, so this should take care of both of them, but I'll also audit the Android strings to make sure everything is consistent.

Comment 12

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

Reader View and Reading List

Comment 13

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1131303/margaret

/r/4027 - Bug 1131303 - Update strings for reader view/reading list UI, and add title text to reader view toolbar buttons. r=Unfocused,rnewman

Pull down this commit:

hg pull review -r ed6792c9cbf17ccd95ba6e342a0b0d462827b07f

Comment 14

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/4027/#review3187

Some definite and some potential capitalization issues here.

I note that desktop uses Title Case for menu items etc., which might conflict with Android. In that case we will need to split these string files.

::: browser/locales/en-US/chrome/browser/readerMode.properties
(Diff revision 1)
> -readerMode.exit=Exit Reader Mode
> +readerView.exit=Exit Reader View

Android style guide says to use sentence case whenever possible:

```

* Use sentence-style capitalization for all UI strings: “Words to live by.”

* Capitalize all important words in:
        App names (Calendar, Google Drive)
        Named features (Android Beam, Face Unlock)
        Proper nouns (Statue of Liberty, San Francisco Giants)
        
* Be conservative. Don't capitalize words that aren't part of a formal feature name:
        Sim card lock, Home screen, not Sim Card Lock, Home Screen.
```

Unless we're saying that "Reader View" is a 'named feature' that the user is expected to understand, these should be sentence case.

::: toolkit/locales/en-US/chrome/global/aboutReader.properties
(Diff revision 1)
> +aboutReader.footer.deleteThisArticle=Delete this Article

These should definitely be sentence case:

```
  Type controls
  Delete this article
```

"Reading list" and "reader view" are the same question about _named feature_ as in my first issue.

Comment 15

[Matej Novak [:matej]]

(In reply to Richard Newman [:rnewman] from comment #14)
> https://reviewboard.mozilla.org/r/4027/#review3187
> 
> Some definite and some potential capitalization issues here.

I'm definitely in favor of sentence case as much as possible, and we should follow the respective style guides for all strings. I would, however, consider Reader View and Reading List to be named features. That's the way I've been treating them in the first time use copy, to not only tell users about the functionality, but also to introduce the names of the features.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8566272 [details]
MozReview Request: bz://1131303/margaret

/r/4027 - Bug 1131303 - Update strings for reader view/reading list UI, and add title text to reader view toolbar buttons. r=Unfocused,rnewman

Pull down this commit:

hg pull review -r 43d1803270277fd937e12f787e62fe09aa785e06

Comment 17

[:Margaret Leibovic]

I updated "Type controls" and "Delete this article" to sentence case, but I kept "Reader View" and "Reading List" as suggested by Matej.

Comment 18

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/4027/#review3237

Ship It!

::: toolkit/components/reader/AboutReader.jsm
(Diff revision 2)
>    _updateToggleButton: function Reader_updateToggleButton() {

Sidenote: method functions don't need names these days (not for years!), and we can even do this:

```
  _updateToggleButton() {
    ...
  },
```

::: toolkit/components/reader/AboutReader.jsm
(Diff revision 2)
> +  _onList: function Reader_onList() {

s/Reader_onList//

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://reviewboard.mozilla.org/r/4027/#review3245

::: toolkit/components/reader/AboutReader.jsm
(Diff revision 2)
> +    // To be implemented (bug 1132665)

Bug 1133489

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8566272 [details]
MozReview Request: bz://1131303/margaret

https://reviewboard.mozilla.org/r/4025/#review3247

Ship It!

Comment 21

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #18)
> https://reviewboard.mozilla.org/r/4027/#review3237
> 
> Ship It!
> 
> ::: toolkit/components/reader/AboutReader.jsm
> (Diff revision 2)
> >    _updateToggleButton: function Reader_updateToggleButton() {
> 
> Sidenote: method functions don't need names these days (not for years!), and
> we can even do this:

I won't introduce new ones, but I'll file a mentor bug to get rid of these throughout the tree.

I'm assuming you meant an r+ to apply here :)

Comment 22

[Richard Newman [:rnewman]]

Comment on attachment 8566272 [details]
MozReview Request: bz://1131303/margaret

Such reviewboard, wow.

Comment 23

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/1146999121f4

Comment 24

[:Margaret Leibovic]

(FYI filed bug 1134940 for the function names issue)

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1146999121f4

Comment 26

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 39.0a1 (2015-03-09) and Aurora 38.0a2 (2015-03-09), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Comment 27

[David Lawrence [:dkl]]

Mistakenly filed against Firefox 38 and should be instead 38 Branch. Sorry for the spam. dkl

Comment 29

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1131303 - Update strings for reader view/reading list UI, and add title text to reader view toolbar buttons. r=Unfocused,rnewman