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

VERIFIED FIXED in Firefox 38

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
mozilla38
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
These aren't links, they're buttons. This is more semantically correct, and it will give us better accessibility support.
(Assignee)

Comment 1

4 years ago
Created attachment 8564377 [details] [diff] [review]
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).
Attachment #8564377 - Flags: review?(bnicholson)
(Assignee)

Updated

4 years ago
Blocks: 1131303
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)?
Attachment #8564377 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 3

4 years ago
(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.
(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?
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
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...
(Assignee)

Comment 7

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/d71a3d134bf3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Status: RESOLVED → VERIFIED
status-firefox38: fixed → verified
status-firefox39: --- → verified
Flags: qe-verify+
QA Contact: andrei.vaida
You need to log in before you can comment on or make changes to this bug.