1131458 - ReaderMode button in the URLbar isn't accessible

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The ReaderMode button in the URLbar can't be focused via keyboard navigation, nor does it have any text describing it. So at the moment, it's completely inaccessible to anyone using a screenreader.

And before anyone asks: Yes, ReaderMode is useful to people using a screenreader - it cuts out extraneous crap, and can help in terms of increased contrast (etc).

Comment 1

[:Margaret Leibovic]

Thanks for filing, I can take this on.

mmaslaney, what string should we associate with the reader mode button?

For Android, we use "Enter Reader Mode" and "Exit Reader Mode":
http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties#365

Comment 2

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

For consistency, I would use the same strings. 

Thanks!

Comment 3

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1131458/margaret

/r/3753 - Bug 1131458 - Make reader mode button accessible

Pull down this commit:

hg pull review -r 655da0f968b47746b126c1e2b08648c633508095

Comment 4

[:Gijs (out for now; he/him)]

https://reviewboard.mozilla.org/r/3753/#review2967

::: browser/themes/osx/browser.css
(Diff revision 1)
> +  height: 16px;

Is the height actually necessary here?

::: browser/base/content/browser.xul
(Diff revision 1)
>                  <image id="page-report-button"

Can you ensure we file a bug about making this a button, too?

::: browser/base/content/browser.xul
(Diff revision 1)
> -                <image id="reader-mode-button"
> +                <toolbarbutton id="reader-mode-button"

Unfortunately, this is not enough to make the button keyboard-accessible, which the bug summary kind of says is part of fixing this...

IOW, you need to make sure that you can get to the button using <tab> and <shift-tab>, and that you can activate it using enter/space.

I hope that either adding tabindex="0" or the tabbable class would help, and adding a :focus style to it would probably also be necessary (so you can *see* that the button is focused), but I've not checked any further than that...

::: browser/themes/osx/browser.css
(Diff revision 1)
> -    width: 22px;
> +    width: 22px; /* 16px + 6px padding */

This seems fragile. Now that we're setting the width of the icon, this shouldn't be necessary anymore, right?

Comment 5

[:Gijs (out for now; he/him)]

Comment on attachment 8562890 [details]
MozReview Request: bz://1131458/margaret

r- because not actually keyboard-accessible yet...

Comment 6

[:Margaret Leibovic]

I also realized this patch is wrong because I only updated the osx styles. Will fix!

Comment 7

[:Margaret Leibovic]

Comment on attachment 8562890 [details]
MozReview Request: bz://1131458/margaret

/r/3753 - Bug 1131458 - Make reader mode button accessible

Pull down this commit:

hg pull review -r 6e3f1bfcc937b99dd60941b440f7791627c546d2

Comment 8

[:Margaret Leibovic]

I don't have a Windows or Linux machine handy, so I just guessed at what to do there based on other styles I saw.

I verified that with this patch I can tab to the reader mode button, see a focus ring around it, and then hitting enter loads reader mode.

Comment 9

[:Gijs (out for now; he/him)]

https://reviewboard.mozilla.org/r/3753/#review3009

::: browser/locales/jar.mn
(Diff revision 2)
> +    locale/browser/readerMode.properties              (%chrome/browser/readerMode.properties)

The JSM that requires this file doesn't depend on MOZ_SERVICES_SYNC, so the inclusion of the .properties file probably shouldn't, either?

::: browser/themes/linux/browser.css
(Diff revision 2)
>  #reader-mode-button:hover:active,
>  #reader-mode-button[readeractive] {
>     -moz-image-region: rect(0, 32px, 16px, 16px);

So unfortunately, at least on OS X, I noticed a subtle but eye-catching issue:

1. open an article on e.g. the BBC.
2. activate reader mode using the keyboard
3. deactivate reader mode using the keyboard *and be sure not to touch your mouse/touchpad while the page reloads*

When the page reloads, the reader mode icon comes back with :hover:active styling (ie orange). I checked, the readeractive attribute is gone, so that's not it. I bet that the button gets hidden when its active state is cleared, and somehow layout is not being updated correctly.

It feels hacky to have to work around this in frontend code, though we probably could relatively easily (forcing a layout flush on the button itself would probably do it). The orange is pretty eye-catching, and is clearly "wrong" as you're no longer in reader mode... but it does go away when you move the mouse in a way that causes a layout refresh (?). Thoughts?

::: browser/base/content/browser.xul
(Diff revision 2)
> -                       class="urlbar-icon"
> +                               class="tabbable"

It's vaguely sad that the urlbar-icon class is useless / actively harmful here, because semantically it should be making sense... Not sure what to do about that though; it doesn't seem to make sense to add a separate class if we're only using it here...

::: browser/themes/windows/browser.css
(Diff revision 2)
> +  padding: 0;

Windows wants border: 0 none; as well, because otherwise you get ugly win95-style 3d borders. I bet Linux is the same, but I'm rebuilding in my VM right now so I won't know until half an hour. I'll update the bug (please ping me if I forget) when I have a build.

Comment 10

[:Gijs (out for now; he/him)]

Comment on attachment 8562890 [details]
MozReview Request: bz://1131458/margaret

> So unfortunately, at least on OS X, I noticed a subtle but eye-catching issue:

This reproduces on Windows as well.

r+ on the patch with the comments taken into account, though if we're going to try to fix the orange-highlight-post-reader-mode-exit thing, I'd like to see that version of the patch again, please. :-)

Comment 11

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8562890 [details]
> MozReview Request: bz://1131458/margaret
> 
> > So unfortunately, at least on OS X, I noticed a subtle but eye-catching issue:
> 
> This reproduces on Windows as well.

And on Linux. On Linux it seems you won't need to remove the border; I don't see any for hover/active states.

Comment 12

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #9)
> https://reviewboard.mozilla.org/r/3753/#review3009
> 
> ::: browser/locales/jar.mn
> (Diff revision 2)
> > +    locale/browser/readerMode.properties              (%chrome/browser/readerMode.properties)
> 
> The JSM that requires this file doesn't depend on MOZ_SERVICES_SYNC, so the
> inclusion of the .properties file probably shouldn't, either?

Good catch! I just blindly picked an alphabetical-looking spot. Will fix.

> ::: browser/themes/linux/browser.css
> (Diff revision 2)
> >  #reader-mode-button:hover:active,
> >  #reader-mode-button[readeractive] {
> >     -moz-image-region: rect(0, 32px, 16px, 16px);
> 
> So unfortunately, at least on OS X, I noticed a subtle but eye-catching
> issue:
> 
> 1. open an article on e.g. the BBC.
> 2. activate reader mode using the keyboard
> 3. deactivate reader mode using the keyboard *and be sure not to touch your
> mouse/touchpad while the page reloads*
> 
> When the page reloads, the reader mode icon comes back with :hover:active
> styling (ie orange). I checked, the readeractive attribute is gone, so
> that's not it. I bet that the button gets hidden when its active state is
> cleared, and somehow layout is not being updated correctly.
> 
> It feels hacky to have to work around this in frontend code, though we
> probably could relatively easily (forcing a layout flush on the button
> itself would probably do it). The orange is pretty eye-catching, and is
> clearly "wrong" as you're no longer in reader mode... but it does go away
> when you move the mouse in a way that causes a layout refresh (?). Thoughts?

I can't seem to reproduce this... I tried a few times, being sure to leave my mouse/keyboard alone, but the button would reappear in the gray state. I'm going to land this patch as it is (with the jar.mn fix), and we can investigate this more in another bug if it continues to be an issue.

> ::: browser/base/content/browser.xul
> (Diff revision 2)
> > -                       class="urlbar-icon"
> > +                               class="tabbable"
> 
> It's vaguely sad that the urlbar-icon class is useless / actively harmful
> here, because semantically it should be making sense... Not sure what to do
> about that though; it doesn't seem to make sense to add a separate class if
> we're only using it here...

Yeah, however, .urlbar-icon is only used for #page-report-button at this point, so maybe it's actually time to retire that class.

In fact, its open/active/hover styles don't even seem to get applied ever, since #page-report-button has its own styles:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#2188
http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#2503

I'll file a bug to make #page-report-button into a button as well, and maybe we can just remove this class as part of that work.

Comment 13

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/0b63903a6f41

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b63903a6f41

Comment 15

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 38.0a1 (2015-01-17), using Ubuntu 14.04 (64 bit), Windows 8.1 (64 bit) and Mac OS X 10.9.5.

I was able to reproduce the issue Gijs mentioned in Comment 9 across all platforms. The Reader Mode button remains in the enabled state (orange) after disabling it via keyboard shortcut, but immediately after the Location Bar is no longer in focus, the button turns back to disabled (gray). Filed separate bug for it.

Comment 17

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1131458 - Make reader mode button accessible