Last Comment Bug 652301 - Focused non-link <a> does not show a focus ring
: Focused non-link <a> does not show a focus ring
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Mac OS X
: P2 normal (vote)
: mozilla8
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 786428
Blocks: 267119
  Show dependency treegraph
 
Reported: 2011-04-22 21:47 PDT by Boris Zbarsky [:bz]
Modified: 2012-08-28 13:49 PDT (History)
6 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Like so (4.66 KB, patch)
2011-04-22 23:21 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
slightly more conservative fix (3.97 KB, patch)
2011-04-25 10:22 PDT, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review
With that change (4.48 KB, patch)
2011-04-27 13:52 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
This passes try (4.42 KB, patch)
2011-04-28 04:55 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Merged to tip (4.90 KB, patch)
2011-07-22 09:13 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2011-04-22 21:47:41 PDT
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?
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-22 22:21:43 PDT
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.
Comment 2 Boris Zbarsky [:bz] 2011-04-22 22:31:54 PDT
> (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.
Comment 3 Boris Zbarsky [:bz] 2011-04-22 23:21:23 PDT
Created attachment 527914 [details] [diff] [review]
Like so
Comment 4 Boris Zbarsky [:bz] 2011-04-25 08:52:47 PDT
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?
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-25 08:59:53 PDT
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.
Comment 6 Boris Zbarsky [:bz] 2011-04-25 10:22:01 PDT
Created attachment 528124 [details] [diff] [review]
slightly more conservative fix
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-25 12:02:05 PDT
Comment on attachment 528124 [details] [diff] [review]
slightly more conservative fix

r=dbaron, though I might have written:

:-moz-focusring:not(input):not(button):not(select):not(textarea)


Hopefully the tests won't have reliability issues.
Comment 8 Boris Zbarsky [:bz] 2011-04-25 12:08:52 PDT
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.  :(
Comment 9 Boris Zbarsky [:bz] 2011-04-27 13:52:33 PDT
Created attachment 528704 [details] [diff] [review]
With that change
Comment 11 Boris Zbarsky [:bz] 2011-04-27 20:40:04 PDT
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.
Comment 12 Boris Zbarsky [:bz] 2011-04-28 04:55:43 PDT
Created attachment 528824 [details] [diff] [review]
This passes try
Comment 13 Boris Zbarsky [:bz] 2011-07-22 09:13:12 PDT
Created attachment 547710 [details] [diff] [review]
Merged to tip
Comment 15 Eric Shepherd [:sheppy] 2011-08-22 14:07:04 PDT
I'm not seeing anything that obviously requires developer documentation here. Can someone tell me what needs to be written up? Thanks.
Comment 16 Boris Zbarsky [:bz] 2011-08-22 14:13:43 PDT
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 j.j. 2011-08-22 14:34:19 PDT
(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 ...
Comment 18 Boris Zbarsky [:bz] 2011-08-22 14:49:12 PDT
Yes, everything that wasn't in the whitelist before and is not in the blacklist now.
Comment 19 Eric Shepherd [:sheppy] 2011-10-18 09:25:44 PDT
This still doesn't seem like something that requires developer documentation; this is just a cosmetic issue, isn't it?
Comment 20 Boris Zbarsky [:bz] 2011-10-18 09:54:04 PDT
For web developers, those need documentation...  In particular, we've had a number of web developer bug reports about the new behavior.
Comment 21 Eric Shepherd [:sheppy] 2011-10-18 10:05:48 PDT
I've added documentation explaining this here:

https://developer.mozilla.org/en/Focus_management_in_HTML

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.

Note You need to log in before you can comment on or make changes to this bug.