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

add shortcut for fullscreen in live view #12924

Merged
merged 5 commits into from
Aug 10, 2024
Merged

Conversation

stavros-k
Copy link
Contributor

Hello, this PR adds keyboard shortcut f for full screen toggling in LiveView, and a fullscreen query param.
I can now with this set my kiosk headless raspberry to start with ${URL}/?group=my_group&fullscreen=true

I know that changes made here might not align with your codebase standards, which I give you permission to take this PR over and modify it accordingly.

Thanks for the great software!

Copy link

netlify bot commented Aug 10, 2024

Deploy Preview for frigate-docs canceled.

Name Link
🔨 Latest commit fe991c1
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/66b791f12ad0ce00084cacc6

@stavros-k stavros-k changed the title add shortcut and query for live view add shortcut and query for fullscreen in live view Aug 10, 2024
Comment on lines 273 to 277
const desiredState = value === "true";
if (fullscreen === desiredState) {
return;
}
toggleFullscreen;
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const desiredState = value === "true";
if (fullscreen === desiredState) {
return;
}
toggleFullscreen;
if (!fullscreen) {
toggleFullscreen();
}

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not showing in full screen in the default state so I think it is safe to assume that if the query arg is provided the user wants fullscreen

also, toggleFullscreen needs to be called not just passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check that the value === "true"? Assuming someone can programmatically open the URL with it being either true or false?
so in case of fullscreen=false we just ignore it

web/src/views/live/LiveDashboardView.tsx Outdated Show resolved Hide resolved
web/src/views/live/LiveDashboardView.tsx Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>
@hawkeye217
Copy link
Collaborator

Thanks for your contribution!

The Fullscreen API requires that the request to enter fullscreen mode must be initiated by a user gesture (a click, tap, or keypress). This is a security measure so that malicious websites can't unexpectedly take over a user's entire screen without their consent.

https://developer.mozilla.org/en-US/docs/Web/API/Fullscreen_API

So I think the code to initiate fullscreen from a url argument should be removed from this PR.

To confirm, I tested it on a Mac with Firefox, Safari, and Chrome, and each of them failed to go fullscreen with the url argument, each of them presenting a console error similar to:

use-fullscreen.ts:36 Failed to execute 'requestFullscreen' on 'Element': API can only be initiated by a user gesture.

Perhaps the browser you're using doesn't have a strict check, and if so, your browser is likely an exception. This isn't something we'd be able to support for the general user.

But let's definitely move forward with the keypress. That's a great addition.

Copy link
Collaborator

@hawkeye217 hawkeye217 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove url arg code per my earlier comment

@stavros-k
Copy link
Contributor Author

Thanks for your contribution!

The Fullscreen API requires that the request to enter fullscreen mode must be initiated by a user gesture (a click, tap, or keypress). This is a security measure so that malicious websites can't unexpectedly take over a user's entire screen without their consent.

https://developer.mozilla.org/en-US/docs/Web/API/Fullscreen_API

So I think the code to initiate fullscreen from a url argument should be removed from this PR.

To confirm, I tested it on a Mac with Firefox, Safari, and Chrome, and each of them failed to go fullscreen with the url argument, each of them presenting a console error similar to:


use-fullscreen.ts:36 Failed to execute 'requestFullscreen' on 'Element': API can only be initiated by a user gesture.

Perhaps the browser you're using doesn't have a strict check, and if so, your browser is likely an exception. This isn't something we'd be able to support for the general user.

But let's definitely move forward with the keypress. That's a great addition.

Hmm I remember such a restriction now, and I was amazed when it worked initially. I do use it in kiosk mode, so it might have something like to do with it. But at least now I can script a key press to automatically move it to full screen. Thanks!

@NickM-27 NickM-27 merged commit 9b96211 into blakeblackshear:dev Aug 10, 2024
10 checks passed
@stavros-k stavros-k changed the title add shortcut and query for fullscreen in live view add shortcut in live view Aug 10, 2024
@stavros-k stavros-k changed the title add shortcut in live view add shortcut for fullscreen in live view Aug 10, 2024
sandnabba pushed a commit to sandnabba/frigate that referenced this pull request Aug 14, 2024
…2924)

* add shortcut and query for live view

* Update web/src/views/live/LiveDashboardView.tsx

* Update web/src/views/live/LiveDashboardView.tsx

Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>

* Apply suggestions from code review

Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>

* Update LiveDashboardView.tsx

---------

Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>
@hawkeye217
Copy link
Collaborator

@stavros-k FYI, we had to revert your changes in this PR. Users couldn't create or edit camera groups with "f" in the name.

@immkg
Copy link

immkg commented Aug 18, 2024

@stavros-k FYI, we had to revert your changes in this PR. Users couldn't create or edit camera groups with "f" in the name.

You can update use-keyboard-listener.ts with following conditions:

e.target.tagName !== "INPUT" unless modifier is set at ctrl: e.ctrlKey || e.metaKey

This would allow user to type in input fields without shortcuts effecting them. Also it would allow users to make use of shortcuts with modifiers.

@stavros-k
Copy link
Contributor Author

stavros-k commented Aug 20, 2024

@stavros-k FYI, we had to revert your changes in this PR. Users couldn't create or edit camera groups with "f" in the name.

You can update use-keyboard-listener.ts with following conditions:

e.target.tagName !== "INPUT" unless modifier is set at ctrl: e.ctrlKey || e.metaKey

This would allow user to type in input fields without shortcuts effecting them. Also it would allow users to make use of shortcuts with modifiers.

I would appreciate if you could do that change. I don't have the time currently to look into that.
Thanks!

NickM-27 added a commit that referenced this pull request Aug 30, 2024
* add shortcut and query for live view

* Update web/src/views/live/LiveDashboardView.tsx

* Update web/src/views/live/LiveDashboardView.tsx

Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>

* Apply suggestions from code review

Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>

* Update LiveDashboardView.tsx

---------

Co-authored-by: Nicolas Mowen <nickmowen213@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants