1132307 - Use <button> elements instead of <a> elements in reader mode toolbar

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[:Margaret Leibovic]

These aren't links, they're buttons. This is more semantically correct, and it will give us better accessibility support.

Comment 1

[:Margaret Leibovic]

Attachment: Use <button> elements instead of <a> elements in reader mode toolbar

On IRC, mmaslaney told me that we should be using a normal pointer, not a cursor, so I fixed that.

In bug 1131303, I'll use the strings we settle on to add aria-label attributes to these buttons, which should make them accessible for screen readers (although the font style popup is pretty useless if you're blind).

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 8564377 [details] [diff] [review]
Use <button> elements instead of <a> elements in reader mode toolbar

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

::: mobile/android/themes/core/aboutReader.css
@@ +359,5 @@
> +  border: 0;
> +  width: 100%;
> +}
> +
> +/* Rmove dotted border when button is focused */

Nit: Rmove -> Remove

@@ +360,5 @@
> +  width: 100%;
> +}
> +
> +/* Rmove dotted border when button is focused */
> +.button::-moz-focus-inner {

Why do we want to remove the inner border when the button is focused? Won't that make it impossible to determine which element is focused when tabbing (for Android keyboard users)?

Comment 3

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #2)

> @@ +360,5 @@
> > +  width: 100%;
> > +}
> > +
> > +/* Rmove dotted border when button is focused */
> > +.button::-moz-focus-inner {
> 
> Why do we want to remove the inner border when the button is focused? Won't
> that make it impossible to determine which element is focused when tabbing
> (for Android keyboard users)?

I did this because I found that with this change to a button the focus ring was appearing when users clicked on this.

I think that if we do want a focus style, it should probably be a different background or something that matches desktop.

Do you think tabbing with a keyboard even works now? That seems like a very under-tested feature.

Comment 4

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #3)
> Do you think tabbing with a keyboard even works now? That seems like a very
> under-tested feature.

I think it does, but I have no idea how many people use it. I wonder why clicking a button on Android gives it focus, but desktop doesn't?

Comment 5

[:Margaret Leibovic]

Hm, I do see a focus style on desktop if I tab to these controls: http://cl.ly/image/3G2w0N0x0j3v

I probably should just figure out why the button is getting focus when clicked on mobile, but not on desktop.

Comment 6

[:Margaret Leibovic]

But on the bright side, this appears to make these toolbar buttons actually keyboard accessible, which they weren't before. So maybe our keyboard users on Android couldn't have navigated to these before anyway...

Comment 7

[:Margaret Leibovic]

I decided that I need to land this, since it blocks me making the desktop toolbar buttons accessible. I'll file a follow-up to look into the focus issue on Android, but I think not having focus styles on the buttons isn't a major blocker for us.

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

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Andrei Vaida [:avaida]]

This is verified fixed on Nightly 39.0a1 (2015-03-09) and Aurora 38.0a2 (2015-03-09), using Windows 7 (x64), Ubuntu 14.04 (x86) and Mac OS X 10.9.5. The focus related issue mentioned in Comment 5 was filed as Bug 1135589.