1140340 - Add Reader Mode menu item to the menu bar

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P3)

Description

[Dão Gottwald [:dao]]

As discussed in bug 1132678 comment 28 and below, having the icon in the location bar as the only entry point for reader mode seems suboptimal from an accessibility point of view. Seems like the most straightforward way would be to add something to the menu bar, e.g. View > Enter/Exit Reader View.

Comment 1

[:Gijs (he/him)]

This doesn't address the issue of discoverability. Reader mode is not available for most pages, and the icon is a very clear indicator that it is. The menu item wouldn't be; the user would have to check and open the menu to figure out if it was available.

Comment 2

[:Gijs (he/him)]

(likewise, in the case of non-sighted users, to verify if it was on or off)

Comment 3

[Dão Gottwald [:dao]]

This bug's goal isn't to remove the icon, it would stay there as an indicator.

Comment 4

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #3)
> This bug's goal isn't to remove the icon, it would stay there as an
> indicator.

You indicated in bug 1135351 comment 16 that you want to change it back to being an image. That will mean it is no longer accessible as an indicator.

Comment 5

[Dão Gottwald [:dao]]

Do you mean that the indicator being a toolbar button makes screen readers announce state changes?

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> Do you mean that the indicator being a toolbar button makes screen readers
> announce state changes?

Depends what you mean.

It won't automatically announce the button's being shown and hidden (see also e.g. bug 1067446 for context about "announcing things being hidden is hard") automatically. We'd need to use a live region for that, irrespective of the item being an image or a toolbarbutton.

The difference with using a toolbarbutton is that we:
a) can have it in the regular tab order, making it much easier to find and activate;
b) we could change its role to a toggle button that has state (pressed / not pressed), which I expect isn't possible with a plain image because its role is apparently taken from the binding (see the discussion in bug 1123771). That would correctly expose whether reader mode is enabled or not, which combined with (a) makes it much more usable.

Shameless plug: once we decide what to do here, we should fix bug 1123760, too.

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #6)
> The difference with using a toolbarbutton is that we:
> a) can have it in the regular tab order, making it much easier to find and
> activate;

So you need to hit Accel-L (not exactly an intuitive start), then Tab, carefully, maybe multiple times if there are other tabbable buttons. If you find the button, you can then hit Enter. If you don't find the button, you end up in the search bar. This doesn't seem ideal, and also makes moving focus from the location bar to the search bar more cumbersome.

With the menu item, if you know that you're not in reader mode and you want to enter it, you just hit Alt-V + R. The failure case is simpler here: if reader mode is unavailable, the command will fail just like for other menu items. Granted, if you want to know whether reader mode is active, you're back to a workflow that's roughly as cumbersome as dealing with the url bar icon.

> b) we could change its role to a toggle button that has state (pressed / not
> pressed), which I expect isn't possible with a plain image because its role
> is apparently taken from the binding (see the discussion in bug 1123771).
> That would correctly expose whether reader mode is enabled or not, which
> combined with (a) makes it much more usable.

We already expose this through the tooltip (Enter/Exit Reader View).

Comment 8

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > The difference with using a toolbarbutton is that we:
> > a) can have it in the regular tab order, making it much easier to find and
> > activate;
> 
> So you need to hit Accel-L (not exactly an intuitive start), then Tab,
> carefully, maybe multiple times if there are other tabbable buttons. If you
> find the button, you can then hit Enter. If you don't find the button, you
> end up in the search bar. This doesn't seem ideal, and also makes moving
> focus from the location bar to the search bar more cumbersome.
> 
> With the menu item, if you know that you're not in reader mode and you want
> to enter it, you just hit Alt-V + R. The failure case is simpler here: if
> reader mode is unavailable, the command will fail just like for other menu
> items. Granted, if you want to know whether reader mode is active, you're
> back to a workflow that's roughly as cumbersome as dealing with the url bar
> icon.

Hence my pointing out this loses discoverability - not the toggle functionality... I'm not sure why you claim accel-l is not discoverable - keyboard/AT users will likely already be using either that or F6 to shift focus between the location bar, search bar and content. The common case here is still that where the icon in question is the first one, which means one extra tab keypress beyond accel-l.

Comment 9

[jaramat]

Another thing going for this bug is that the Menu Bar is a classic way to access every feature and function available in the browser.

This isn't about discoverability. Everything - whether it is actually used by the masses or not and whether it already has an assigned keyboard shortcut or not - should be present there. The Menu Bar is a go-to place when you can't find a certain button or feature, because it's not customizable (not out of the box, anyway) and therefore users cannot accidentally remove a menu entry from there.

I vote for adding a "Reader Mode" to the Menu Bar under the View submenu, because it doesn't make sense not to do it. Also, there isn't other way to enter the reader mode other than to click the button in the URL bar.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Michael: Thoughts on this? If we do want it, we need to sort out strings ASAP (label and access key).

I'm thinking we do need this menu item. And also that the button in the urlbar needs to be accessible regardless.

Comment 11

[Dão Gottwald [:dao]]

If string freeze is a problem, we could probably reuse readerView.enter and readerView.close from aboutReader.properties and omit access keys as a short-term hack for one release.

Comment 12

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

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #10)
> Michael: Thoughts on this? If we do want it, we need to sort out strings
> ASAP (label and access key).
> 
> I'm thinking we do need this menu item. And also that the button in the
> urlbar needs to be accessible regardless.

Regarding accessibility, I would suggest we add the "Enter" and "Exit Reader View" functionality to the menu bar. I would use the same tooltip strings as the menu item labels.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dão Gottwald [:dao] from comment #11)
> If string freeze is a problem, we could probably reuse readerView.enter and
> readerView.close from aboutReader.properties and omit access keys as a
> short-term hack for one release.

We have a l10n extension for the ReadingList project (which is aimed at 38), so I've been pre-landing strings.

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from comment #12)
> I would suggest we add the "Enter" and "Exit Reader
> View" functionality to the menu bar. I would use the same tooltip strings as
> the menu item labels.

This would break consistency - all other menu items use a checkbox for things that are toggled on/off.
(Only exception I know of is Full Screen on OSX, which changes its label to match platform conventions because its an OS feature on OSX)

Comment 14

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

Attachment: Patch without accesskeys

Comment 15

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

Attachment: Patch adding accesskeys (for 39+)

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8580989 [details] [diff] [review]
Patch without accesskeys

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

::: browser/modules/ReaderParent.jsm
@@ +119,5 @@
>      if (browser.currentURI.spec.startsWith("about:reader")) {
>        button.setAttribute("readeractive", true);
>        button.hidden = false;
>        button.setAttribute("tooltiptext", gStringBundle.GetStringFromName("readerView.close"));
> +      command.setAttribute("label", gStringBundle.GetStringFromName("readerView.close"));

If it's only for 38 this is fine, but if we keep this code, could you please use a variable to avoid duplicating the gStringBundle.GetStringFromName call?

@@ +125,5 @@
>      } else {
>        button.removeAttribute("readeractive");
>        button.setAttribute("tooltiptext", gStringBundle.GetStringFromName("readerView.enter"));
>        button.hidden = !browser.isArticle;
> +      command.setAttribute("label", gStringBundle.GetStringFromName("readerView.enter"));

Same here.

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8580991 [details] [diff] [review]
Patch adding accesskeys (for 39+)

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

::: toolkit/locales/en-US/chrome/global/aboutReader.properties
@@ +27,5 @@
>  aboutReader.toolbar.share=Share
>  
>  aboutReader.footer.deleteThisArticle=Delete this article
>  
>  # Reader View toolbar button

I more or less expected the strings for the menuitem to be in a .dtd file, but given that we are updating the label dynamically, having them in a .properties file makes sense, and I guess keeping the accesskey close to the labels is better.

I think we should improve the localization note above these strings, as "Reader View toolbar button" is now misleading.

Comment 18

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

Attachment: Patch for 38

Comment 19

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

Attachment: Patch for 39

Comment 20

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

https://hg.mozilla.org/integration/fx-team/rev/86d77e15b8e1

Comment 21

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/86d77e15b8e1

Comment 22

[:Gijs (he/him)]

Can we already ask for uplift to aurora on the patch for 38?

Comment 23

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

Attachment: Patch for 38 (no dtd changes)

Nothing more is needed for this patch to be uplifted, but it depends on bug 1140345 and it's predecessors to be uplifted first.

Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: no accessible way to open the reader mode
[Describe test coverage new/current, TreeHerder]: manual testing
[Risks and why]: none expected
[String/UUID change made/needed]: none

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d7569b6d46d4

Comment 25

[Stefan Plewako [:stef]]

BTW: readerView.accesskey should not be shared between readerView.enter and readerView.close

Comment 26

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

(In reply to Stefan Plewako [:stef] from comment #25)
> BTW: readerView.accesskey should not be shared between readerView.enter and
> readerView.close

Thank you for your comment. I've filed bug 1149179 and hope to get it in to 39 before it uplifts to Aurora.