LBRYCommunity/lbry-desktop

Fork 0

Fullscreen hotkey fix #5874

Merged

MaxKotlan merged 1 commit from fullscreen-hotkey-fix into master

2021-04-19 19:34:10 +02:00

MaxKotlan commented

2021-04-13 02:39:46 +02:00

(Migrated from github.com)

PR Checklist

Please check all that apply to this PR using "x":

I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
I have checked that this PR does not introduce a breaking change
This PR introduces breaking changes and I have provided a detailed explanation below

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting)
Refactoring (no functional changes)
Documentation changes
Other - Please describe:

Fixes

Issue Number: 5865

What is the current behavior?

CTRL+F is a commonly used hotkey for searching text on a page
LBRY currently has an "F" hotkey that will put the video player in fullscreen mode.
Unfortunately, it will activate fullscreen even if the user is trying to use the CTRL+F shortcut.

What is the new behavior?

This PR makes it so the F hotkey only works if the control key/metakey are not also being pressed.

Other information

Tested it on windows and it works.
Haven't tested on OS X, but I think it should also work, since this also checks for the metakey (⌘ Command / ⊞ Windows )
and the shortcut is Command + F on OS X
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/ctrlKey
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/metaKey

## PR Checklist  Please check all that apply to this PR using "x": - [x] I have checked that this PR is not a duplicate of an existing PR (open, closed or merged) - [x] I have checked that this PR does not introduce a breaking change - [ ] This PR introduces breaking changes and I have provided a detailed explanation below ## PR Type What kind of change does this PR introduce? - [x] Bugfix - [ ] Feature - [ ] Code style update (formatting) - [ ] Refactoring (no functional changes) - [ ] Documentation changes - [ ] Other - Please describe: ## Fixes Issue Number: 5865 ## What is the current behavior? CTRL+F is a commonly used hotkey for searching text on a page LBRY currently has an "F" hotkey that will put the video player in fullscreen mode. Unfortunately, it will activate fullscreen even if the user is trying to use the CTRL+F shortcut. ## What is the new behavior? This PR makes it so the F hotkey only works if the control key/metakey are not also being pressed. ## Other information Tested it on windows and it works. Haven't tested on OS X, but I think it should also work, since this also checks for the metakey (⌘ Command / ⊞ Windows ) and the shortcut is Command + F on OS X [https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/ctrlKey](url) [https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/metaKey](url)

infinite-persistence commented

2021-04-13 11:13:41 +02:00

(Migrated from github.com)

It would be nice if we can clean up the shortcut-handling code for all single-key actions to ignore ctrl, alt, cmd, etc.
- Currently, the issue will still happen with Alt+F, which is the shortcut to invoke Chrome's Menu. That action still causes the video to go fullscreen.

- It would be nice if we can clean up the shortcut-handling code for all single-key actions to ignore `ctrl`, `alt`, `cmd`, etc. - Currently, the issue will still happen with `Alt+F`, which is the shortcut to invoke Chrome's Menu. That action still causes the video to go fullscreen.

MaxKotlan commented

2021-04-13 13:57:29 +02:00

(Migrated from github.com)

Good point. I'll add in the altkey and apply it to all single-key actions.

MaxKotlan commented

2021-04-15 01:26:03 +02:00

(Migrated from github.com)

@infinite-persistence
I broke up the handleKeyDown function into a couple of new smaller functions:
handleShiftKeyActions() and handleSingleKeyActions().
Both handleShiftKeyActions() and handleSingleKeyActions() will exit if either alt, ctrl, or the meta key is pressed.
handleShiftKeyActions() will exit if shift is not being pressed
handleSingleKeyActions() will exit if shift is being pressed.

There is still some weird behavior with the hotkeys, but this behavior exists in the current version of lbry.

F11 hotkey is really buggy even though it uses the same code as the F hotkey. The F hotkey does not have this issue. There seems to be a side effect that's also causing the video to go fullscreen (or something like that).
The < and > hotkeys for video scrubbing require the user to press shift, even though there is nothing specified in the handleKeyDownFunction that would require that. The code for < and > is located in the exact same spot as the j and l hotkeys, and those work just fine without pressing shift. In fact, it's really confusing to me, because the code for all seeking actions is located in the handleSingleKeyActions() function, which disables shift key use, so I'm still not entirely sure how that is working. My guess is it's another side effect, but I'm just not sure. Maybe it's just the default player behavior.

In my testing, everything else was working normally, and the issues listed still exist in current lbry. This PR fixes the ctrl+f issue and it should have fixed any other issue related to other browser hotkeys.

@infinite-persistence I broke up the handleKeyDown function into a couple of new smaller functions: _handleShiftKeyActions()_ and _handleSingleKeyActions()_. Both handleShiftKeyActions() and handleSingleKeyActions() will exit if either alt, ctrl, or the meta key is pressed. handleShiftKeyActions() will exit if shift is **not** being pressed handleSingleKeyActions() will exit if shift is being pressed. There is still some weird behavior with the hotkeys, but this behavior exists in the current version of lbry. 1. F11 hotkey is really buggy even though it uses the same code as the F hotkey. The F hotkey does not have this issue. There seems to be a side effect that's also causing the video to go fullscreen (or something like that). 2. The < and > hotkeys for video scrubbing require the user to press shift, even though there is nothing specified in the handleKeyDownFunction that would require that. The code for < and > is located in the exact same spot as the j and l hotkeys, and those work just fine without pressing shift. In fact, it's really confusing to me, because the code for all seeking actions is located in the handleSingleKeyActions() function, which disables shift key use, so I'm still not entirely sure how that is working. My guess is it's another side effect, but I'm just not sure. Maybe it's just the default player behavior. In my testing, everything else was working normally, and the issues listed still exist in current lbry. This PR fixes the ctrl+f issue and it should have fixed any other issue related to other browser hotkeys.

👍 1

MaxKotlan commented

2021-04-15 02:02:34 +02:00

(Migrated from github.com)

Actually never-mind on the 2nd issue. I was getting the speedup keys confused with the scrubbing keys (left arrow and right arrow). Everything should be fine.
Although I do wonder if there's a specific reason why the < and > hotkeys require the user to hold shift, while the rest of the hot keys don't. Is there a specific reason why the < and > hotkeys require the user to press shift?

Actually never-mind on the 2nd issue. I was getting the speedup keys confused with the scrubbing keys (left arrow and right arrow). Everything should be fine. Although I do wonder if there's a specific reason why the < and > hotkeys require the user to hold shift, while the rest of the hot keys don't. Is there a specific reason why the < and > hotkeys require the user to press shift?

infinite-persistence commented

2021-04-15 13:53:24 +02:00

(Migrated from github.com)

F11 hotkey is really buggy even though it uses the same code as the F hotkey. The F hotkey does not have this issue. There seems to be a side effect that's also causing the video to go fullscreen (or something like that).

Yeah, F11 should be for "browser full screen" instead of "player full screen". Seems like it was added to address #2514 Can't exit full-screen from embedded content with key F11 , which looks like is already fixed in the version of Electron that we are using. I think we can remove F11.

> _F11 hotkey is really buggy even though it uses the same code as the F hotkey. The F hotkey does not have this issue. There seems to be a side effect that's also causing the video to go fullscreen (or something like that)._ Yeah, F11 should be for "browser full screen" instead of "player full screen". Seems like it was added to address #2514 [Can't exit full-screen from embedded content with key F11 ](https://github.com/lbryio/lbry-desktop/issues/2514), which looks like is already fixed in the version of Electron that we are using. I think we can remove F11.

MaxKotlan commented

2021-04-16 03:17:17 +02:00

(Migrated from github.com)

After looking into the fullscreen issue a bit more... I realized that it was actually freaking out because the fullscreen of the video player was interfering with the fullscreen hotkey in chrome.
I added e.preventDefault(), which prevents chrome's fullscreen hotkey, but the video player handles it, so it's enabled and disabled like normal.
Now the F hotkey and F11 work the same.

I also tested in both the web client and electrum and it worked in both.

After looking into the fullscreen issue a bit more... I realized that it was actually freaking out because the fullscreen of the video player was interfering with the fullscreen hotkey in chrome. I added e.preventDefault(), which prevents chrome's fullscreen hotkey, but the video player handles it, so it's enabled and disabled like normal. Now the F hotkey and F11 work the same. I also tested in both the web client and electrum and it worked in both.

MaxKotlan commented

2021-04-16 05:08:00 +02:00

(Migrated from github.com)

Sorry, I realized that what I was doing was a bad idea. Removed the f11 hotkey instead like you said. F11 makes the browser go fullscreen, and f makes the video go fullscreen.

Is there anything else I need to do for this PR? Not sure what the contributing process is for this project.

Sorry, I realized that what I was doing was a bad idea. Removed the f11 hotkey instead like you said. F11 makes the browser go fullscreen, and f makes the video go fullscreen. Is there anything else I need to do for this PR? Not sure what the contributing process is for this project.

neb-b commented

2021-04-16 17:23:13 +02:00

(Migrated from github.com)

@MaxKotlan This is great! Can you just squash your commits? Then it's good to merge.

MaxKotlan commented

2021-04-16 21:08:40 +02:00

(Migrated from github.com)

@seanyesmunt Commits are now squashed!