Fix: add support for :focus-visible selector #5483

gjanblaszczyk · 2018-10-05T22:34:44Z

Description

A proposed fix for #5474
It based on :focus-visible selector.
For now, it works only on Chrome. You have to enable "Experimental Web Platform features" under the Chrome settings (chrome://flags/) to be an able test this solution.
The PR doesn't break other browsers.

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chome, Firefox, IE)
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

sync with origin branch

OwenEdwards · 2018-10-07T22:44:04Z

"The PR doesn't break other browsers" - is that true? Doesn't this PR disable focus outlines if :focus-visible is not present, so it will disable the outline for all browsers except Chrome with "Experimental Web Platform features" under the Chrome settings (chrome://flags/)" enabled?

We can't do that and break accessibility across all browsers except Chrome with an Experimental flag enabled. I thought this PR was going to integrate the ":focus-visible" polyfill.

gjanblaszczyk · 2018-10-08T09:07:33Z

Hi Owen, Thanks for the comment.
For your question: "The PR doesn't break other browsers" - is that true?
Yes, it shouldn't break any browsers at least according to some comments from focus-visible polyfill project.
See here:
https://github.com/WICG/focus-visible/blame/master/README.md#L134
I used the same CSS rule:
/* Remove default focus styles for mouse users ONLY if :focus-visible is supported on this platform. */ button:focus:not(:focus-visible) { … }
I think also @gkatsev suggesting using:
:focus:not(:focus-visible) { outline: none; }
#5474 (comment)

I created also codepen demo example here:
https://codepen.io/grbla/pen/PybOYX

asd29696 · 2018-10-08T10:05:55Z

Owen Edwards <[email protected]> 於 2018年10月8日週一 6:44 寫道：

…

"The PR doesn't break other browsers" - is that true? Doesn't this PR disable focus outlines if :focus-visible is not present, so it will disable the outline for *all* browsers *except* Chrome with *"Experimental Web Platform features" under the Chrome settings (chrome://flags/)"* enabled? We can't do that and break accessibility across all browsers except Chrome with an Experimental flag enabled. I thought this PR was going to integrate the ":focus-visible" polyfill. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5483 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AeI8rr-nlpGGZR_XB8hA6kcmJuBY2_6qks5uioPrgaJpZM4XLBGZ> .

gkatsev · 2018-10-09T15:14:30Z

What about a general one in the main scss file instead for each component?

.video-js *:focus:not(:focus-visible) {
  outline: none;
}

@OwenEdwards older browsers that don't support that pseudo-selector will ignore the entire block.

OwenEdwards · 2018-10-09T20:50:47Z

@gjanblaszczyk and @gkatsev: "older browsers that don't support that pseudo-selector will ignore the entire block." - ahh, thanks for that clarification, I misunderstood that.

Then I agree with @gkatsev's suggestion above, making it generic for all of video.js. Additionally, I'd suggest that we add the :focus-visible polyfill into one of the HTML examples in docs or sandbox, and refer to it in some of the documentation - that will help steer people in that direction if they're concerned about this, rather than them submitting just a univeral outline: none; PR again (like #5027).

This reverts commit 6c44d07.

gjanblaszczyk · 2018-10-10T20:30:20Z

Thanks for comments,
@gkatsev I added focus-visible selector to the main file.
Do we want to add focus-visible polyfill to the sandbox demo? If Yes I suggest adding the new focus-visible.html demo to the sandbox directory. any objections?

gkatsev · 2018-10-10T21:20:36Z

Adding a focus-visible.html.example to the sandbox sounds reasonable to me.

gkatsev · 2018-10-22T21:54:25Z

You can go to this URL directly chrome://flags/#enable-experimental-web-platform-features

gkatsev · 2018-10-22T21:58:31Z

@gjanblaszczyk hey, did you get a chance to add the example page using the polyfill?

gjanblaszczyk · 2018-10-23T14:27:13Z

Hi @gkatsev, not yet but I am planning to do that tomorrow.

gkatsev

Thanks for the update

gkatsev · 2018-10-24T15:33:17Z

sandbox/focus-visible.html.example

+      This will hide the focus indicator if the element receives focus via the mouse,
+      but it will still show up on keyboard focus.
+    */
+    .js-focus-visible :focus:not(.focus-visible) {


is this piece necessary since we have a similar one in Video.js now?

gkatsev · 2018-10-24T15:34:07Z

sandbox/focus-visible.html.example

+<body>
+  <div style="background-color:#eee; border: 1px solid #777; padding: 10px; margin-bottom: 20px; font-size: .8em; line-height: 1.5em; font-family: Verdana, sans-serif;">
+    <p>You can use /sandbox/ for writing and testing your own code. Nothing in /sandbox/ will get checked into the repo, except files that end in .example (so don't edit or add those files). To get started make a copy of index.html.example and rename it to index.html.</p>
+    <pre>cp sandbox/focus-visible.html.example sandbox/focus-visible.html</pre>


we really need to update this text, npm start will automatically copy these files over from .example if they don't already exist.

gkatsev · 2018-10-24T15:34:36Z

sandbox/focus-visible.html.example

+  <video id="vid1" class="video-js vjs-default-skin" controls preload="auto" width="640" height="264"
+      poster="http://vjs.zencdn.net/v/oceans.png"
+      data-setup='{"controlBar": {"volumePanel": {"inline": false}}}'>
+    <!-- <source src="http://wms.shared.streamshow.it/canale8/canale8/playlist.m3u8" type="application/x-mpegURL"> -->


can this line be removed?

gkatsev · 2018-10-24T15:36:39Z

Looks like the package-lock hasn't been updated, can you update it with npm 6.4.1?

gkatsev · 2018-10-24T20:04:33Z

src/css/video-js.scss

@@ -52,3 +52,7 @@
  border: none;
  visibility: hidden;
 }
+
+.video-js *:focus:not(.focus-visible) {


does the polyfill require the usage of the .focus-visible class? We probably want to have both selectors in that case.

Yes, according to the polyfill doc. This class is needed.

Ah, cool, didn't realize. Yeah, let's just do both, then eventually we can remove this one. Thanks!

If you're going to do this (which makes sense because the focus-visible polyfill can only apply a .focus-visible class, not a :focus-visible pseudo-class, I think you need to include the syntax I had in my example:

.js-focus-visible .video-js *:focus:not(.focus-visible) { outline: none; }

Without the .js-focus-visible it's going to remove the focus outline if the developer doesn't include the polyfill, which is exactly what we want to avoid.

For example, see https://deploy-preview-5483--videojs-docs.netlify.com/test-example/index.html and use keyboard-only operation.

^^^ That comment should have been a review

gkatsev · 2018-10-24T20:36:02Z

Looks great, thanks! https://deploy-preview-5483--videojs-docs.netlify.com/test-example/focus-visible.html

gkatsev · 2018-10-24T20:41:51Z

Actually, it's weird but I'm not seeing the outline when using the keyboard which is kind of weird. I'd expect to still see it then.

gjanblaszczyk · 2018-10-24T20:48:21Z

Hmm, I guess https://deploy-preview-5483--videojs-docs.netlify.com/node_modules/focus-visible/dist/focus-visible.min.js polyfill was not deployed.
https://github.com/videojs/video.js/pull/5483/files#diff-9b1584df4b6a33e59cd754007cf0042bR39

gkatsev · 2018-10-24T20:50:47Z

Aah, makes sense. Totally missed that. I guess either swap it out for

<script src="https://unpkg.com/focus-visible"></script>

or edit the build file https://github.com/videojs/video.js/blob/master/build/generate-example.js

The updating the url is probably easiest.

gkatsev · 2018-10-24T20:54:49Z

Loading in the polyfill manually made it work beautifully.

gjanblaszczyk · 2018-10-24T21:01:05Z

Great but maybe I should change the focus-visible polyfill script src to https://unpkg.com/focus-visible on the focus-visible example page and remove focus-visible polyfill from the node package.json file. I guess better to have less dependency. What do you think?

gkatsev · 2018-10-24T21:02:15Z

Sounds good.

gkatsev

🎉

OwenEdwards

See my note on the need for the .js-focus-visible class in the CSS selector.

OwenEdwards · 2018-10-25T01:01:23Z

src/css/video-js.scss

@@ -52,3 +52,7 @@
  border: none;
  visibility: hidden;
 }
+
+.video-js *:focus:not(.focus-visible) {


^^^ That comment should have been a review

gkatsev · 2018-10-25T01:06:04Z

Oh, yeah, I'll add it in when I merge if @gjanblaszczyk doesn't get to it first.

welcome · 2018-10-25T19:23:17Z

Congrats on merging your first pull request! 🎉🎉🎉

gkatsev · 2018-10-25T19:23:24Z

Thanks @gjanblaszczyk!

gjanblaszczyk · 2018-10-26T06:32:31Z

np @gkatsev

Add support for focus-visible so that mouse-users don't need to see focus outlines but keyboard and Screen Reader users still do. It includes both the standard selector and the selector intended to work with the polyfill: https://github.com/WICG/focus-visible. The polyfill is *not* included in Video.js and must be included on the page separately. Fixes #5474.

gjanblaszczyk and others added 5 commits September 3, 2018 11:07

Merge pull request #1 from videojs/master

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

d51fb4c

sync with origin branch

Merge remote-tracking branch 'upstream/master'

fb16dcf

Merge remote-tracking branch 'upstream/master'

34ca2d2

Merge remote-tracking branch 'upstream/master'

4749478

fix: add support for :focus-visible selector

6c44d07

gjanblaszczyk changed the title ~~Prevent focus on click v2~~ Fix: add support for :focus-visible selector Oct 5, 2018

gjanblaszczyk added 2 commits October 10, 2018 22:01

Revert "fix: add support for :focus-visible selector"

542e592

This reverts commit 6c44d07.

fix: add default focus-visible selector to all elements

bf8b648

Merge remote-tracking branch 'upstream/master'

b13ea8b

gkatsev approved these changes Oct 22, 2018

View reviewed changes

Adds focus-visible example

e62d3ac

gkatsev reviewed Oct 24, 2018

View reviewed changes

gjanblaszczyk added 3 commits October 24, 2018 21:23

updates package-lock.json file

76e7460

fixes typo in CSS rule

fdee5f3

adds changes according to the review comments

e117f88

gkatsev reviewed Oct 24, 2018

View reviewed changes

gjanblaszczyk added 3 commits October 24, 2018 22:04

Merge remote-tracking branch 'upstream/master'

7535983

resolves merge conflict

3d99bc0

updates package-lock.json

e0ee616

adds .focus-visible polyfill class

9d22be2

gjanblaszczyk added 3 commits October 24, 2018 23:20

Changes the focus-visible src on the example page

100ac17

removes npm focus-visible package

9703842

reverts package-lock.json file to the latest master version.

bc91e33

gkatsev approved these changes Oct 24, 2018

View reviewed changes

OwenEdwards requested changes Oct 25, 2018

View reviewed changes

Adds missing .js-focus-visible class to the CSS selector.

d0654fe

gkatsev merged commit b8fe624 into videojs:master Oct 25, 2018

gkatsev mentioned this pull request Dec 18, 2018

Fix - Firefox Outline on player #5695

Closed

4 tasks

PoojaPanchal12 mentioned this pull request Aug 31, 2024

[Snyk] Upgrade video.js from 6.12.0 to 6.13.0 CleverTap/vue-video-player#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: add support for :focus-visible selector #5483

Fix: add support for :focus-visible selector #5483

gjanblaszczyk commented Oct 5, 2018

OwenEdwards commented Oct 7, 2018

gjanblaszczyk commented Oct 8, 2018 •

edited

Loading

asd29696 commented Oct 8, 2018 via email

gkatsev commented Oct 9, 2018

OwenEdwards commented Oct 9, 2018

gjanblaszczyk commented Oct 10, 2018

gkatsev commented Oct 10, 2018

gkatsev commented Oct 22, 2018

gkatsev commented Oct 22, 2018

gjanblaszczyk commented Oct 23, 2018

gkatsev left a comment

gkatsev Oct 24, 2018

gkatsev Oct 24, 2018

gkatsev Oct 24, 2018

gkatsev commented Oct 24, 2018

gkatsev Oct 24, 2018

gjanblaszczyk Oct 24, 2018

gkatsev Oct 24, 2018

OwenEdwards Oct 25, 2018 •

edited

Loading

OwenEdwards Oct 25, 2018

gkatsev commented Oct 24, 2018

gkatsev commented Oct 24, 2018

gjanblaszczyk commented Oct 24, 2018 •

edited

Loading

gkatsev commented Oct 24, 2018

gkatsev commented Oct 24, 2018

gjanblaszczyk commented Oct 24, 2018

gkatsev commented Oct 24, 2018

gkatsev left a comment

OwenEdwards left a comment

OwenEdwards Oct 25, 2018

gkatsev commented Oct 25, 2018

welcome bot commented Oct 25, 2018

gkatsev commented Oct 25, 2018

gjanblaszczyk commented Oct 26, 2018

Fix: add support for :focus-visible selector #5483

Fix: add support for :focus-visible selector #5483

Conversation

gjanblaszczyk commented Oct 5, 2018

Description

Requirements Checklist

OwenEdwards commented Oct 7, 2018

gjanblaszczyk commented Oct 8, 2018 • edited Loading

asd29696 commented Oct 8, 2018 via email

gkatsev commented Oct 9, 2018

OwenEdwards commented Oct 9, 2018

gjanblaszczyk commented Oct 10, 2018

gkatsev commented Oct 10, 2018

gkatsev commented Oct 22, 2018

gkatsev commented Oct 22, 2018

gjanblaszczyk commented Oct 23, 2018

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenEdwards Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Oct 24, 2018

gkatsev commented Oct 24, 2018

gjanblaszczyk commented Oct 24, 2018 • edited Loading

gkatsev commented Oct 24, 2018

gkatsev commented Oct 24, 2018

gjanblaszczyk commented Oct 24, 2018

gkatsev commented Oct 24, 2018

gkatsev left a comment

Choose a reason for hiding this comment

OwenEdwards left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Oct 25, 2018

welcome bot commented Oct 25, 2018

gkatsev commented Oct 25, 2018

gjanblaszczyk commented Oct 26, 2018

gjanblaszczyk commented Oct 8, 2018 •

edited

Loading

OwenEdwards Oct 25, 2018 •

edited

Loading

gjanblaszczyk commented Oct 24, 2018 •

edited

Loading