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: support use selection for find on macOS #39629

Closed
wants to merge 2 commits into from

Conversation

anyerqi
Copy link
Contributor

@anyerqi anyerqi commented Aug 23, 2023

Description of Change

Chromium has supported Use Selection for Find on macOS, which is commonly-available feature across macOS apps
The changes are:

  • add a menu role useSelectionForFind with Command-E as the standard keyboard shortcut
  • add a default Edit > Use Selection for Find menu on macOS
  • useSelectionForFind maps to copyToFindPboard action

Fixes #29635

Checklist

Release Notes

notes: Added a menu role on macOS: useSelectionForFind

mhli and others added 2 commits August 23, 2023 18:48
MacOS Only
add a MenuItem role: `useSelectionForFind`, map to `copyToFindPboard`
use the default accelerator in MacOS: Command+E
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 23, 2023
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

Can you talk a little about what use cases you're hoping to enable here? I understand Chromium makes use of it, but on Chromium for example it is part of a greater set of find-related menu items, none of which we support as bespoke menu item roles:

Screenshot 2023-08-24 at 10 40 56 AM

Given we've previously merged some Chromium-specific menu roles, I think we're open to this, but would we want to support any other commands in that menu?

@anyerqi
Copy link
Contributor Author

anyerqi commented Aug 24, 2023

Use Selection for Find is a common function in apps on MacOS, such as Mail, Safari. In addition, Electron-based VSCode also implements this function by itself. So we can speculate that there must be many other apps that also need this feature. This function is not only effective within the app, it will be passed across apps, you can use it in A app, and then use Find Next in B app. Although there is already an API that allows developers to implement similar function themselves, I think it is more convenient to provide it directly.

For the other commands about Finding, I found that implementing these features in chromium would require a lot of extra work and possibly UI components, but pre-defining the UI in ELectron is not appropriate. In addition, as to which content can be searched and selected, different apps may be different, and there are no uniform rules for Finding

@zcbenz zcbenz added semver/minor backwards-compatible functionality no-backport labels Aug 25, 2023
@electron-cation electron-cation bot added api-review/requested 🗳 and removed new-pr 🌱 PR opened in the last 24 hours labels Aug 25, 2023
@itsananderson
Copy link
Contributor

The clipboard module already has APIs to read and write to the Find Pasteboard, so I think individual apps could use those APIs to implement their own version of this behavior.

In Chrome, the Use Selection for Find menu item actually opens UI widget to begin a search, whereas I think what you have here would only copy the current selection to the Find Pasteboard, which makes the Use Selection for Find menu name seem misleading. Please correct me if I'm misinterpreting what this new menu item does.

If we were to add any new API surface here, perhaps we could add an API on webContents to make it easier to fetch the current text selection. Otherwise I think currently one would need to run something like the following to fetch the selection, when running from the Main process.

const selectionText = await window.webContents.executeJavaScript('window.getSelection().toString()')
clipboard.writeFindText(selectionText)

@anyerqi
Copy link
Contributor Author

anyerqi commented Nov 12, 2023

OK, I understand, I will close this PR

@anyerqi anyerqi closed this Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encourage apps to support "Use Selection for Find"
4 participants