-
Notifications
You must be signed in to change notification settings - Fork 37
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
Show VideoPlayer tooltips when associated control receives focus #872
base: main
Are you sure you want to change the base?
Conversation
|
🟢 No design token changes found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (1)
- packages/react/src/VideoPlayer/VideoPlayer.module.css: Language not supported
Comments suppressed due to low confidence (3)
packages/react/src/VideoPlayer/components/CCButton/CCButton.tsx:21
- The removal of the 'aria-label' attribute might affect accessibility. Ensure that the tooltip alone is sufficient for screen readers or consider retaining the 'aria-label' attribute.
aria-label={ccEnabled ? 'Disable captions' : 'Enable captions'}
packages/react/src/VideoPlayer/components/VideoTooltip/VideoTooltip.tsx:10
- Ensure that the focus and blur event listeners correctly show and hide the tooltip. Add tests if not already covered.
const tooltipRef = useRef<HTMLDivElement>(null)
packages/react/src/VideoPlayer/components/IconControl/IconControl.tsx:12
- Removing the aria-label attribute may affect accessibility. Consider keeping aria-label={tooltip} to ensure screen readers can provide the necessary information.
<button className={clsx(styles.VideoPlayer__iconControl, className)} {...rest}>
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
6f3db51
to
18707d8
Compare
Summary
Previously, the
VideoPlayer
component's tooltips would only show when the associated control was hovered. This PR makes them show on focus as well.Unfortunately we can't just use the
:focus
pseudo-class because we need to be able to dismiss the tooltip with the Esc key.Longer term, we'd benefit from using the PB
Tooltip
component, rather than this custom implementation.This change doesn't show the tooltip when focusing the
Range
sliders (eg seek, volume) as those tooltips are currently bound to mouse position, which isn't relevant when using keyboard controls.List of notable changes:
VideoTooltip
on focus/blurWhat should reviewers focus on?
Steps to test:
VideoPlayer
storiesSupporting resources (related issues, external links, etc):
Contributor checklist:
update snapshots
label to the PR)Reviewer checklist: