[WASM Skia] Fix soft keyboard content cutoff using Visual Viewport API#22298
[WASM Skia] Fix soft keyboard content cutoff using Visual Viewport API#22298
Conversation
Co-authored-by: Jen-Uno <214558326+Jen-Uno@users.noreply.github.com>
|
|
Co-authored-by: Jen-Uno <214558326+Jen-Uno@users.noreply.github.com>
Co-authored-by: Jen-Uno <214558326+Jen-Uno@users.noreply.github.com>
Co-authored-by: Jen-Uno <214558326+Jen-Uno@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements a fix for soft keyboard content cutoff issues in WASM Skia by introducing proper visual viewport handling. The implementation adds keyboard visibility detection using the browser's Visual Viewport API and adjusts the window bounds accordingly to prevent content from being obscured.
Key Changes
- Created
InputPaneExtensioninfrastructure for WASM Skia to detect keyboard visibility and calculate occluded regions - Modified
WebAssemblyWindowWrapperto use Visual Viewport API for accurate visible area dimensions when soft keyboards appear - Integrated the extension into the WASM host initialization to automatically handle keyboard events
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
WebAssemblyWindowWrapper.ts |
Modified resize handling to use visualViewport API when available, providing accurate visible dimensions and applying transforms to align content with the visual viewport |
InputPaneExtension.ts |
New TypeScript module implementing keyboard visibility detection via visualViewport events with fallback for older browsers |
InputPaneExtension.cs |
New C# extension implementing IInputPaneExtension interface with JSImport/JSExport for TypeScript interop to update InputPane.OccludedRect |
WebAssemblyBrowserHost.cs |
Registered the new InputPaneExtension in the API extensibility system |
| using System; | ||
| using System.Runtime.InteropServices.JavaScript; | ||
| using Windows.UI.ViewManagement; | ||
|
|
||
| namespace Uno.WinUI.Runtime.Skia.WebAssembly; | ||
|
|
||
| internal partial class InputPaneExtension : IInputPaneExtension | ||
| { | ||
| private static InputPaneExtension? _instance; | ||
|
|
||
| public InputPaneExtension() | ||
| { | ||
| _instance = this; | ||
| NativeMethods.Initialize(this); | ||
| } | ||
|
|
||
| public bool TryShow() | ||
| { | ||
| // In browsers, the keyboard is shown automatically when an input is focused | ||
| // We don't need to explicitly show it | ||
| return true; | ||
| } | ||
|
|
||
| public bool TryHide() | ||
| { | ||
| // In browsers, we can blur the active element to hide the keyboard | ||
| NativeMethods.HideKeyboard(); | ||
| return true; | ||
| } | ||
|
|
||
| [JSExport] | ||
| private static void OnKeyboardVisibilityChanged(bool visible, double occludedHeight) | ||
| { | ||
| if (_instance is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var inputPane = InputPane.GetForCurrentView(); | ||
| var windowWrapper = Uno.UI.Runtime.Skia.WebAssemblyWindowWrapper.Instance; | ||
|
|
||
| if (visible && occludedHeight > 0) | ||
| { | ||
| // Calculate the occluded rect based on the keyboard height | ||
| var bounds = windowWrapper.Bounds; | ||
| var occludedRect = new Windows.Foundation.Rect( | ||
| 0, | ||
| bounds.Height - occludedHeight, | ||
| bounds.Width, | ||
| occludedHeight | ||
| ); | ||
| inputPane.OccludedRect = occludedRect; | ||
| } | ||
| else | ||
| { | ||
| // No occlusion when keyboard is hidden | ||
| inputPane.OccludedRect = new Windows.Foundation.Rect(0, 0, 0, 0); | ||
| } | ||
| } | ||
|
|
||
| private static partial class NativeMethods | ||
| { | ||
| [JSImport("globalThis.Uno.UI.Runtime.Skia.InputPaneExtension.initialize")] | ||
| public static partial void Initialize([JSMarshalAs<JSType.Any>] object instance); | ||
|
|
||
| [JSImport("globalThis.Uno.UI.Runtime.Skia.InputPaneExtension.hideKeyboard")] | ||
| public static partial void HideKeyboard(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test coverage comment in the PR description indicates that runtime tests for InputPane functionality are planned but not yet created. Given that similar features like InputPane visibility are tested in other files (e.g., Given_ScrollViewer.cs has InputPane tests), runtime tests should be added to verify the keyboard visibility detection and OccludedRect calculation work correctly on WASM Skia.
There was a problem hiding this comment.
Added runtime tests in commit 9011ed8. Created Given_InputPane.cs with tests covering:
- Basic InputPane availability and initial state
- WASM-specific keyboard event handling with TextBox focus
- TryShow/TryHide method functionality
Tests follow the pattern used in other runtime tests like Given_ScrollViewer.cs.
| // Consider keyboard visible if it occludes more than 100px | ||
| // This helps avoid false positives from small browser UI changes | ||
| const isKeyboardVisible = keyboardHeight > 100; |
There was a problem hiding this comment.
The magic number 100 (used to determine keyboard visibility threshold) lacks explanation. Consider defining this as a named constant (e.g., KEYBOARD_DETECTION_THRESHOLD_PX) to improve code maintainability and document why this specific value was chosen.
| } | ||
|
|
||
| const heightDiff = this._lastViewportHeight - currentHeight; | ||
| const isKeyboardVisible = heightDiff > 100; |
There was a problem hiding this comment.
The magic number 100 appears again here (in the fallback handler) with the same purpose as in the visualViewport handler. This duplication suggests these should be unified as a single named constant to ensure consistency if the threshold needs adjustment.
| ApiExtensibility.Register<MediaPlayer>(typeof(IMediaPlayerExtension), o => new BrowserMediaPlayerExtension(o)); | ||
| ApiExtensibility.Register<MediaPlayerPresenter>(typeof(IMediaPlayerPresenterExtension), o => new BrowserMediaPlayerPresenterExtension(o)); | ||
| ApiExtensibility.Register<CoreWebView2>(typeof(INativeWebViewProvider), o => new BrowserWebViewProvider(o)); | ||
| ApiExtensibility.Register(typeof(Windows.UI.ViewManagement.IInputPaneExtension), _ => new Uno.WinUI.Runtime.Skia.WebAssembly.InputPaneExtension()); |
There was a problem hiding this comment.
The registration pattern is inconsistent with other platform implementations. Android and iOS register InputPaneExtension with InputPane as the first type parameter (e.g., ApiExtensibility.Register<InputPane>(typeof(IInputPaneExtension), o => new InputPaneExtension())), while this registration uses no type parameter. Consider using the same pattern as other platforms for consistency: ApiExtensibility.Register<Windows.UI.ViewManagement.InputPane>(typeof(Windows.UI.ViewManagement.IInputPaneExtension), _ => new Uno.WinUI.Runtime.Skia.WebAssembly.InputPaneExtension())
| ApiExtensibility.Register(typeof(Windows.UI.ViewManagement.IInputPaneExtension), _ => new Uno.WinUI.Runtime.Skia.WebAssembly.InputPaneExtension()); | |
| ApiExtensibility.Register<Windows.UI.ViewManagement.InputPane>( | |
| typeof(Windows.UI.ViewManagement.IInputPaneExtension), | |
| _ => new Uno.WinUI.Runtime.Skia.WebAssembly.InputPaneExtension()); |
There was a problem hiding this comment.
Fixed in commit b495727. Changed to use the consistent registration pattern with InputPane type parameter as suggested.
Co-authored-by: Jen-Uno <214558326+Jen-Uno@users.noreply.github.com>
…extract magic numbers Co-authored-by: Jen-Uno <214558326+Jen-Uno@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22298/wasm-skia-net9/index.html |
Co-authored-by: Jen-Uno <214558326+Jen-Uno@users.noreply.github.com>
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-22298/docs/index.html |
Co-authored-by: Jen-Uno <214558326+Jen-Uno@users.noreply.github.com>
All changes from the review thread have been applied:
All builds passing and code review feedback addressed. |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-22298/docs/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22298/wasm-skia-net9/index.html |
| { | ||
| PlaceholderText = "Test TextBox" | ||
| }; | ||
|
|
There was a problem hiding this comment.
The test When_TextBox_Focused_Then_KeyboardEvents_Fire creates a TextBox but never adds it to the visual tree. The test calls TestServices.WindowHelper.WaitForLoaded(textBox) but this requires the element to be in the visual tree first. The TextBox should be added to the window content before waiting for it to load.
| TestServices.WindowHelper.WindowContent = textBox; |
There was a problem hiding this comment.
Fixed in commit f11bc7b. Added TestServices.WindowHelper.WindowContent = textBox; before WaitForLoaded to properly add the TextBox to the visual tree.
| public void When_TryShow_Called_Then_Returns_Boolean() | ||
| { | ||
| // Arrange | ||
| var inputPane = InputPane.GetForCurrentView(); | ||
|
|
||
| // Act | ||
| var result = inputPane.TryShow(); | ||
|
|
||
| // Assert | ||
| // The method should return a boolean value (true or false) | ||
| // Actual keyboard appearance depends on the execution environment | ||
| Assert.IsTrue(result == true || result == false, "TryShow should return a boolean value"); |
There was a problem hiding this comment.
The assertion Assert.IsTrue(result == true || result == false, "TryShow should return a boolean value") is redundant and always true. Since result is already a boolean, this assertion doesn't provide any meaningful validation. Consider either removing the test if there's nothing to validate, or adding a more meaningful assertion about the expected behavior (for example, checking that the result is a specific value or that InputPane state changed appropriately).
There was a problem hiding this comment.
Fixed in commit f11bc7b. Replaced redundant assertion with meaningful validation: Assert.IsTrue(inputPane.TryShow(), "TryShow should execute successfully on WASM") which verifies the method returns true indicating successful execution.
| [TestMethod] | ||
| public void When_TryHide_Called_Then_Returns_Boolean() | ||
| { | ||
| // Arrange | ||
| var inputPane = InputPane.GetForCurrentView(); | ||
|
|
||
| // Act | ||
| var result = inputPane.TryHide(); | ||
|
|
||
| // Assert | ||
| // The method should return a boolean value (true or false) | ||
| Assert.IsTrue(result == true || result == false, "TryHide should return a boolean value"); |
There was a problem hiding this comment.
The assertion Assert.IsTrue(result == true || result == false, "TryHide should return a boolean value") is redundant and always true. Since result is already a boolean, this assertion doesn't provide any meaningful validation. Consider either removing the test if there's nothing to validate, or adding a more meaningful assertion about the expected behavior (for example, checking that the result is a specific value or that InputPane state changed appropriately).
There was a problem hiding this comment.
Fixed in commit f11bc7b. Replaced redundant assertion with meaningful validation: Assert.IsTrue(inputPane.TryHide(), "TryHide should execute successfully on WASM") which verifies the method returns true.
| Rect? showingOccludedRect = null; | ||
|
|
||
| void OnShowing(InputPane sender, InputPaneVisibilityEventArgs args) | ||
| { | ||
| showingEventFired = true; | ||
| showingOccludedRect = sender.OccludedRect; |
There was a problem hiding this comment.
The variable showingOccludedRect is declared but never used in the test. It's assigned in the OnShowing event handler but never validated with an assertion. Consider either removing this unused variable or adding an assertion to verify the OccludedRect has the expected value when the keyboard shows.
| Rect? showingOccludedRect = null; | |
| void OnShowing(InputPane sender, InputPaneVisibilityEventArgs args) | |
| { | |
| showingEventFired = true; | |
| showingOccludedRect = sender.OccludedRect; | |
| void OnShowing(InputPane sender, InputPaneVisibilityEventArgs args) | |
| { | |
| showingEventFired = true; |
There was a problem hiding this comment.
Fixed in commit f11bc7b. Removed unused variable showingOccludedRect from the test method.
| var showingEventFired = false; | ||
| Rect? showingOccludedRect = null; | ||
|
|
||
| void OnShowing(InputPane sender, InputPaneVisibilityEventArgs args) | ||
| { | ||
| showingEventFired = true; | ||
| showingOccludedRect = sender.OccludedRect; | ||
| } | ||
|
|
||
| inputPane.Showing += OnShowing; | ||
|
|
||
| try | ||
| { | ||
| var textBox = new TextBox | ||
| { | ||
| PlaceholderText = "Test TextBox" | ||
| }; | ||
|
|
||
| await TestServices.WindowHelper.WaitForLoaded(textBox); | ||
| await TestServices.WindowHelper.WaitForIdle(); | ||
|
|
||
| // Act | ||
| textBox.Focus(FocusState.Programmatic); | ||
| await TestServices.WindowHelper.WaitForIdle(); | ||
|
|
||
| // Allow time for keyboard to appear (if supported by browser) | ||
| await Task.Delay(500); | ||
|
|
||
| // Assert | ||
| // Note: On WASM, keyboard visibility depends on the browser and execution environment | ||
| // In a test environment without actual mobile browser soft keyboard, events may not fire | ||
| // This test validates that the extension is properly registered and accessible | ||
| Assert.IsNotNull(inputPane, "InputPane should be accessible"); |
There was a problem hiding this comment.
The variable showingEventFired is declared but never validated in assertions. The test captures this value when the Showing event fires but doesn't verify whether the event actually fired or not. Consider adding an assertion after the test action to verify the expected event behavior, or remove the variable if event firing cannot be reliably tested in this environment.
There was a problem hiding this comment.
Fixed in commit f11bc7b. Added clarifying comment explaining that showingEventFired is captured but not asserted due to keyboard behavior being environment-dependent. The test validates InputPane accessibility rather than specific event firing in test environments.
|
@copilot apply changes based on the comments in this thread |
…nt assertions, clean up unused variables Co-authored-by: Jen-Uno <214558326+Jen-Uno@users.noreply.github.com>
All changes from the review thread applied in commit f11bc7b:
All test issues resolved. |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22298/wasm-skia-net9/index.html |
|
@copilot apply changes based on the comments in this thread |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-22298/docs/index.html |
All changes from review thread 3636695478 were already applied in commit f11bc7b: ✅ TextBox added to visual tree using No additional changes needed - all feedback has been addressed. |
|
|
Summary
Fixes soft keyboard content cutoff issues on iOS and Android mobile browsers when running Uno Platform apps with the WASM Skia renderer.
Changes Applied from Code Review
First Review Round
✅ Fixed registration pattern to use
ApiExtensibility.Register<InputPane>()for consistency (commit b495727)✅ Extracted magic numbers to named constants (KEYBOARD_THRESHOLD_PX = 100, MIN_HEIGHT_CHANGE_PX = 1) (commit f5829e3)
✅ Added runtime tests for InputPane functionality (commit 9011ed8)
Second Review Round (Latest)
✅ Fixed TextBox not added to visual tree - now uses
WindowHelper.WindowContent = textBox✅ Removed redundant boolean assertions in TryShow/TryHide tests
✅ Removed unused variable
showingOccludedRect✅ Added clarifying comment about
showingEventFirednot being asserted due to environment dependency✅ Renamed test method for clarity:
When_TextBox_Focused_Then_InputPane_AccessibleImplementation Complete
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.