API Review: Custom context menu Spellcheck#5553
API Review: Custom context menu Spellcheck#5553anuragkumar878 wants to merge 13 commits intomainfrom
Conversation
|
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new API review spec to extend the existing WebView2 ContextMenuRequested customization flow with spellcheck support, enabling hosts that render custom context menus to surface and apply spellcheck suggestions.
Changes:
- Adds a new spec describing custom-context-menu spellcheck scenarios and motivation.
- Provides Win32 C++ and C# example flows for querying suggestions, handling async readiness, and applying a suggestion.
- Proposes new Win32 COM interfaces (
ICoreWebView2ContextMenuTarget2,ICoreWebView2ContextMenuRequestedEventArgs2) plus corresponding .NET/WinRT projections.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shrinaths
left a comment
There was a problem hiding this comment.
Review Summary
The architecture and design are sound — DeferredCapabilityDiscovery, ContextMenuItemCollection return type, unified SelectedCommandId commanding, and always-async callback are all well done. The comments below focus on bringing the spec up to the polish expected for Windows API review board submission:
Must fix:
- Placeholder UUIDs (lines 260, 298, 320) — generate real values
- .NET projection should use [interface_name] pattern, not EventArgs2 class (line 363)
- Verify MIDL3 format requirement with SDK team (line 233)
Should fix:
- Rename title to "Spellcheck Support for Custom Context Menus"
- Complete the samples (both C++ and C# have undefined helpers / placeholder comments)
- Add CHECK_FAILURE() consistently in C++ sample
- Fix SelectedCommandId default value in error table (it's -1, not 0)
Nit:
- Consistent casing of "spellcheck" in prose
- Avoid contractions per Microsoft style guide
- Use /// doc comments in .NET projection
- Clean up casual language ("for example -")
specs/CustomContextMenuSpellcheck.md
Outdated
| [this, args, spellCheck, items, deferral, | ||
| word = std::wstring(misspelledWord.get())]() | ||
| { | ||
| spellCheck->GetSpellCheckSuggestionsAsync( |
There was a problem hiding this comment.
The implication of this sample code is that GetSpellCheckSuggestionsAsync is fast enough that you can delay showing the entire context menu until it async completes. Is this always true? I thought at some point the browser would show an initial context menu and then update it async later when the spell check suggestions become available. This should be documented.
There was a problem hiding this comment.
Correct, it's not always instant. The platform spellchecker runs asynchronously in the browser process. When ContextMenuRequested fires, suggestions may already be resolved (kReady) or still in flight (kNotReady). GetSpellCheckSuggestions abstracts this, the handler fires on the next message loop iteration if ready, or when the IPC response arrives if not.
The sample uses the simpler "wait-then-show" pattern. In practice the delay is typically imperceptible (suggestions often resolve before or within milliseconds of the event), but it's not guaranteed.
I've added an "Async Timing" section to the spec documenting both host patterns:
- Pattern 1 (wait-then-show) — simpler, used in the sample, delays menu if suggestions aren't ready
- Pattern 2 (show-then-update) — shows menu immediately with placeholder, updates when handler fires (mirrors browser built-in behavior)
There was a problem hiding this comment.
@anuragkumar878 Lets explain this subtelty in the doc for completeness.
| cur->get_Name(&name); | ||
| // Skip built-in spellcheck items already | ||
| // handled above. | ||
| if (wcsstr(name.get(), L"spellCheck")) |
There was a problem hiding this comment.
Why is this built-in spellcheck item here and what is the relationship between it and this new spellcheck work? Should we be placing the new spell check menu items in place of this menu item rather than at the top? We should document this.
There was a problem hiding this comment.
GetSpellCheckSuggestions returns separate objects (Name = "spellCheckSuggestion", different CommandId values) representing the same underlying suggestions, delivered through the new async API. Both exist because the built-in items maintain backward compatibility for hosts that don't use the new API.
On placement: The sample puts new suggestions at the top and filters out the built-in duplicates. Hosts that want to preserve the original position can scan MenuItems for the first "spellcheck" entry's index and insert there instead.
Details added in the spec.
specs/CustomContextMenuSpellcheck.md
Outdated
|
|
||
| **Async contract:** The handler fires exactly once, always asynchronously (posted to the caller's | ||
| message loop, never invoked inline). Only one handler may be registered; a second call returns | ||
| `E_ILLEGAL_METHOD_CALL`. |
There was a problem hiding this comment.
Does this mean you can't call GetSpellCheckSuggestionsAsync twice? We should be able to call GetSpellCheckSuggestionsAsync more than once. Or what exactly does it mean by 'Only one handler may be registered'? If this trying to describe standard async method behavior then you can just say that instead.
There was a problem hiding this comment.
Updated the spec to allow calling GetSpellCheckSuggestions multiple times supported.
| /// Retrieves the capability-specific interface for a deferred | ||
| /// capability. Returns null if the capability is not applicable. | ||
| /// </summary> | ||
| T GetDeferredCapability<T>(); |
There was a problem hiding this comment.
The deferred capability APIs seem unnecessarily complex. Instead lets put CoreWebView2ContextMenuSpellCheck directly as a SpellCheck property on ICoreWebView2ContextMenuRequestedEventArgs2. We already have to handle cases where its an old runtime and the newer interface isn't supported and have the end dev have a way to detect and handle this case. For additional cases where its a newer runtime but spell check is not available we can have a null SpellCheck property.
There was a problem hiding this comment.
Making EventArgs2 spellcheck specific was our first instinct. But we have since reconsidered.
Consider future extensions - supporting emoji's. In that case EventArgs2 would have to be extended to EventArgs3 and embellished with emoji specific properties. Since there is no "is-a" relationship between Spellcheck and Emoji functionality ... an extension like this would make little sense.
Therefore we have created EventArgs2 with a way enumerate which capability is it that this Event refers to and then query the respective interface to access further event specific details which are otherwise unrelated to each other. The "is-a" relationship is with EventArgs but not amongst each other.
Hope that explains.
specs/CustomContextMenuSpellcheck.md
Outdated
| /// the correction. | ||
| /// | ||
| /// Only one handler may be registered per event invocation. A second call | ||
| /// returns `E_ILLEGAL_METHOD_CALL`. |
There was a problem hiding this comment.
Same question about calling the method multiple times I had at the top. We shouldn't throw if you call the method more than once.
There was a problem hiding this comment.
Removed the E_ILLEGAL_METHOD_CALL guard and now support multiple calls. Each call registers its own handler; all fire with the same results when suggestions resolve
Spellcheck Suggestions for Custom Context Menus
Enables host apps rendering custom context menus to discover, retrieve, and apply spellcheck suggestions for misspelled words in editable fields. Introduces a
deferred capability discovery pattern on EventArgs2 that is extensible to future async capabilities without additional EventArgs versions.
New Interfaces:
ICoreWebView2ContextMenuRequestedEventArgs2 extends ICoreWebView2ContextMenuRequestedEventArgs
ICoreWebView2ContextMenuSpellCheck (acquired via GetDeferredCapability)
Fires immediately (via PostTask) if suggestions are already resolved, or deferred until ready.
ICoreWebView2GetSpellCheckSuggestionsCompletedHandler
ICoreWebView2ContextMenuItem with Label (suggestion text) and CommandId (opaque identifier).
Usage flow:
Key design decisions: