Closed Bug 1131458 Opened 5 years ago Closed 5 years ago

ReaderMode button in the URLbar isn't accessible

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- verified

People

(Reporter: Unfocused, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

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).
Flags: qe-verify+
Flags: firefox-backlog+
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
Assignee: nobody → margaret.leibovic
Flags: needinfo?(mmaslaney)
For consistency, I would use the same strings. 

Thanks!
Flags: needinfo?(mmaslaney)
/r/3753 - Bug 1131458 - Make reader mode button accessible

Pull down this commit:

hg pull review -r 655da0f968b47746b126c1e2b08648c633508095
Attachment #8562890 - Flags: review?(gijskruitbosch+bugs)
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 on attachment 8562890 [details]
MozReview Request: bz://1131458/margaret

r- because not actually keyboard-accessible yet...
Attachment #8562890 - Flags: review?(gijskruitbosch+bugs) → review-
I also realized this patch is wrong because I only updated the osx styles. Will fix!
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
Attachment #8562890 - Flags: review- → review?(gijskruitbosch+bugs)
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.
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 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. :-)
Attachment #8562890 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
(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.
See Also: → 1132678
https://hg.mozilla.org/mozilla-central/rev/0b63903a6f41
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Iteration: --- → 38.3 - 23 Feb
QA Contact: andrei.vaida
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.
Blocks: 1133732
Status: RESOLVED → VERIFIED
Depends on: 1135351
No longer blocks: 1133732
Depends on: 1133732
Depends on: 1140345
Attachment #8562890 - Attachment is obsolete: true
Attachment #8619418 - Flags: review+
You need to log in before you can comment on or make changes to this bug.