Control text fullscreen fix on ESC #3485

vdeshpande · 2016-08-01T23:09:57Z

Description

This PR is to make sure the correct control text is displayed on the Fullscreen button after pressing the ESC key while in Fullscreen mode.

Current Status:
(Steps to reproduce)

Load the player on a page.
Press the fullscreen button, the player goes into fullscreen mode.
Hit the ESC key on your keyboard, and the player will go back to normal mode, however the control text on fullscreen button does not change or revert back to text as Fullscreen. It reads as Non-Fullscreen which is not correct.

After this fix:
Follow Step 1 and Step 2 as above. Press the ESC key and the control text should now revert back to the correct text as Fullscreen

Before this fix (current scenario)

After this fix

Specific Changes proposed

The changes made in this fix makes sure that we have the correct control text when you exit fullscreen mode.

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

gkatsev · 2016-08-01T23:18:47Z

src/js/player.js

@@ -1092,6 +1092,8 @@ class Player extends Component {
      this.addClass('vjs-fullscreen');
    } else {
      this.removeClass('vjs-fullscreen');
+      // Making sure the Control text reverts back after pressing ESC
+      this.controlBar.fullscreenToggle.controlText('Fullscreen');


the fullscreen toggle handles the control text changes. We should make that component handle this use case correctly as well.

mister-ben · 2016-08-02T11:09:17Z

Currently the control's text updates even when an attempt to switch to fullscreen is unsuccessful, such as when in an iframe without allowfullscreen - example. It would be good to fix that here too.

gkatsev · 2016-08-02T20:29:11Z

src/js/clickable-component.js

@@ -28,6 +28,7 @@ class ClickableComponent extends Component {
    this.on('click', this.handleClick);
    this.on('focus', this.handleFocus);
    this.on('blur', this.handleBlur);
+    player.on('fullscreenchange', this.handleFullscreenChange);


why not do this only in fullscreen-toggle? Generic clickable-components don't need to know about it.

gkatsev · 2016-08-02T20:31:10Z

There are test failures.

gkatsev · 2016-08-02T20:32:57Z

src/js/control-bar/fullscreen-toggle.js

+    }
+    else {
+      this.removeClass('vjs-fullscreen');
+      this.controlBar.fullscreenToggle.controlText('Fullscreen');


we should make sure the context for this method is the fullscreen toggle itself, otherwise it can be super confusing.

gkatsev · 2016-08-02T22:30:08Z

Also, would be nice to get a test for this, I think it'll just go into the test/unit/controls.js.

OwenEdwards · 2016-08-03T04:40:26Z

src/js/control-bar/fullscreen-toggle.js

+   * @method handleFullscreenChange
+   */
+  handleFullscreenChange() {
+    let fst = this.player().controlBar.fullscreenToggle;


This seems unreliable - making the change that @gkatsev suggests would mean that fst would be entirely replaced in this method by this, which would make FullscreenToggle much more robust and extensible.

misteroneill · 2016-08-08T15:56:54Z

I think this needs to be brought up to date with master because automated linting via videojs-standard has been added there.

vdeshpande · 2016-08-08T18:48:53Z

@misteroneill How do the above changes look ?

gkatsev · 2016-08-08T21:28:56Z

I think something went wrong in the rebase. You have extra commits in this branch now.

# The first commit's message is: Squashing commits and rebasing Making changes to take into consideration fullscreen change on component Making proposed changes Removing test commented code Making the necessary changes and adding a test # The 2nd commit message will be skipped: # Adding the correct style to the test # The 3rd commit message will be skipped: # Removing duplicate instance of same test

Making changes to take into consideration fullscreen change on component Making proposed changes Removing test commented code Making the necessary changes and adding a test

misteroneill · 2016-08-09T19:05:48Z

LGTM

gkatsev · 2016-08-09T19:59:42Z

src/js/control-bar/fullscreen-toggle.js

+  constructor(player, options) {
+    super(player, options);
+    this.on(player, 'fullscreenchange', this.handleFullscreenChange);
+  }


new line after this and before the next comment.

gkatsev · 2016-08-09T20:00:16Z

One super small comment

vdeshpande · 2016-08-09T21:00:58Z

@gkatsev done

misteroneill · 2016-08-10T17:10:35Z

My LGTM still applies 😄

brandonocasey · 2016-08-10T17:17:36Z

LGTM

DerekCalder · 2020-01-07T20:22:51Z

This bug has returned. It is exactly as described in the original Description. I see it happening with version 7.6.6 in Chrome and Firefox. Here is a jsfiddle where it can be reproduced: https://jsfiddle.net/fzkLhs7e/1/. Should I open a new issue for this?

gkatsev · 2020-01-28T23:05:14Z

Generally, best to open new issues for this as commenting on old things will likely get lost. Almost lost this but remembered seeing a comment about it as I'm currently refactoring our fullscreen support.

gkatsev · 2020-01-29T20:28:34Z

I believe it will be fixed as part of #6422.

gkatsev reviewed Aug 1, 2016
View reviewed changes

gkatsev reviewed Aug 2, 2016
View reviewed changes

OwenEdwards reviewed Aug 3, 2016
View reviewed changes

vdeshpande added 9 commits August 9, 2016 14:03

Squashing commits and rebasing

8db1807

Making changes to take into consideration fullscreen change on component Making proposed changes Removing test commented code Making the necessary changes and adding a test

Control text fix fullscreen

85c7872

Making changes to take into consideration fullscreen change on component

aac5e44

Making proposed changes

3bd02d7

Removing test commented code

18974c4

Making the necessary changes and adding a test

8053aab

Adding the correct style to the test

ea10b78

Removing duplicate instance of same test

7afa4a0

vdeshpande force-pushed the fullScreenESCfix branch from 809ad2d to 7afa4a0 Compare August 9, 2016 18:11

gkatsev reviewed Aug 9, 2016
View reviewed changes

Adding a new line as requested

7ea58fe

gkatsev added confirmed patch This PR can be added to a patch release. labels Aug 10, 2016

gkatsev closed this in 2c84f45 Aug 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control text fullscreen fix on ESC #3485

Control text fullscreen fix on ESC #3485

vdeshpande commented Aug 1, 2016

gkatsev Aug 1, 2016

mister-ben commented Aug 2, 2016

gkatsev Aug 2, 2016

gkatsev commented Aug 2, 2016

gkatsev Aug 2, 2016

gkatsev commented Aug 2, 2016

OwenEdwards Aug 3, 2016

misteroneill commented Aug 8, 2016

vdeshpande commented Aug 8, 2016

gkatsev commented Aug 8, 2016

misteroneill commented Aug 9, 2016

gkatsev Aug 9, 2016

gkatsev commented Aug 9, 2016

vdeshpande commented Aug 9, 2016

misteroneill commented Aug 10, 2016

brandonocasey commented Aug 10, 2016

DerekCalder commented Jan 7, 2020

gkatsev commented Jan 28, 2020

gkatsev commented Jan 29, 2020

Control text fullscreen fix on ESC #3485

Control text fullscreen fix on ESC #3485

Conversation

vdeshpande commented Aug 1, 2016

Description

Specific Changes proposed

Requirements Checklist

gkatsev Aug 1, 2016

Choose a reason for hiding this comment

mister-ben commented Aug 2, 2016

gkatsev Aug 2, 2016

Choose a reason for hiding this comment

gkatsev commented Aug 2, 2016

gkatsev Aug 2, 2016

Choose a reason for hiding this comment

gkatsev commented Aug 2, 2016

OwenEdwards Aug 3, 2016

Choose a reason for hiding this comment

misteroneill commented Aug 8, 2016

vdeshpande commented Aug 8, 2016

gkatsev commented Aug 8, 2016

misteroneill commented Aug 9, 2016

gkatsev Aug 9, 2016

Choose a reason for hiding this comment

gkatsev commented Aug 9, 2016

vdeshpande commented Aug 9, 2016

misteroneill commented Aug 10, 2016

brandonocasey commented Aug 10, 2016

DerekCalder commented Jan 7, 2020

gkatsev commented Jan 28, 2020

gkatsev commented Jan 29, 2020