Closed Bug 652301 Opened 10 years ago Closed 10 years ago

Focused non-link <a> does not show a focus ring


(Core :: CSS Parsing and Computation, defect, P2)






(Reporter: bzbarsky, Assigned: bzbarsky)



(Keywords: dev-doc-complete)


(1 file, 4 obsolete files)

It looks like we have two selectors in our UA stylesheet that show focus rings.  ua.css has "*|*:-moz-any-link:-moz-focusring" while html.css has a long list of element names _not_ including "a".

So given this markup:

  <a tabindex="0">anchor</a>

I can tab to the anchor and it gets focus but does not draw a focus ring.  This seems undesirable.

Is there a reason not to add "a" to the list in html.css?  And should we really be restricting that list to certain HTML elements?
Now that outlines don't cause scrollable overflow (right?) I think there's less risk of breakage from allowing more things to draw focus rings.  We probably should.
> (right?)

data:text/html,<div style="overflow: auto"><div style="outline: 10px solid black"><br><br><br><br><br><br><br>

says yes.

The only real risk is adding another *|*:something selector, but we'd be removing one too (the :-moz-any-link one), so maybe it's ok.
Attached patch Like so (obsolete) — Splinter Review
Attachment #527914 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Priority: -- → P2
Whiteboard: [need review]
Blocks: 267119
Keywords: dev-doc-needed
This is no good, sadly... in particular, it makes us put focus outlines on <input> (which we did not before) and some XUL stuff (so e.g. the text in XUL alerts gets focus outline, the text input inside the url bar gets a focus outline, etc).

I suppose I could restrict the outlining to links and HTML elements except input, button, select, textarea.  Or I could just add "a" to the list and have random tag names not work.  David, preferences?
Obviously adding a to the list is the safest, but trying to get all HTML elements except for the known one we shouldn't do sounds worth a try.
Attached patch slightly more conservative fix (obsolete) — Splinter Review
Attachment #527914 - Attachment is obsolete: true
Attachment #527914 - Flags: review?(dbaron)
Attachment #528124 - Flags: review?(dbaron)
Comment on attachment 528124 [details] [diff] [review]
slightly more conservative fix

r=dbaron, though I might have written:


Hopefully the tests won't have reliability issues.
Attachment #528124 - Flags: review?(dbaron) → review+
Hmm.  I'll switch to that :not(a):not(b) pattern, yes.

The tests do need the reftest window to have focus, but so do other existing reftests.  :(
Whiteboard: [need review] → [need landing]
Attached patch With that change (obsolete) — Splinter Review
Attachment #528124 - Attachment is obsolete: true
OK, looks like in addition to frame and iframe I need body and html....

Pushed and updated patch to try; will attach if it cycles green.
Attached patch This passes try (obsolete) — Splinter Review
Attachment #528704 - Attachment is obsolete: true
Whiteboard: [need landing]
Attached patch Merged to tipSplinter Review
Attachment #528824 - Attachment is obsolete: true
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla8
Flags: in-testsuite+
I'm not seeing anything that obviously requires developer documentation here. Can someone tell me what needs to be written up? Thanks.
The main change that needs documenting is that now unknown HTML elements that are made focusable through the use of tabindex will now show a focus outline.  They didn't use to do that.
(In reply to Boris Zbarsky (:bz) from comment #16)
> The main change that needs documenting is that now unknown HTML elements

... including all HTML5 elements ...
Yes, everything that wasn't in the whitelist before and is not in the blacklist now.
This still doesn't seem like something that requires developer documentation; this is just a cosmetic issue, isn't it?
For web developers, those need documentation...  In particular, we've had a number of web developer bug reports about the new behavior.
I've added documentation explaining this here:

Please let me know if this is inadequate and feel free to augment if more is needed.

This is also mentioned on Firefox 8 for developers.
Depends on: 786428
You need to log in before you can comment on or make changes to this bug.