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

RESOLVED FIXED in mozilla8



CSS Parsing and Computation
6 years ago
5 years ago


(Reporter: bz, Assigned: bz)



Mac OS X
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(1 attachment, 4 obsolete attachments)

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.
Created attachment 527914 [details] [diff] [review]
Like so
Attachment #527914 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Priority: -- → P2
Whiteboard: [need review]


6 years ago
Blocks: 267119


6 years ago
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.
Created attachment 528124 [details] [diff] [review]
slightly more conservative fix
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]
Created attachment 528704 [details] [diff] [review]
With that change
Attachment #528124 - Attachment is obsolete: true
This landed:

and caused orange.  So a bustage fix was applied:

But there was still orange, so khuey backed the whole thing out:
Whiteboard: [need landing]
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.
Created attachment 528824 [details] [diff] [review]
This passes try
Attachment #528704 - Attachment is obsolete: true
Whiteboard: [need landing]
Created attachment 547710 [details] [diff] [review]
Merged to tip
Attachment #528824 - Attachment is obsolete: true
Last Resolved: 6 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.

Comment 17

6 years ago
(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.
Keywords: dev-doc-needed → dev-doc-complete


5 years ago
Depends on: 786428
You need to log in before you can comment on or make changes to this bug.