[Windows] Fixed TimePicker CharacterSpacing issue #30533
Conversation
|
Hey there @@SubhikshaSf4851! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
dotnet-policy-service
bot
added
community ✨
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where the Windows TimePicker control did not apply character spacing to its internal text blocks when loaded asynchronously. It enhances the UpdateCharacterSpacing extension to defer applying the spacing until after the control is loaded and adds helper methods to target each text block.
- Applies
CharacterSpacingtoHourTextBlock,MinuteTextBlock, andPeriodTextBlockwhen the control is ready. - Uses
OnLoadedto handle async loading scenarios. - Adds UI tests in both HostApp and Shared.Tests to visually verify the fix.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Core/src/Platform/Windows/TimePickerExtensions.cs | Enhanced UpdateCharacterSpacing to wait for IsLoaded and apply spacing to each part. |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue30199.cs | Added a UI test that waits for the TimePicker and captures a screenshot. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue30199.cs | Created a HostApp page with a TimePicker configured to demonstrate the fix. |
Comments suppressed due to low confidence (2)
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue30199.cs:21
- The UI test currently only waits for the element and takes a screenshot. Consider adding assertions that programmatically verify the CharacterSpacing on the internal TextBlocks or compare pixel spacing to ensure automated validation.
App.WaitForElement("timePicker");
src/Core/src/Platform/Windows/TimePickerExtensions.cs:23
- [nitpick] This public extension method lacks XML documentation. Adding a
<summary>explaining its purpose and behavior (especially the use ofOnLoaded) would improve discoverability and maintainability.
public static void UpdateCharacterSpacing(this TimePicker platformTimePicker, ITimePicker timePicker)
| SetCharacterSpacingToBlocks(platformTimePicker, "HourTextBlock"); | ||
| SetCharacterSpacingToBlocks(platformTimePicker, "MinuteTextBlock"); | ||
| SetCharacterSpacingToBlocks(platformTimePicker, "PeriodTextBlock"); |
There was a problem hiding this comment.
[nitpick] The part names 'HourTextBlock', 'MinuteTextBlock', and 'PeriodTextBlock' are hardcoded. Consider defining these strings as constants or iterating over a collection to reduce duplication and make future template changes easier to manage.
| SetCharacterSpacingToBlocks(platformTimePicker, "HourTextBlock"); | |
| SetCharacterSpacingToBlocks(platformTimePicker, "MinuteTextBlock"); | |
| SetCharacterSpacingToBlocks(platformTimePicker, "PeriodTextBlock"); | |
| // Iterate over part names using an array | |
| string[] partNames = { HourTextBlock, MinuteTextBlock, PeriodTextBlock }; | |
| foreach (var partName in partNames) | |
| { | |
| SetCharacterSpacingToBlocks(platformTimePicker, partName); | |
| } |
Copilot uses AI. Check for mistakes.
jsuarezruiz
added
platform/windows
area-controls-datetimepicker
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| public void Issue30199TimePickerCharacterSpacingShouldApply() | ||
| { | ||
| App.WaitForElement("timePicker"); | ||
| VerifyScreenshot(); |
There was a problem hiding this comment.
Pending snapshots, running a build.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Root Cause:
The UpdateCharacterSpacing method didn't apply spacing to the individual text blocks in TimePicker
Description of Change
Enhanced the UpdateCharacterSpacing method to apply CharacterSpacing to individual text blocks (HourTextBlock, MinuteTextBlock, and PeriodTextBlock) within the TimePicker. This ensures the property works correctly even when the control is loaded asynchronously.
Issues Fixed
Fixes #30199
Tested the behaviour in the following platforms
Screenshot