New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: add background:none to the :focus-visible selector #5558

Merged
merged 14 commits into from Nov 30, 2018

Conversation

Projects
None yet
3 participants
@gjanblaszczyk
Copy link
Contributor

gjanblaszczyk commented Nov 7, 2018

Description

A proposed fix for #5557
Please watch below video to see how the captions menu will work after proposed changes.
videojsmenuclickafterfix

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
@gjanblaszczyk

This comment has been minimized.

Copy link
Contributor Author

gjanblaszczyk commented Nov 7, 2018

Weird that Travis CI failed but I guess because of partially degraded service.

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Nov 7, 2018

Yeah, I'm not sure what's happening with tests. I updated deps this week which seems to help but I'm not sure.

@gkatsev
Copy link
Member

gkatsev left a comment

Setting background:none makes sense as this is similar to the outline thing.

@@ -43,13 +43,13 @@
}

.vjs-menu li.vjs-menu-item:focus,
.vjs-menu li.vjs-menu-item:hover {
.vjs-menu .vjs-menu-content li.vjs-menu-item:hover {

This comment has been minimized.

@gkatsev

gkatsev Nov 7, 2018

Member

why increase the specificity?

This comment has been minimized.

@gjanblaszczyk

gjanblaszczyk Nov 7, 2018

Author Contributor

The CSS ".js-focus-visible .video-js *:focus:not(.focus-visible)" selector has higher priority than ".vjs-menu li.vjs-menu-item:hover". So if the first menu item gets focus, it doesn't change the background color on hover. That's why I increased the specificity to the hover CSS selector to resolve above problem.

This comment has been minimized.

@gkatsev

gkatsev Nov 13, 2018

Member

Ah, hm... I'd be worried that increasing this specificity would break some custom skins

This comment has been minimized.

@gjanblaszczyk

gjanblaszczyk Nov 19, 2018

Author Contributor

@gkatsev Does it look better now?

@gkatsev gkatsev requested a review from OwenEdwards Nov 7, 2018

@OwenEdwards
Copy link
Member

OwenEdwards left a comment

I don't see any problem with this.

@gkatsev
Copy link
Member

gkatsev left a comment

Thanks! That seems safer.

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Nov 29, 2018

@gjanblaszczyk can you rebase/merge in master? We have a fix for the test failures.

@gjanblaszczyk

This comment has been minimized.

Copy link
Contributor Author

gjanblaszczyk commented Nov 30, 2018

@gkatsev Ok done and now all tests have passed.

@gkatsev gkatsev merged commit e5e1e29 into videojs:master Nov 30, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/videojs-docs/deploy-preview Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment