fix: add ui focus indicator for keyboard navigation

[CodeDotJS]

Fixes #2863

cc @joeyparrish @michellezhuogg

[michellezhuogg]

Hmmm... We already have focus indicator with blue lines, on the demo page in Chrome and Firefox.
Are you not seeing focus indicator from the master branch? If so, let us know what browsers you're using and how we can reproduce that. Thank you!

[joeyparrish]

I don't have the blue focus indicator on my Chromebook. If I pause the video on a lightly-colored scene, I can see a subtle black outline instead.

This is the case in v2.5, v3.0, and nightly.

[CodeDotJS]

Just installed Firefox (86.0) to test this, and yes, there's a blue focus indicator, but not in Chrome (89.0.4389.90) BTW.

The bug is reproducible. I'll get back with the explanation in a while.

[joeyparrish]

Just installed Firefox (86.0) to test this, and yes, there's a blue focus indicator, but not in Chrome (89.0.4389.90) BTW.

The bug is reproducible. I'll get back with the explanation in a while.

I've been wondering if Highlight as a color is not supported correctly in Chrome.

[joeyparrish]

It looks like we have focus styles already. Could those could be fixed instead?

https://github.com/google/shaka-player/blob/e7173960a7249fdb369d942976d2598a7374d981/ui/less/buttons.less#L89-L107

[joeyparrish]

I've been wondering if Highlight as a color is not supported correctly in Chrome.

Looks like -webkit-focus-ring-color is replacing Highlight. Removing -webkit-focus-ring-color works. Not sure if there's a reason we need it, or if -webkit-focus-ring-color is only black on ChromeOS. I'm going to use our test lab to look into this a little more on different operating systems and browsers.

[joeyparrish]

Both -webkit-focus-ring-color and Highlight work on Safari. Both work on Chrome on Mac, though they are different shades of blue. Both work on Firefox on Mac.

Starting to smell like a bug in ChromeOS.

[joeyparrish]

-webkit-focus-ring-color is black in Chrome on Windows, as it is in ChromeOS. Highlight works fine on Windows.

With these colors, Chrome on Linux appears to behave the same as Windows & ChromeOS.

A revision to my earlier statements about Firefox: it is ignoring -webkit-focus-ring-color, since it has no WebKit ancestry. But I'm also not sure Highlight is having any effect. Without it, the default outline color for focus is the same.

So I think we should just delete the webkit prefixed color and call it a day.

[joeyparrish]

I'm going to close this PR and remove the -webkit-focus-ring-color style which seems to be causing the problem. Thanks for contributing, and for giving me the impetus to dig into the issue more deeply.

[lucksy]

Sorry, if this was already closed, But not sure 'Highlight' is a color keyword. If we want to give browser to handle the color. 'invert' might be the keyword value.

outline-color: invert; expected to perform a color inversion on the pixels on the screen.

outline: 1px solid invert;

https://drafts.csswg.org/css-ui-3/#valdef-outline-color-invert

[joeyparrish]

Highlight appears to be a valid color keyword: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value

[lucksy]

That explained it. Is't it?

"Highlight ~ Background of selected items"

Since we don't have background color for the buttons, outline color will not set.

Edited: I overlooked, keyword "invert" browser support is terrible. So no to 'invert'

[joeyparrish]

Hightight is a real thing you can use. Here's a demo:

outline: 1px solid red;
outline: 1px solid Highlight;

You'll see light blue, from the highlight color. Compare with this:

outline: 1px solid red;
outline: 1px solid foo;

You'll see red. Since "foo" is definitely not a recognized color, it will fall back to red. Also, the CSS debugger in Chrome will show a "warning" icon next to the "foo" line, with a tooltip of "invalid property value". It does not do this for Highlight.

[CodeDotJS]

@joeyparrish although the PR is close, this is what I found. Sorry, I'm a little late, there was a power cut since morning.

I tested the focus feature with the following piece of code -
You can test it using -

<html>

<head>
    <style type="text/css">
    button:focus+div {
        outline: 1px solid Highlight;
        outline: 1px solid -webkit-focus-ring-color;
    }
    </style>
</head>

<body>
    <div><button>TEST</button>
        <div><br>FOCUS</div>
    </div>
</body>

</html>

• Result in Chrome
  

• Result in Firefox
  

Now, coming to Highlight and -webkit-focus-ring-color -

When I removed outline: 1px solid Highlight;, the focus didn't work in FireFox but it was working in Chrome, so I tested it again by removing outline: 1px auto -webkit-focus-ring-color; and switching back to Highlight - now the focus was working well in both browsers.

I'd still recommend having a focus of different color especially for the video player instead of the default blue, setting a browser-independent focus style would probably be a good idea.

Let me know what you think.

[joeyparrish]

Sounds like you came to the same conclusions in your testing: Highlight works, but -webkit-focus-ring-color doesn't. I think we'll stick with Highlight for now.

If we're going to change to an explicit color that's not browser-dependent or OS-dependent like Highlight is, then we'd have to talk to our UX people to clarify what the color should be. The UI as it is today is built to mimic the style of the built-in controls in Chrome, so Highlight is probably the most appropriate.

[CodeDotJS]

Yes, as of now I went with the Highlight and updated the changes before commenting here, if you don't mind, I can push it.

Lemme know if that's okay.

Didn't notice the changes have been already pushed.

[joeyparrish]

Sorry I got ahead of you on this!