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

Fullscreen button should be removed or greyed out when fullscreen is unavailable #5290

Closed
DoomTay opened this issue Jun 29, 2018 · 2 comments · Fixed by #5296
Closed

Fullscreen button should be removed or greyed out when fullscreen is unavailable #5290

DoomTay opened this issue Jun 29, 2018 · 2 comments · Fixed by #5296

Comments

@DoomTay
Copy link
Contributor

DoomTay commented Jun 29, 2018

Description

See title.

Example use case: an iframe that doesn't have the allowfullscreen attribute. The given starter template is such an example

Results

Expected

Fullscreen button should be removed or greyed out

Actual

Fullscreen button appears functional, but clicking on it does nothing. It takes looking at the console to see that it's because the containing iframe doesn't have the allowfullscreen attribute

Error output

If there are any errors at all, please include them here.

Additional Information

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

versions

videojs

7.0.5

browsers

Firefox 60.0.2

OSes

Windows 10

plugins

None

@gkatsev gkatsev added pinned Things that stalebot shouldn't close automatically and removed pinned Things that stalebot shouldn't close automatically labels Jul 2, 2018
@gkatsev
Copy link
Member

gkatsev commented Jul 3, 2018

Looks like we can check document.fullscreenEnabled and the vendor prefixed versions of that to know whether fullscreen is enabled or not.
Looks like we already get the correct prefix in our fullscreen-api file (sample usage, so, we can just use that either in the fullscreen-toggle file to disable it and grey it out (making sure to apply correct aria labels) or just removing it from the default components. I'm leaning towards disabling it rather than removing the button.

Would you be interested in making the change?

@DoomTay
Copy link
Contributor Author

DoomTay commented Jul 3, 2018

Sure, sounds pretty simple

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants