feat: Expose renderer spellcheck API #25060
Conversation
| #if BUILDFLAG(ENABLE_BUILTIN_SPELLCHECKER) | ||
| base::WeakPtr<SpellCheck> c = GetSpellCheck()->AsWeakPtr(); | ||
|
|
||
| process_dict.SetMethod("isWordMisspelled", base::BindRepeating(&IsWordMisspelled, c)); |
There was a problem hiding this comment.
Hm, so this definitely shouldn't be on process. My guess is it should probably be on something like webFrame 🤔
There was a problem hiding this comment.
Yeah agreed. I'm debating whether to put it on webFrame or adding its own module, something like require('electron').spellcheck.
Here's what I wrote in the PR description:
In practice, I have not found a way to pass the
WeakPtr<Spellcheck>to the renderer API stack. I was also unable to find a reference back toElectronRendererClient/RenderClientBasefrom an API module.
If someone with experience can give me some tips on how to achieve it that would be great!
There was a problem hiding this comment.
@lishid You can find some examples on adding new modules under shell/renderer/api/ folder.
Adding the APIs to webFrame would be simplest and you only need to move the code to shell/renderer/api/electron_api_web_frame.cc.
There was a problem hiding this comment.
@zcbenz Thank you for the help! In my working copy I was already able to move the code & API to webFrame, however, I was unable to find a reference to the spellchecker object which is located on RefererClientBase. Any tips? It seems that all the samples I've seen only references content::RenderFrame gotten from a v8::Context
There was a problem hiding this comment.
Since the RendererClientBase is guaranteed to have only one instance per process, I think you can add a RendererClientBase::Get() method to return the global instance, like what we do in ElectronBrowserMainParts::Get.
Chromium also does a similar thing for ChromeExtensionsRendererClient, so this pattern should be fine.
There was a problem hiding this comment.
Great having some code I can reference makes this a lot easier. Will update the PR soon, thanks again!
|
@MarshallOfSound Quick ping on this. Can you please advise on how I can get a reference from: This is blocking further progress on this PR. Thanks. |
|
Did a quick test and it seems to work pretty well. |
|
Also I can't seem to be able to view the lint results from CircleCI without "logging in with Github" and giving it read/write access to all my repos. The details link gives a 404 "You may have been logged out." Curious if this is intentionally configured, and if there's an alternate way to check the result details. |
| @@ -672,6 +675,38 @@ blink::WebCacheResourceTypeStats GetResourceUsage(v8::Isolate* isolate) { | |||
| return stats; | |||
| } | |||
|
|
|||
| bool SpellCheckWord(v8::Isolate* isolate, | |||
There was a problem hiding this comment.
Can you move this to anonymous namespace? i.e. where the GetRenderFrame are defined.
|
The lint error is: $ node ./script/lint.js && npm run lint:clang-format && npm run lint:docs
--- a/shell/renderer/api/electron_api_web_frame.cc
+++ b/shell/renderer/api/electron_api_web_frame.cc
@@ -675,9 +675,9 @@
return stats;
}
-bool SpellCheckWord(v8::Isolate* isolate,
+bool SpellCheckWord(v8::Isolate* isolate,
v8::Local<v8::Value> window,
- const std::string& word,
+ const std::string& word,
std::vector<base::string16>* optional_suggestions) {
size_t start;
size_t length;It seems that you need to remove some trailing whitespaces. I'm not sure why the lint CI result is not visible to you. /cc @jkleinsc |
|
Current implementation looks good to me, and the API will be discussed at next wg-api's meeting. Can you also add some documentation for the APIs? |
|
Sounds good, will do. |
|
@lishid @zcbenz I checked with CircleCI and in regards to viewing CircleCi results (eg lint results), you will need to login to CircleCI to see the details. You can see the overall status on GitHub, but viewing the details requires logging into CircleCI. You should be able to pick and choose what repos you give CircleCI access to. |
|
@jkleinsc I see. It seems like the only choice given was "All repos" or "Public repos only" Choosing "Public repos only" asks for read-write access to all public repos: IIRC the problem is with Github's OAuth not having granular control over specific repos, but I guess in the meantime it shouldn't be much trouble registering a separate account if access control is an issue. |
|
Test and docs have both been added. Excited to finally get native spellchecker support for our CodeMirror editor! |
|
The API WG reviewed this on Sept 21, 2020. |
|
Quick follow-up: is there anything still blocking this PR? |
| base::string16 w = base::UTF8ToUTF16(word); | ||
| int id = render_frame->GetRoutingID(); | ||
| return client->GetSpellCheck()->SpellCheckWord( | ||
| w.c_str(), 0, word.size(), id, &start, &length, optional_suggestions); |
There was a problem hiding this comment.
Hang on, I think we can just specify kNoTag for the tag? Which would mean we wouldn't need to fetch the render frame, which makes me think maybe this API shouldn't be on webFrame at all?
There was a problem hiding this comment.
hm... https://source.chromium.org/chromium/chromium/src/+/master:components/spellcheck/renderer/spellcheck_provider.cc;l=310;drc=c83ae1d48c762639e626be9be0c77a209fdea359 is a reference that uses the routing id. there are other places in the code that use kNoTag. cc @MarshallOfSound for further review
There was a problem hiding this comment.
It seems to be used in the macOS platform spellchecker impl: https://source.chromium.org/chromium/chromium/src/+/master:components/spellcheck/browser/spellcheck_platform_mac.mm;l=201;drc=da08b7363d0947b0e7d98c5282b4cca513f62100
|
Quick follow-up: is there anything still blocking this PR? Seems like it's ready to merge. |
|
I did a rebase which seemed to have fixed the specific build issue with mac, but latest master has some other build failures :( Anything else I need to do before a merge can happen? 🚀 |
|
The CI is failing on master branch, we need to fix it first. #25992 |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
Release Notes Persisted
|
|
Congrats!!!!! |
|
@zcbenz Thanks for the merge! Thanks again for all the help on this PR! |
|
Yeah it will be included in v12, for backporting to v11 please check https://github.com/electron/governance/blob/master/wg-releases/feature-backport-requests.md. |
|
Sounds good.
I can take responsibility for this PR if that means we can backport it to v11. Given there's still over 3 weeks until v11 goes stable, let me know what I should do to initiate the backport request. |
|
/trop run backport |
|
The backport process for this PR has been manually initiated, here we go! :D |
|
I was unable to backport this PR to "11-x-y" cleanly; |
|
I have added this PR to @electron/wg-releases 's meeting items, once it is approved you can start backporting it to 11-x. |
|
Seems like we got less than a week to go before v11 hits stable so I'm guessing a backport isn't likely to happen, is that correct? |
|
@lishid the @electron/wg-releases missed this one, but we will vote on it. It probably won't make 11.0.0, but if approved it could go in as a 11.1.0 release. |
|
What's the current status of this request, when is it going to be implemented into Electron? |
Description of Change
Exposes renderer side browser spellcheck API to the javascript environment.
Resolves #22829
Rationale: Electron-based apps that uses CodeMirror (a hugely popular editor) are unable to benefit from the built-in spellchecker since CodeMirror does not use a ContentEditable (everything is faked). CodeMirror exposes a way to highlight errors, but given that we are currently unable to ask for a spellcheck from javascript, all Electron+CodeMirror apps use a js-based spellcheck library like https://github.com/cfinke/Typo.js which is considerably slower than the built-in native spellchecker.
This PR adds the following API:
isWordMisspelled(word: string): booleangetWordSuggestions(word: string): string[]Current WIP adds the methods to the global
processobject which is just so I can test that it works.I've tested this and it works. Now to figure out where to put the API.
Ideally I would like this to be a module on
require('electron'), similar to IPCRenderer.In practice, I have not found a way to pass the
WeakPtr<Spellcheck>to the renderer API stack. I was also unable to find a reference back toElectronRendererClient/RenderClientBasefrom an API module.If someone with experience can give me some tips on how to achieve it that would be great!
Also currently missing tests and documentation.
Checklist
npm testpassesRelease Notes
Notes: Added spellcheck API to renderer.