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

Jump to bottom

fix: pointer lock escape handling #32369

Merged
merged 3 commits into from Feb 9, 2022
Merged

fix: pointer lock escape handling #32369

merged 3 commits into from Feb 9, 2022

Conversation

codebytere
Copy link
Member

Description of Change

Fixes an issue where the Pointer Lock API could be properly engaged but not disengaged. We were not properly routing requests through the ExclusiveAccessManager nor handling special override keys for exclusive access in WebContents::PreHandleKeyboardEvent. As of this change, this demo works identically to Chrome.

cc @nornagon @jkleinsc

Checklist

Release Notes

Notes: Fixed an issue where Pointer Lock behavior could not be properly exited.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 6, 2022
@codebytere codebytere added target/16-x-y and removed new-pr 🌱 PR opened in the last 24 hours labels Jan 6, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 6, 2022
@codebytere codebytere added semver/patch backwards-compatible bug fixes and removed new-pr 🌱 PR opened in the last 24 hours labels Jan 6, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 6, 2022
@codebytere codebytere force-pushed the match-chromium-pointer-lock branch 2 times, most recently from d5e63f4 to 19387ca Compare January 6, 2022 16:42
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 7, 2022
jkleinsc
jkleinsc requested changes Jan 10, 2022
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

I agree with @nornagon -- we should maintain the call to WebContentsPermissionHelper::RequestPermission as it provides a way for Electron devs to manage pointer lock permissioning.

@codebytere
Copy link
Member Author

codebytere commented Jan 11, 2022 •
edited

@jkleinsc @nornagon i refactored this to split the difference - it now still allows for control via session.setPermissionRequestHandler, and then instead of calling directly into web_contents->GotResponseToLockMouseRequest on response, it now calls into the exclusive access controls so state can be properly tracked and handled.

Chromium itself never asks for explicit permissions on this front - instead it decides based on the following comment whether to grant permission. While this new behavior still ultimately diverges from Chrome's own, i think in light of maintaining backwards compat as well as for improved permissions control this hybrid approach is likely our best bet for serving user and dev needs.

nornagon
nornagon approved these changes Feb 4, 2022
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

Yeah, I guess so. I rather expect that in a few months we'll get a bug report like "no way to prevent Esc from exiting pointer lock", but I suppose we can address that when it comes up.

jkleinsc
jkleinsc approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

LGTM

@codebytere
Copy link
Member Author

Merging as failure is unrelated and a CircleCI infra issue.

@codebytere codebytere merged commit ac1d426 into main Feb 9, 2022
@codebytere codebytere deleted the match-chromium-pointer-lock branch February 9, 2022 09:40
@release-clerk
Copy link

release-clerk bot commented Feb 9, 2022

Release Notes Persisted

Fixed an issue where Pointer Lock behavior could not be properly exited.

This was referenced Feb 9, 2022
Merged
Closed
Merged
@trop
Copy link
Contributor

trop bot commented Feb 9, 2022

I have automatically backported this PR to "16-x-y", please check out #32826

@trop
Copy link
Contributor

trop bot commented Feb 9, 2022

I have automatically backported this PR to "17-x-y", please check out #32827

@trop
Copy link
Contributor

trop bot commented Feb 9, 2022

I have automatically backported this PR to "18-x-y", please check out #32828

nealmanaktola pushed a commit to discord/electron that referenced this pull request Feb 10, 2022
@codebytere @nealmanaktola
fix: pointer lock escape handling (electron#32369)
e05a54a 
# Conflicts:
#	shell/browser/api/electron_api_web_contents.cc
#	shell/browser/api/electron_api_web_contents.h
@zcbenz
Copy link
Member

zcbenz commented Feb 14, 2022

@codebytere This PR makes the " tag requestFullscreen from webview pressing ESC should emit the leave-html-full-screen event" test fail on mac arm64 CI. I think it is not a flaky since this PR has code handling ESC key.

I'm not sure why the test passes on x64 CI though, it is probably related to macOS version or SDK version.

@commit-lint commit-lint bot mentioned this pull request Apr 6, 2022
Open
@nornagon nornagon mentioned this pull request Apr 11, 2022
Closed
3 tasks
@trop trop bot removed the in-flight/17-x-y label Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr left review comments

@nornagon nornagon nornagon approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@codebytere @zcbenz @ckerr @nornagon @jkleinsc @deepak1556