feat(richeditbox): implement Skia support - #23924
Conversation
…View/GetRangeFromPoint)
github-actions
Bot
added
platform/wasm 🌐
|
Claude finished @morning4coffe-dev's task in 15m 47s —— View job Code Review —
|
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | Medium | RichEditBoxAutomationPeer.cs:42 |
PatternInterface.TextEdit removed from GetPatternCore — Win32 has a GetTextEditProvider fallback, but macOS and X11 UIA providers do not. Non-Win32 Skia accessibility silently advertises no TextEdit pattern despite TextAdapter implementing ITextEditProvider. |
| 2 | Medium | SkiaAccessibilityBase.cs:415 |
NotifyTextEditTextChangedEvent is a no-op at the base class level; additionally, UpdateTextValue (which calls uno_accessibility_update_value on macOS) gates on GetPattern(PatternInterface.Value), which returns null for RichEditBox since Value was intentionally removed from GetPatternCore. VoiceOver text sync is silently dropped for RichEditBox. |
| 3 | Low | Win32ClipboardExtension.cs:126 |
ThreadLocal<HWND> created without trackAllValues: true — threads that exit without calling DestroyWindow leak message-only windows. The field's Dispose can't enumerate values to clean up. |
Cross-cutting notes
Solid areas:
- All
async voidpaths (PasteFromClipboard,BeginPasteFromClipboard) are wrapped in comprehensive try/catch with correct exception-type handling and re-throw on fatal exceptions. event Action<T>violation inTextInputPluginwas correctly converted toEventHandler<TextInputConnectionCreatedEventArgs>— no new violations introduced.RichEditBox.Properties.skia.csDP registrations useFrameworkPropertyMetadata, correct callback signatures, anddefault(T)for primitive defaults.AcceptsReturndefaults totrue(correct WinUI behavior forRichEditBox, unlikeTextBox).- RTF parser security limits (
MaxRtfInputLength = 16 MB,MaxGroupDepth = 256,MaxParsedGroups = 65 536) andFeatureConfiguration.RichEditBox.MaxRtfImportCharactersare appropriately bounded. - Generated stubs correctly updated —
__SKIA__removed from[Uno.NotImplemented]attributes and#ifguards in theGenerated/folder following the standard pattern. ITextBoxViewHostabstraction is a clean seam that keepsTextBlock.OwningTextBoxdecoupled from the concreteTextBoxtype.
Scope note: This is a large initial implementation (~4 800-line RTF codec + full editing engine). The Win32 path is well-exercised by the new Uno.UI.RuntimeTests.Win32UiaClient harness. The two medium findings above specifically affect macOS/X11 Skia accessibility, which would otherwise be silently incomplete at the IAccessible/AT-SPI layer.
github-actions
Bot
added
area/automation
WinAppSDK sync generator drift detected on
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| protected override object GetPatternCore(PatternInterface patternInterface) |
There was a problem hiding this comment.
PatternInterface.TextEdit was removed from GetPatternCore. Win32 has a dedicated fallback in Win32RawElementProvider.GetTextEditProvider() that retrieves it via the Text pattern, so Win32 UIA keeps working. However, non-Win32 Skia targets (macOS NSAccessibility / X11 AT-SPI) discover patterns via GetPattern(PatternInterface.TextEdit), and that now returns null for RichEditBox. Since TextAdapter (the m_textPattern object) implements ITextEditProvider, an explicit fallback for TextEdit identical to what the Win32 provider does would close the gap:
| protected override object GetPatternCore(PatternInterface patternInterface) | |
| protected override object GetPatternCore(PatternInterface patternInterface) | |
| { | |
| if (patternInterface == PatternInterface.Text | |
| || patternInterface == PatternInterface.Text2 | |
| || patternInterface == PatternInterface.TextEdit) | |
| { |
| } | ||
| } | ||
|
|
||
| public virtual void NotifyTextEditTextChangedEvent( |
There was a problem hiding this comment.
The base NotifyTextEditTextChangedEvent is a no-op, which is fine for the base class. However, none of the macOS, X11, or WASM subclasses override it, so TextEdit composition events are silently dropped on those platforms. That's expected for a first-pass Skia implementation, but the existing OnAutomationEvent handler at line 400-407 fires UpdateTextValue via peer.GetPattern(PatternInterface.Value) — and RichEditBox intentionally does not advertise Value pattern. macOS VoiceOver therefore won't pick up RichEditBox text changes via UpdateTextValue.
Consider propagating rich text updates from NotifyTextEditTextChangedEvent to UpdateTextValue using the peer's IValueProvider directly (cast peer as IValueProvider) as a bridge, or override this method in MacOSAccessibility to push the updated text. The Win32 path is unaffected since its UpdateTextValue is a no-op.
| } | ||
|
|
||
| _hwnd = PInvoke.CreateWindowEx( | ||
| _clipboardWindows = new ThreadLocal<HWND>(CreateClipboardWindow); |
There was a problem hiding this comment.
ThreadLocal<HWND> creates a new message-only HWND for every thread that calls GetClipboardWindow(). The created HWNDs are never destroyed when their threads exit because ThreadLocal<T> does not call DestroyWindow on eviction. On a long-running app that calls clipboard APIs from short-lived thread-pool threads this accumulates orphaned HWNDs (capped by the system limit of ~10,000 HWNDs per process, not a practical problem in normal use, but still a leak).
To clean up, use new ThreadLocal<HWND>(CreateClipboardWindow, trackAllValues: true) and call DestroyWindow on each tracked value in the singleton's (currently absent) disposal path, or pump a WM_CLOSE / WM_DESTROY via PostThreadMessage when each thread exits. If clipboard APIs are only called from the UI thread in practice, a simpler alternative is to drop ThreadLocal and go back to a single HWND (the previous design) with a comment explaining why multi-thread access is not expected.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23924/docs/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23924/wasm-skia-net9/index.html |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| var valueProvider = peer?.GetPattern(PatternInterface.Value) as IValueProvider; | ||
|
|
||
| Assert.IsNotNull(peer); | ||
| Assert.AreEqual("Notes", peer.GetName()); |
|
|
||
| Assert.IsNotNull(textProvider); | ||
| Assert.IsNotNull(textProvider2); | ||
| Assert.AreEqual(SupportedTextSelection.Single, textProvider.SupportedTextSelection); |
| Assert.AreEqual(1, selection.Length); | ||
| Assert.AreEqual("ell", selection[0].GetText(-1)); | ||
|
|
||
| var caret = textProvider2.GetCaretRange(out var isActive); |
| fake.SimulateCompositionStart(); | ||
| fake.SimulateCompositionUpdate("nihao", cursorPosition: 2, resolvedLength: 2); | ||
|
|
||
| Assert.AreEqual("nihao", textEditProvider.GetActiveComposition().GetText(-1)); |
| Assert.IsNotNull(peer); | ||
| Assert.IsNotNull(textProvider); | ||
|
|
||
| var firstChildren = textProvider.DocumentRange.GetChildren(); |
| Assert.AreEqual("logo", imageRange.GetText(-1)); | ||
| Assert.IsNotNull(linkTextChild); | ||
| Assert.IsNotNull(imageTextChild); | ||
| Assert.AreSame(peer, linkTextChild.TextContainer.AutomationPeer); |
| Assert.IsNotNull(linkTextChild); | ||
| Assert.IsNotNull(imageTextChild); | ||
| Assert.AreSame(peer, linkTextChild.TextContainer.AutomationPeer); | ||
| Assert.AreSame(peer, imageTextChild.TextContainer.AutomationPeer); |
| var textProvider = peer?.GetPattern(PatternInterface.Text) as ITextProvider; | ||
| Assert.IsNotNull(peer); | ||
| Assert.IsNotNull(textProvider); | ||
| var children = textProvider.DocumentRange.GetChildren(); |
| var peer = FrameworkElementAutomationPeer.CreatePeerForElement(sut); | ||
| var textProvider = peer?.GetPattern(PatternInterface.Text) as ITextProvider; | ||
| Assert.IsNotNull(textProvider); | ||
| var range = textProvider.DocumentRange.FindText("context", backward: false, ignoreCase: false); |
|
The build 225049 found UI Test snapshots differences: Details
|
3 participants
GitHub Issue: closes #3848
PR Type:
✨ Feature
What changed? 🚀
This adds a functional WinUI-compatible
RichEditBoximplementation for Uno's Skia targets: Desktop, Skia-on-Android/iOS, and Skia WebAssembly.User-visible behavior
RichEditTextDocument,ITextRange,ITextSelection, character/paragraph formats, tracked ranges, clipboard operations, streams, and undo/redo.What came from
/winui-portThe public/MUX-observable control behavior was ported and aligned from WinUI sources using the
/winui-portworkflow. The changed files retain MUX references or source-location comments for:RichEditBox_Partial.cppcontrol/template behaviorRichEditBoxAutomationPeer_Partial.cppautomation class/control semanticsTextAdapter_Partial.cppand Text/Text2 pattern behaviorTextBoxBase.cpp/.hediting, focus, flyout, proofing, gripper, and selection behaviorTextSelectionManager.cpp,TextControlFlyoutHelper.*,UIElement, andCoreServicesbehavior shared with the existing text stackPublic parity tests are intentionally unsuffixed where they can run against both Uno and native WinUI.
What had to be implemented from scratch
WinUI delegates the backing editor to non-public Windows RichEdit/Text Services and COM internals. Those APIs are not public or portable, so they could not be losslessly copied into Uno. The following cross-platform pieces are managed Uno implementations built from scratch against the public observable contract:
TextBlockpipelineUno does not expose private
ITextDocument2/ITextRange2COM identity, OLE hosting, marshaling, apartment, reference-counting, or Windows RichEdit implementation details. The public WinUI API and observable behavior remain the compatibility boundary.Scope and safeguards
IValueProviderABI is retained, but RichEditBox continues to advertise the WinUI Text/Text2 patterns rather than a Value pattern.Coverage
Validation performed
net10.0Skia SamplesApp build.articles/controls/RichEditBox.htmlwith no page-specific diagnostics. The full local docs build retains the known missing external/generated TOC baseline.PR Checklist ✅
Screenshots Compare Test Runresults.