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.

Sign up for GitHub

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

Jump to bottom

videojs default skin with vertical volumeslider causes viewport overflow in rtl layout #7743

Open
hartman opened this issue May 2, 2022 · 7 comments · May be fixed by #8320
Open

videojs default skin with vertical volumeslider causes viewport overflow in rtl layout #7743

hartman opened this issue May 2, 2022 · 7 comments · May be fixed by #8320
Labels
bug good first issue i18n Relation to localisation and translation.

Comments

@hartman
Copy link
Contributor

hartman commented May 2, 2022

Description

The viewport overflows because of the use of left:-3000em within a rtl html layout

Steps to reproduce

Explain in detail the exact steps necessary to reproduce the issue.
Using the default skin. Set:

	controlBar: {
		volumePanel: {
			vertical: true,
			inline: false
		}
	}

Open a videojs player within a <html dir="rtl">-layout

Results

Expected

The viewport should not horizontally overflow

Actual

The viewport overflows on the left to -3000 em, as the overflow of viewport in rtl is always on the left.

This is a common problem with trying to push text offscreen. See also:

Screenshot 2022-05-02 at 20 58 55

A quick way around this is using top:-negative (though might still have performance problems) or to clip the surface to invisible.

Additional Information

Please include any additional information necessary here. Including the following:

versions

videojs

what version of videojs does this occur with?

browsers

what browser are affected?

OSes

what platforms (operating systems and devices) are affected?

plugins

are any videojs plugins being used on the page? If so, please list them below.
@hartman
Copy link
Contributor Author

hartman commented May 3, 2022

I'm kinda wondering why that -3000 is there anyway... Its clearly to make animations work, but I can't find a situation where this does something that the opacity doesn't already handle. I'm probably overlooking the real reason it is this value, but for now on en.wp I'm going to simply set it to -3.5em; which it is also when visible.

@hartman hartman changed the title videojs default skin with vertical volumeslider overflows viewport in rtl layout videojs default skin with vertical volumeslider causes viewport overflow in rtl layout May 3, 2022
@gkatsev
Copy link
Member

gkatsev commented May 3, 2022 •
edited
Loading

I believe it's moved away because otherwise, you could still interact with the control even when it wasn't visible, which we didn't want. There is probably a way to fix this with any writing direction layout.
I guess the simplest is to add something like:

html[dir=rtl] .video-js .vjs-volume-panel .vjs-volume-control.vjs-volume-vertical {
  right: -3000em;
}

Though, ideally, we'd find something that works well for both.

@misteroneill misteroneill added bug good first issue i18n Relation to localisation and translation. labels May 16, 2022
@lalitkumawat1m
Copy link

I want to work on this issue could you please assign it to me

@mister-ben
Copy link
Contributor

Feel free, thanks for contributing.

@evoxf1
Copy link

evoxf1 commented Jun 13, 2023

is anyone working on this issue

@evoxf1
Copy link

evoxf1 commented Jun 13, 2023

all good first issues are taken smh :(

amtins added a commit to amtins/video.js that referenced this issue Jun 13, 2023
@amtins
fix(volume): vertical slider causes viewport overflow in rtl layout
3054a2f 
Fixes videojs#7743
amtins added a commit to amtins/video.js that referenced this issue Jun 13, 2023
@amtins
fix(volume): vertical slider causes viewport overflow in rtl layout
5d4b28c 
Fixes videojs#7743
@amtins amtins linked a pull request Jun 13, 2023 that will close this issue
Open
7 tasks
@amtins
Copy link
Contributor

amtins commented Jun 13, 2023

@evoxf1 I've just created a draft PR, I still have to test it before I can submit it or see if there isn't a better approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug good first issue i18n Relation to localisation and translation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(volume): vertical slider causes viewport overflow in rtl layout amtins/video.js
7 participants
@hartman @misteroneill @gkatsev @mister-ben @amtins @lalitkumawat1m @evoxf1