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(volume): vertical slider causes viewport overflow in rtl layout #8320

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amtins
Copy link
Contributor

@amtins amtins commented Jun 13, 2023

Description

This PR fixes #7743 where a scroll bar appears when the text direction is set to RTL.

Screencast.from.13.06.23.19.02.52.webm

Specific Changes proposed

  • .video-js .vjs-volume-vertical
    • remove the initial width/height so that it falls back to .video-js .vjs-volume-panel .vjs-volume-control
    • add pointer-events set to none so that the slider does not appear when the mouse is over the volume button
  • resets the width, height and pointer-events when the mouse hovers over the volume button

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

@amtins
Copy link
Contributor Author

amtins commented Jun 25, 2023

Observation

The current behavior allows users to hover over the volume slider when it's disappearing to make it reappear.

actual-behavior.webm

With this fix, the behavior doesn't allow users to hover over the volume slider when it's disappearing to make it reappear, which is a regression.

behavior-with-fix.webm

To test the fix open the preview page, then in the browser's developer console paste document.body.style.direction = 'rtl'

@amtins
Copy link
Contributor Author

amtins commented Jun 25, 2023

Observation

This is outside the scope of this fix. On touchscreen devices, a long press on the volume button displays the volume slider. Usually, on this type of device, a user would tend to use the device's physical buttons to adjust the volume.

&:active .vjs-volume-control,
&:focus .vjs-volume-control,
& .vjs-volume-control:active,

@amtins
Copy link
Contributor Author

amtins commented Jul 2, 2023

I'm putting this PR up for review to get opinions and maybe find a better approach that doesn't introduce regression. Please see comment #8320 (comment)

@amtins amtins marked this pull request as ready for review July 2, 2023 12:30
@amtins amtins added the needs: discussion Needs a broader discussion label Jul 2, 2023
@misteroneill
Copy link
Member

Observation

This is outside the scope of this fix. On touchscreen devices, a long press on the volume button displays the volume slider. Usually, on this type of device, a user would tend to use the device's physical buttons to adjust the volume.

&:active .vjs-volume-control,
&:focus .vjs-volume-control,
& .vjs-volume-control:active,

Yes, my expectation would be that on a phone/tablet the user would use the physical buttons for volume instead of the web UI.

@amtins
Copy link
Contributor Author

amtins commented Jul 7, 2023

Yes, my expectation would be that on a phone/tablet the user would use the physical buttons for volume instead of the web UI.

@misteroneill thank you for the feedback. I'll try to create a PR to solve this problem this week-end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: discussion Needs a broader discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

videojs default skin with vertical volumeslider causes viewport overflow in rtl layout
2 participants