Skip to content
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

fix: don't reset background styles of selected MenuItem when focused #7202

Merged
merged 1 commit into from
May 11, 2021

Conversation

acmertz
Copy link
Contributor

@acmertz acmertz commented Apr 22, 2021

Fixes #7200

Description

Fixes a styling issue with the MenuItem component that caused it to display dark text over a dark background when focused, if it was the currently selected MenuItem (.vjs-selected).

Specific Changes proposed

Use :not(.vjs-selected) to prevent the menu styles from resetting the background of a selected MenuItem on focus.

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 (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

Adds a :not() pseudo class to ensure that the MenuItem background color doesn't get incorrectly reset on mouse focus.
Fixes videojs#7200
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The easiest way I've found to verify this is to add vjs-hover class to a menu button (like subs caps button or the playback rate button), then, find the selected item under the <vjs-menu> element, and force the :focus state on it. Without this change, you end up no longer being able to read it the text. With this, you can continue seeing the item selected and readable.

Thanks @acmertz!

@gkatsev gkatsev added the needs: LGTM Needs one or more additional approvals label May 10, 2021
Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 Nice one! Thanks for the pull request.

@gkatsev gkatsev merged commit 06cdb6f into videojs:main May 11, 2021
@misteroneill misteroneill removed the needs: LGTM Needs one or more additional approvals label May 23, 2023
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
Adds a :not() pseudo class to ensure that the MenuItem background color doesn't get incorrectly reset on mouse focus.

Fixes videojs#7200
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selected MenuItem loses background when focused and becomes unreadable
4 participants