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

feat: Implement BrowserWindow.setFocusable on macOS #19033

Merged
merged 1 commit into from Jul 8, 2019

Conversation

@CapOM
Copy link
Contributor

CapOM commented Jun 28, 2019

Description of Change

It was not implemented on Mac despite being available as a constructor
option. Implementation already exists on Windows. Linux case can be
separately.

#19032

Checklist

Release Notes

Notes: Notes: Implemented BrowserWindow.setFocusable on macOS.

@CapOM

This comment has been minimized.

Copy link
Contributor Author

CapOM commented Jun 28, 2019

Hi @codebytere , please take a look, thx!

@codebytere codebytere requested a review from erickzhao Jun 28, 2019
Copy link
Member

erickzhao left a comment

Fix seems to be working for the most part. Small comment regarding focus behavior with this implementation.

shell/browser/native_window_mac.mm Show resolved Hide resolved
@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 29, 2019
@CapOM CapOM force-pushed the CapOM:setFocusable_mac branch 2 times, most recently from 0b5cd43 to fd13983 Jul 1, 2019
@CapOM

This comment has been minimized.

Copy link
Contributor Author

CapOM commented Jul 2, 2019

Hi, all remarks are addressed, please take a look, thx!

@codebytere codebytere requested a review from erickzhao Jul 2, 2019
Copy link
Member

erickzhao left a comment

Just tested manually on Electron Fiddle and it seems to only work when you have multiple windows open. It's working properly if you use the focusable: false constructor option instead of calling w.setFocusable(false). Could you possibly investigate?

screencast 2019-07-02 15-04-18

Fiddle link: https://gist.github.com/c977efc714ed09c0c7087d8222b938b3

shell/browser/native_window_mac.mm Outdated Show resolved Hide resolved
It was not implemented on Mac despite being available as a constructor
option. Implementation already exists on Windows. Linux case can be
separately.

#19032

Notes: Implemented BrowserWindow.setFocusable on macOS.
@CapOM CapOM force-pushed the CapOM:setFocusable_mac branch from fd13983 to 41c245c Jul 3, 2019
@zcbenz
zcbenz approved these changes Jul 8, 2019
@zcbenz zcbenz merged commit 09c3277 into electron:master Jul 8, 2019
8 of 9 checks passed
8 of 9 checks passed
build-mac Workflow: build-mac
Details
Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jul 8, 2019

Release Notes Persisted

Notes: Implemented BrowserWindow.setFocusable on macOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.