API Review: Custom context menu Spellcheck#5553
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 -")
| @@ -0,0 +1,453 @@ | |||
| Custom Context Menu SpellCheck | |||
There was a problem hiding this comment.
Title: Consider renaming to "Spellcheck Support for Custom Context Menus". The current title "Custom Context Menu SpellCheck" reads as if you're customizing a spellcheck menu, rather than adding spellcheck to custom context menus. The revised title puts the feature (spellcheck support) first and the context (custom context menus) second, which better matches how the feature is described in the Background section.
| @@ -0,0 +1,453 @@ | |||
| Custom Context Menu SpellCheck | |||
There was a problem hiding this comment.
Nit: "SpellCheck" uses inconsistent casing — the rest of the spec uses "spellcheck" (lowercase) or "Spellcheck" (sentence case). Pick one and use it consistently. The IDL uses SpellCheck (PascalCase) for type names, which is correct, but prose should use lowercase "spellcheck" per Microsoft style guide (common nouns are not capitalized).
|
|
||
| - **`MisspelledWord`** — Read-only property returning the misspelled word under the cursor. Useful for | ||
| displaying "Suggestions for 'teh':" headers in custom menus. | ||
| - **`GetSpellCheckSuggestionsAsync`** — Retrieves suggestions as `ICoreWebView2ContextMenuItem` objects. |
There was a problem hiding this comment.
Style: Microsoft documentation avoids contractions. Change "doesn't" → "does not". (See Microsoft Style Guide - Contractions)
| // Inside ContextMenuRequested handler for an editable field: | ||
|
|
||
| // ── Step 1: Runtime version check ── | ||
| auto args2 = wil::try_com_query< |
There was a problem hiding this comment.
The existing ContextMenuRequested.md spec in this repo uses more descriptive example headings like "Win32 C++ Use Data to Display Custom Context Menu." Consider something like "Win32 C++ — Display Custom Context Menu with Spellcheck Suggestions" to match the established pattern and make it immediately clear what the example demonstrates.
| auto args2 = wil::try_com_query< | ||
| ICoreWebView2ContextMenuRequestedEventArgs2>(args); | ||
| if (!args2) | ||
| return S_OK; // Old runtime — use default menu. |
There was a problem hiding this comment.
This sample is a code fragment — it starts mid-handler and uses undefined helpers (ShowPopupAtCursor, AddMenuItems). The existing ContextMenuRequested.md spec shows complete, self-contained examples with the full �dd_ContextMenuRequested registration, lambda body, and helper implementations. This sample should be complete enough to compile (with the standard WebView2 boilerplate). At minimum, show the full event handler registration wrapping this code.
| /// This is the word that spellcheck flagged as incorrect and for which | ||
| /// suggestions are available via `GetSpellCheckSuggestionsAsync`. | ||
| /// The caller must free the returned string using `CoTaskMemFree`. | ||
| /// |
There was a problem hiding this comment.
�4a8f3b2-6c1d-4e9a-b5f7-2d8c9a0e1b34 — another placeholder. All three interface UUIDs need real values before this goes to API review.
| namespace Microsoft.Web.WebView2.Core | ||
| { | ||
| [Flags] | ||
| enum CoreWebView2ContextMenuDeferredCapabilities |
There was a problem hiding this comment.
The .NET/WinRT API definition uses // comments (C# style) instead of /// doc comments. WebView2 specs use /// for API documentation in the .NET projection. For example:
csharp /// <summary> /// The misspelled word under the cursor. /// </summary> String MisspelledWord { get; };
Also, CoreWebView2ContextMenuRequestedEventArgs2 : CoreWebView2ContextMenuRequestedEventArgs — WebView2's .NET projection does not use numbered EventArgs2 classes. New members are added to the existing CoreWebView2ContextMenuRequestedEventArgs class with an [interface_name] attribute block, as seen in ContextMenuRequested.md. Should be:
`csharp
runtimeclass CoreWebView2ContextMenuRequestedEventArgs
{
// Existing members unchanged.
[interface_name("ICoreWebView2ContextMenuRequestedEventArgs2")]
{
CoreWebView2ContextMenuDeferredCapabilities DeferredCapabilities { get; };
T GetDeferredCapability<T>();
}
}
`
| | User dismisses menu without selecting | Set `SelectedCommandId` to 0 or simply complete the deferral | | ||
|
|
||
| # Appendix | ||
|
|
There was a problem hiding this comment.
"Spellcheck applicable but word is empty — defensive check recommended" — if the SPELL_CHECK flag is set but MisspelledWord is empty, that is a bug, not a normal state. This row should say the behavior is undefined/unexpected and the host should treat it as "no spellcheck." Don't normalize buggy state as expected behavior in an API spec.
|
|
||
| ## Extensibility | ||
|
|
||
| The deferred capability pattern introduced by this feature is designed for zero-versioning extension: |
There was a problem hiding this comment.
"Set SelectedCommandId to 0 or simply complete the deferral" — the "or" is ambiguous. The existing ContextMenuRequested.md spec says the default SelectedCommandId is -1 (not 0), meaning no selection. Clarify: "Do not set SelectedCommandId (its default value of -1 indicates no selection) and complete the deferral."
| ## Planned SpellCheck Extensions | ||
|
|
||
| The following actions will be added as additional `ICoreWebView2ContextMenuItem` entries in the | ||
| collection returned by `GetSpellCheckSuggestionsAsync`. No new interfaces or methods are required: |
There was a problem hiding this comment.
Minor: "for example - " is casual tone. In Microsoft documentation style, use "For example:" or just show the value directly without the prefix.
SpellCheck Suggestions for Custom Context Menus
Enables host apps rendering custom context menus to asynchronously retrieve and apply spellcheck suggestions for misspelled words in editable fields.
New Interfaces:
ICoreWebView2ContextMenuRequestedEventArgs2 extends ICoreWebView2ContextMenuRequestedEventArgs
ICoreWebView2GetSpellCheckSuggestionsCompletedHandler
Usage flow:
No synchronous readiness query, the single async call covers both immediate and deferred resolution.