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

Mac based browser shortcut fix #4348

Merged
merged 12 commits into from
Dec 25, 2023
Merged

Mac based browser shortcut fix #4348

merged 12 commits into from
Dec 25, 2023

Conversation

gcottom
Copy link

@gcottom gcottom commented Oct 29, 2023

Description:

previously when in the browser on a mac you would have to use the ctrl key to perform keyboard shortcuts which is not intuitive on a mac where cmd is typically used instead of ctrl. this change adds a check in window initialization that will check if the build is browser based (based on build constraints). At runtime there is a javascript check of the navigator.platform value. if this value contains mac, the modifier key handles default shortcut keys with cmd key instead of ctrl key. other operating systems will see no change. Note that this does not change custom defined shortcuts. If you define a shortcut as "ctrl+f" it will be interpreted as ctrl+f meaning that even on a mac you will have to use the ctrl key. This would be in line with the expectation that if you were to run a browser based terminal, that ctrl+c wouldn't copy text but would kill the process.

Fixes #(#4301)
This is part 2 of a 2 part fix. Part 1 is in fyne/glfw-js Pull Request 13. Part 1 addresses the clipboard issue in the browser #4301 . Part 2 which is this PR, addresses keyboard shortcuts not working correctly in Mac browsers which was discovered during the testing of the fix created in part 1.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@andydotxyz this is the solution I proposed in glfw-js Pull Request 14. I will now close that pull request.

Thank you.

andydotxyz and others added 6 commits September 4, 2023 15:53
…. previously when in the browser on a mac you would have to use the ctrl key to perform keyboard shortcuts which is not intuitive on a mac where cmd is typically used instead of ctrl. this change adds a check in window initialization that will check if the build is browser based (based on build constraints). At runtime there is a javascript check of the navigator.platform value. if this value contains mac, the modifier key handles default shortcut keys with cmd key instead of ctrl key. other operating systems will see no change.
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Thanks so much.
I made a suggestion theat keeps the word "browser" out of the general code but also fixes issue with macOS Server and windows client.

internal/driver/glfw/window.go Outdated Show resolved Hide resolved
internal/driver/glfw/web_shortcuts_js.go Outdated Show resolved Hide resolved
internal/driver/glfw/web_shortcuts_wasm.go Outdated Show resolved Hide resolved
…he code. simplify return logic for mac check.
@coveralls
Copy link

coveralls commented Oct 30, 2023

Coverage Status

coverage: 65.099% (+0.006%) from 65.093%
when pulling 6b611d7 on gcottom:browser-shortcut-fix-macos
into 802f92b on fyne-io:develop.

@dweymouth
Copy link
Contributor

There are other places we assign shortcuts dynamically based on runtime.GOOS. widget/entry.go being one (only one I know of without digging around). It would be great to get this improvement there as well!

@andydotxyz
Copy link
Member

True, but I think that's a bigger fix as this is internal to the GLFW driver code for the initial fix?

@gcottom
Copy link
Author

gcottom commented Nov 17, 2023

From what I can see the only place that shortcuts are set outside of what I've changed here is in widget/entry.go. But to add this the shortcut fix would have to live outside of the glfw driver. Any suggestions on how to move forward?

@dweymouth
Copy link
Contributor

I think we will just move forward with this for now as the fix in widget/entry.go would require a new API, which probably should be public, and would wait until 2.5

@andydotxyz
Copy link
Member

I think we will just move forward with this for now as the fix in widget/entry.go would require a new API, which probably should be public, and would wait until 2.5

That works for me. Can you please rebase this on the develop branch @gcottom as that is where we do our development work, then this could be merged.

@andydotxyz
Copy link
Member

The other half of this is now landed, so you can update the version this depends on when rebasing and it should be fully testable.

…. previously when in the browser on a mac you would have to use the ctrl key to perform keyboard shortcuts which is not intuitive on a mac where cmd is typically used instead of ctrl. this change adds a check in window initialization that will check if the build is browser based (based on build constraints). At runtime there is a javascript check of the navigator.platform value. if this value contains mac, the modifier key handles default shortcut keys with cmd key instead of ctrl key. other operating systems will see no change.
…he code. simplify return logic for mac check.
@gcottom gcottom changed the base branch from master to develop November 17, 2023 20:42
@gcottom
Copy link
Author

gcottom commented Nov 17, 2023

@andydotxyz rebased and updated dependency and set to develop branch.

@lorypelli
Copy link

I'm on windows and ALL shortcuts are broken for me

@andydotxyz
Copy link
Member

I'm on windows and ALL shortcuts are broken for me

You are commenting on a pull request here. Do you mean that with this change it is broken, or it was broken for you before and after?

@lorypelli
Copy link

I'm on windows and ALL shortcuts are broken for me

You are commenting on a pull request here. Do you mean that with this change it is broken, or it was broken for you before and after?

After the new update, context menu is working, but still all shortcuts are broken...

@Jacalz
Copy link
Member

Jacalz commented Dec 24, 2023

This is certainly not the place to report a bug. This is a pull request for fixing shortcuts when running Fyne apps in a browser on macOS. If something isn’t working then please open a bug report instead…

@andydotxyz
Copy link
Member

After the new update

You tested this PR and it fixed popup menus? That doesn't seem likely.

@lorypelli
Copy link

After the new update

You tested this PR and it fixed popup menus? That doesn't seem likely.

Is this the correct PR for my issue, is this related to shortcuts like CTRL + C or CTRL + V or F12 not working?

@andydotxyz
Copy link
Member

This should fix all copy-paste shortcuts. When you appply the patch it contains

@andydotxyz andydotxyz merged commit d0b6d2d into fyne-io:develop Dec 25, 2023
10 of 11 checks passed
@Jacalz Jacalz mentioned this pull request Feb 16, 2024
2 tasks
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.

6 participants