735251 - Stop showing a focus ring around video/audio elements after clicking default video control buttons

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

They look ugly, are slow to draw and other browsers don't show them.

Comment 1

[Joe Drew (not getting mail)]

Stephen, should we draw these focus rings at all?

Comment 2

[Stephen Horlander [:shorlander]]

What specifically is this referring to?

Comment 3

[Stephen Horlander [:shorlander]]

To clarify, are we talking about in the UI or on web pages?

Comment 4

[:Gijs (he/him)]

Without more detail, this isn't actionable.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

For example the focus ring on on video puts a focus ring over the video controls. This looks bad.

Comment 6

[:Gijs (he/him)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> For example the focus ring on on video puts a focus ring over the video
> controls. This looks bad.

On which OS and under what circumstances? I couldn't reproduce by going to http://www.quirksmode.org/html5/tests/video.html and clicking around some video controls.

Comment 7

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> > For example the focus ring on on video puts a focus ring over the video
> > controls. This looks bad.
> 
> On which OS and under what circumstances? I couldn't reproduce by going to
> http://www.quirksmode.org/html5/tests/video.html and clicking around some
> video controls.

On OSX we do put a focus ring around videos for some reason.

I think there are two problems with our focus rings:

1) They are ugly

and

2) We seem to be more aggressive about applying them regardless of platform convention

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: Screen Shot 2016-02-24 at 1.07.59 PM.png

Here's an example on OS X from http://opus-codec.org/examples/

Comment 9

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> > > For example the focus ring on on video puts a focus ring over the video
> > > controls. This looks bad.
> > 
> > On which OS and under what circumstances? I couldn't reproduce by going to
> > http://www.quirksmode.org/html5/tests/video.html and clicking around some
> > video controls.
> 
> On OSX we do put a focus ring around videos for some reason.

That's surprising. Windows is generally much more focus-ring-happy and doesn't have them. Has anyone figured out *why* this is the case on OS X?

> I think there are two problems with our focus rings:
> 
> 1) They are ugly

Meh. I'm too used to them on Windows.

> 2) We seem to be more aggressive about applying them regardless of platform
> convention

Right, but neither of these are particularly actionable. The first needs a spec (only for OS X? Or do you think we should start showing non-native focus rings on Windows/Linux?). The second needs specificity about where and when we're wrong. For that purpose, is the video case the only concrete offender or should we turn this into a meta bug? Is the second thing specific to OS X?

FWIW, I don't think any of this is Fx::Theme - it's not like there's some CSS lurking in browser/themes/osx/ that does a "focus all the things" thing and then gets applied to web content - seems much more likely to be widget or some other bit in core that's causing this issue. Moving accordingly.

Comment 10

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Stephen Horlander [:shorlander] from comment #7)
> > (In reply to :Gijs Kruitbosch from comment #6)
> > > (In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> > > > For example the focus ring on on video puts a focus ring over the video
> > > > controls. This looks bad.
> > > 
> > > On which OS and under what circumstances? I couldn't reproduce by going to
> > > http://www.quirksmode.org/html5/tests/video.html and clicking around some
> > > video controls.
> > 
> > On OSX we do put a focus ring around videos for some reason.
> 
> That's surprising. Windows is generally much more focus-ring-happy and
> doesn't have them. Has anyone figured out *why* this is the case on OS X?

I was also surprised :)


> > 2) We seem to be more aggressive about applying them regardless of platform
> > convention
> 
> Right, but neither of these are particularly actionable. The first needs a
> spec (only for OS X? Or do you think we should start showing non-native
> focus rings on Windows/Linux?). The second needs specificity about where and
> when we're wrong. For that purpose, is the video case the only concrete
> offender or should we turn this into a meta bug? Is the second thing
> specific to OS X?

In general I think we should be platform consistent here when possible. That's assuming the platform has a coherent and consistent policy, which doesn't appear to always be the case.

In general I think we mostly do the right thing. The <video> thing seems like an anomaly. There may be others?

As for specs, I haven't investigated this recently so I looked into it a little.

--OS X--

Controlled by a System Wide Pref:
- System Preferences --> Keyboard --> Shortcuts --> Full Keyboard Access:
-- Text boxes and lists only [Default] or All Controls
-- We *do not* appear to honor this pref
-- This pref only applies to tabbing
http://people.mozilla.org/~shorlander/bugs/bug-735251-focus-OSX-System-Setting.png

Click events generally *do not* set focus on UI elements except for:
- (Some) List views that allow subsequent keyboard navigation
- Text Fields

There are some additional exceptions in Web Views (e.g. Safari):
- <select></select> elements do get click focus
-- We also do this
-- This makes sense using the same logic as List views because you might want to use your keyboard to switch selections after the initial selection
-- It does however still differ from native UI behavior
-- Historical artifact?


--Windows (Windows 7+ anyway)--

There doesn't appear to be a way to mess with this on a System Wide level?

Tab focusing targets all UI elements by default

Click events generally *do* set focus on UI elements:
- The gotcha here is that while mouse events do set focus, they do not (usually) have a visible focus ring
-- Tabbing still creates the expected dotted focus ring


--Linux--

Hard to say if this applies to most distros, but on Ubuntu:

There does not appear to be a System Wide pref for this

Click events do not appear to set focus on UI elements outside of fields

Does not appear to use a dotted focus ring

Firefox seems to use the same behavior that it does on Windows

Comment 11

[Boris Zbarsky [:bzbarsky]]

>-- We *do not* appear to honor this pref

We totally have code for honoring it....

Comment 12

[Stephen Horlander [:shorlander]]

(In reply to Boris Zbarsky [:bz] from comment #11)
> >-- We *do not* appear to honor this pref
> 
> We totally have code for honoring it....

I remember it working in the past, but it doesn't work for me on Nightly or Release. I even tried restarting between changing the preference.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  It's possible that we turned it off purposefully, now that I think about it; I recall there being bugs about how it didn't make sense to match this particular OS setting inside web pages (as opposed to UI dialogs)....

Comment 14

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #12)
> (In reply to Boris Zbarsky [:bz] from comment #11)
> > >-- We *do not* appear to honor this pref
> > 
> > We totally have code for honoring it....
> 
> I remember it working in the past, but it doesn't work for me on Nightly or
> Release. I even tried restarting between changing the preference.

It still works on XUL content, try about:preferences. It looks like we explicitly disabled this setting for html content in bug 437296... 5.5 years ago. :-\

FWIW, the 'advanced' section of the Safari prefs has a setting to toggle whether "tab" should focus everything, rather than just what the system level pref does. Judging from my partner's machine (where I am more sure that nobody bothered to toggle it; they use Firefox), it seems like that pref is off by default.

I would tend to agree with Alex Faaborg (who filed 437296) that our default is more sensible for web content than the Safari alternative.

It's not clear to me to what degree that "tab focus policy" is responsible for the issue Jeff is seeing. The summary of this bug ("don't show focus rings") is confusing, because when tabbing around I would definitely expect that we show a focus ring (otherwise, the user doesn't know where focus is...). So presumably the only issue is when we use the mouse or touch to navigate the web and we still show focus rings, specifically on OS X (doesn't seem to happen on Windows)?

In which case, are there other cases besides the video element where we do the wrong thing? From comment #10 it sounds like we mirror's safari's exception for listboxes. AFAICT the focus ring on the video element and/or controls is just wrong, but I'm still struggling to understand where else you think we're going wrong - do you think we should follow the OS pref here? Something else?

Comment 15

[Jeff Muizelaar [:jrmuizel]]

I'd be happy enough if we just fixed the media controls.

Comment 16

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #14)
> I would tend to agree with Alex Faaborg (who filed 437296) that our default
> is more sensible for web content than the Safari alternative.

Yeah, it's trade-off. Do we favor the potential user expectation of the System pref working or that tab will tab through all elements in web content. The current behavior is fine if we did it intentionally with solid rationale. As long as it isn't a bug :)


> It's not clear to me to what degree that "tab focus policy" is responsible
> for the issue Jeff is seeing. The summary of this bug ("don't show focus
> rings") is confusing, because when tabbing around I would definitely expect
> that we show a focus ring (otherwise, the user doesn't know where focus
> is...). So presumably the only issue is when we use the mouse or touch to
> navigate the web and we still show focus rings, specifically on OS X
> (doesn't seem to happen on Windows)?

I think that you are correct. The specific issue with the <video> control is showing the focus ring on mouse/touch events when we shouldn't. I seemed to remember there being more instances of this, but I can't find any at the moment. The <video> element showing a focus ring on click seems like an anomaly.

I would fully expect to see the focus ring around <video> elements while tabbing through pages though.


> In which case, are there other cases besides the video element where we do
> the wrong thing? From comment #10 it sounds like we mirror's safari's
> exception for listboxes. AFAICT the focus ring on the video element and/or
> controls is just wrong, but I'm still struggling to understand where else
> you think we're going wrong - do you think we should follow the OS pref
> here? Something else?

As I may have said somewhere above we have a few types of problems with focus rings visually:

1) They sometimes appear when you don't expect them to (<video> instance)
2) They are pretty bad looking

The second point has a bit of nuance to it and maybe some subjectivity ;) 

For one it depends on platform. I can't remember a time when OS X natively used a dotted outline for focus. It was always some kind of blue glow, previously feathered, now solid. Windows can't seem to decide if it wants to pretty it up or not. File Explorer for example uses solid blue outlines for focus. Other apps seems to mostly be using the traditional dotted outline. Edge content is using a dashed outline that is just slightly cleaner looking. Linux ¯\_(ツ)_/¯

Another more Firefox specific problem is the way we apply the focus ring to elements like <input type="button"> and <select>. We apply the ring to the inside content of the element instead of the outside boundary of the element itself as you would expect.

For example: http://people.mozilla.org/~shorlander/bugs/bug-735251-focus-select-ring.png

You can find many an angry StackOverflow topic bemoaning this fact and how to fix it. The CSS to change it is also different between elements. The focus ring on <select> in particular seems to require this rather odd syntax (hack?):

> select:-moz-focusring {
>   color: transparent;
>   text-shadow:0 0 0 #000; /* your normal text color here */
> }
> select:-moz-focusring * {
>   color: #000; /* your normal text color here */
>   text-shadow: none;
> }

I think the undesirable way it looks — in combination with the CSS hoops you have to jump through to alter it — is why you find more instances than you would like of people disabling the focus ring entirely.

So three actionable ways to improve focus rings:

1) Make sure they only appear when they should — in this case on the <video> and <audio> elements on OS X
2) Fix instances where they appear around the label/inner-content instead of around the element, and make the CSS for focus ring appearance consistent
3) Improve the visual appearance of the focus ring — especially on OS X

Comment 17

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #16)
> 1) Make sure they only appear when they should — in this case on the <video>
> and <audio> elements on OS X

OK, let's do that in this bug. I have a suspicion of what the culprit is, see upcoming comment.

> 2) Fix instances where they appear around the label/inner-content instead of
> around the element, and make the CSS for focus ring appearance consistent

OK. Filed bug 1251282 for this.

> 3) Improve the visual appearance of the focus ring — especially on OS X

This is bug 53927 for OS X, at least. (yay, 5-digit bug!)

Comment 18

[:Gijs (he/him)]

So for this bug, I *suspect* this is because the buttons in the video control are inside native anonymous content and the focus manager balks at trying to focus them. Also relevant: bug 486899, bug 492415, bug 418521. There was a suspicious tabindex=0 setting that the video controls used to do, implemented in one of those bugs, that I thought might be responsible, but that was removed in bug 839378... which does look highly related to what's going on here, also because it seems to have stuck the tabindex into native code instead (?).

:roc, any chance you can take a look and comment as to what's going on with focus on media elements being "propagated" from the media controls to the media element on clicks?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not an expert on focus, really.

Even if focus manager supported focusing individual video controls, I don't think that makes sense. Logically you want to focus the whole video player so all keys get a chance to be processed by any of the controls. So I think our current focus setup is actually the right one.

Certainly the easiest fix, and possibly the best fix, would be to just add <video> and <audio> to
:-moz-focusring:not(input):not(button):not(select):not(textarea):not(iframe):not(frame):not(body):not(html)
in html.css.

Comment 20

[:Gijs (he/him)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> I'm not an expert on focus, really.
> 
> Even if focus manager supported focusing individual video controls, I don't
> think that makes sense. Logically you want to focus the whole video player
> so all keys get a chance to be processed by any of the controls. So I think
> our current focus setup is actually the right one.

I don't think anyone is arguing the controls should get focus instead; all we'd like to do is not draw the focus outline when focus is obtained through the mouse. In fact, I'm not sure that clicking on a video control should move focus to the video at all (just like certain other elements don't get focus on OS X when you click them, see comment #10 ).

> Certainly the easiest fix, and possibly the best fix, would be to just add
> <video> and <audio> to
> :-moz-focusring:not(input):not(button):not(select):not(textarea):not(iframe):
> not(frame):not(body):not(html)
> in html.css.

I tried this and it works, but it also means I can no longer see the focus if I focus the video element using the keyboard, ie it eliminates the focus rectangle completely for <audio> and <video> elements, which I don't think is right either.

Comment 21

[:Gijs (he/him)]

Hm, looking at the patches from bug 577316 and bug 564277, it seems IsHTMLFocusable should return false if aWithMouse is true.

Comment 22

[:Gijs (he/him)]

Err, that one. Sorry for bugspam. I'll try to get a strawman patch in a minute or two.

Comment 23

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 735251 - make HTMLMediaElement unfocusable by mouse on OS X, r?bz

Review commit: https://reviewboard.mozilla.org/r/36687/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36687/

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8723717 [details]
MozReview Request: Bug 735251 - make HTMLMediaElement unfocusable by mouse on OS X, r?bz

https://reviewboard.mozilla.org/r/36687/#review33259

So with this patch, if I start a video by clicking on it and then fullscreen it, can I stop it by hitting spacebar?

Comment 25

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #24)
> Comment on attachment 8723717 [details]
> MozReview Request: Bug 735251 - make HTMLMediaElement unfocusable by mouse
> on OS X, r?bz
> 
> https://reviewboard.mozilla.org/r/36687/#review33259
> 
> So with this patch, if I start a video by clicking on it and then fullscreen
> it, can I stop it by hitting spacebar?

Not on mac, no.

I guess I was wrong in comment #21 (ie this approach is just not correct), and we should instead be working towards what we do on Windows, which is focus it but only paint a focus rectangle when the focus got there by keyboard, not when that happens through the mouse.

Not sure offhand why that code is not applying on OS X for the video controls - it seems to work OK for e.g. links in unstyled documents, and there's no focus ring on Windows, so presumably it works there too... From skim-reading the patch in bug 418521 it seems like somehow nsGlobalWindow's ShouldShowFocusRing is triggered to be true, but it's not clear to me why that would happen.

Comment 26

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #25)
> Not sure offhand why that code is not applying on OS X for the video
> controls - it seems to work OK for e.g. links in unstyled documents, and
> there's no focus ring on Windows, so presumably it works there too... From
> skim-reading the patch in bug 418521 it seems like somehow nsGlobalWindow's
> ShouldShowFocusRing is triggered to be true, but it's not clear to me why
> that would happen.

Looks like this code:

https://dxr.mozilla.org/mozilla-central/rev/918df3a0bc1c4d07299e4f66274a7da923534577/dom/base/nsGlobalWindow.cpp#9666-9674

needs more exceptions than just the IsLink case, because it seems that when we hit ShouldShowFocusRings, mShowFocusRingForContent is true, which only gets set in there.

Comment 27

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 735251 - don't show focusrings on HTML video / audio elements on non-Windows, r?bz

Review commit: https://reviewboard.mozilla.org/r/39201/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39201/

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8728964 [details]
MozReview Request: Bug 735251 - don't show focusrings on HTML video / audio elements on non-Windows, r?bz

https://reviewboard.mozilla.org/r/39201/#review36003

Commit message should mention something about focusing by mouse.

::: dom/base/nsGlobalWindow.cpp:9637
(Diff revision 1)
> +  if (aNode) {

Please just take an early return if !aNode and outdent.  Then you can do:

    if (!aNode) {
      return true;
    }

    return !IsLink(aNode) && !aNode->IsAnyOf....();

::: dom/base/nsGlobalWindow.cpp:9639
(Diff revision 1)
> +        aNode->IsHTMLElement(nsGkAtoms::video) ||

aNode->IsAnyOfHTMLElements(nsGkAtoms::video, nsGkAtoms::audio), please.

r=me with those fixed.

Comment 29

[Pulsebot]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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