Fix RatingControl scaling #187
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is there any way to write a test to cover this? I don't know if it's possible to simulate text scaling but we could just set the registry key for the duration of the test before we launch the app -- I think it's Software\Microsoft\Accessibility\TextScaleFactor -- DWORD that is the percentage (so 100 is 1.0 scaling). |
jevansaks
reviewed
Jan 16, 2019
|
Do we have examples for how to set regkey before running test @jevansaks ? Wasn't able to find it in our test cases... |
In TAEF-based tests you can flag a method to run as administrator so you could have your test setup and test cleanup methods set the reg key and unset it. But I also understand that we're not well set up to write this kind of test at the moment so it's skip it. |
chrisglein
approved these changes
Jan 17, 2019
jevansaks
reviewed
Jan 23, 2019
dev/RatingControl/RatingControl.xaml
Outdated
| @@ -52,7 +52,7 @@ | |||
|
|
|||
| <StackPanel Orientation="Horizontal" Grid.Row="0" Margin="-20,-20,-20,-20"> | |||
| <StackPanel x:Name="RatingBackgroundStackPanel" Orientation="Horizontal" Background="Transparent" Margin="20,20,0,20"/> | |||
| <TextBlock x:Name="Caption" Height="32" Margin="4,9,20,0" TextLineBounds="TrimToBaseline" Style="{ThemeResource CaptionTextBlockStyle}" VerticalAlignment="Center" AutomationProperties.AccessibilityView="Raw" AutomationProperties.Name="RatingCaption" IsHitTestVisible="False" Text="{TemplateBinding Caption}"/> | |||
| <TextBlock x:Name="Caption" Height="32" Margin="4,22,20,0" TextLineBounds="TrimToBaseline" Style="{ThemeResource CaptionTextBlockStyle}" VerticalAlignment="Center" AutomationProperties.AccessibilityView="Raw" AutomationProperties.Name="RatingCaption" IsHitTestVisible="False" Text="{TemplateBinding Caption}"/> | |||
There was a problem hiding this comment.
This causes the code-behind to be out of sync with anyone who copied the template. Why was this margin change necessary?
There was a problem hiding this comment.
Right, this is not used anywhere, the top margin value is now calculated by code-behind based on text scale factor. I will revert it.
For people who already retemplated it and changed this margin, their value will be ignored, do you think that will cause problems?
jevansaks
approved these changes
Jan 23, 2019
kaiguo
force-pushed
the
user/kaiguo/fix-ratingcontrol-scaling
branch
from
ac854b7 to
3e463a5
Compare
ranjeshj
added a commit
that referenced
this pull request
Jan 25, 2019
* Update localization workflow for breaking changes (#162) * Adding instrumentation for AcrylicBrush.TintLuminosityOpacity propert… (#149) * Adding instrumentation for AcrylicBrush.TintLuminosityOpacity property change * Changing telemetry logging of property change as a member instead of class * Oops. Forgot to remove the old id. * Back-porting external changes from the OS repo. (#175) * Avoid Narrator pronouncing the MenuFlyoutItem's keyboard accelerator. (#181) * Avoid Narrator pronouncing the MenuFlyoutItem's keyboard accelerator. * Expanding fix to pre-19H1 releases. * Add screen capture and improved logging to helix tests (#176) Enable /screencaptureonerror for TAEF tests and upload the screenshots to helix storage. Also upload te.wtl log file. Report uploaded urls of both of these to the Azure DevOps test report so that someone investigating a test failure can link directly to them instead of having to go to through the Mission Control site. Here is an example run showing this in action: https://dev.azure.com/ms/microsoft-ui-xaml/_build/results?buildId=1332&view=ms.vss-test-web.test-result-details * Update the bug report issue template to not use checkboxes (#183) * Renaming a few Scroller/ScrollViewer APIs (#185) * Fixing Scroller's SnapPointsWithInvalidArgsThrow test. * Renaming enum ScrollerState to InteractionState. Renaming its enum values Interacting to Interaction and CustomAnimation to Animation. * Updating ScrollBarVisibility enum. Replacing Collapsed with Hidden. * Fix OS ports (#186) * Update feature request issue template (#191) * Make CalendarViewCalendarItemBackground the same as RS3 (#189) * add SystemControlBackgroundAltHighRevealBackgroundBrush * lift CalendarViewCalendarItemRevealBackground * move CalendarViewCalendarItemRevealBackground to difference file * Update TwoPaneView for API changes (#193) * MUX NavigationView should throw if WUXC NavViewItems are added to it (#69) MUX NavigationView should throw if WUXC NavViewItems are added to it Use IsItemItsOwnContainerOverride as a hook to approximate list contents change events and deny any WUX items in the MUX list. [Internal Issue](https://microsoft.visualstudio.com/OS/ft_xamlcon/_workitems/edit/19017881) * Applying some of the changes that came out of the API review meetings: (#200) - Renaming enumerations ScrollerScrollMode --> ScrollMode ScrollerZoomMode --> ZoomMode ScrollerChainingMode --> ChainingMode ScrollerRailingMode --> RailingMode ScrollerInputKind --> InputKind - Removing the ScrollMode.Auto enum value and make ScrollMode.Enabled the new default. I am preserving the old code with #ifdef USE_SCROLLMODE_AUTO since the Auto value may be re-introduced later. * Adding telemetry for XLayout types (#169) * Adding telemetry for XLayout types * Update localization workflow for breaking changes (#162) * Adding instrumentation for AcrylicBrush.TintLuminosityOpacity propert… (#149) * Adding instrumentation for AcrylicBrush.TintLuminosityOpacity property change * Changing telemetry logging of property change as a member instead of class * Oops. Forgot to remove the old id. * Adding telemetry for XLayout types * Undoing noisy merge * Undoing another noisy merge * Changes for Mux Acrylic and Reveal fallback is islands (#192) * Keyboard Navigation: 'Back' button cannot be reached using tab order (#204) * Keyboard Navigation: 'Back' button cannot be reached using tab order Remove special case code that disabled tab navigation on the NavigationView back button. [Internal Issue](https://microsoft.visualstudio.com/OS/ft_xamlcon/_workitems/edit/20149845) * Remove test case that assumes tab navigation is disabled on the back button * Remove no longer used m_buttonHolderGettingFocusRevoker * Add hints to cause GUID calculations to happen during precompile (#209) * CommandBarFlyout Narrator fix (#199) * Adding FlowsTo and FlowsFrom to ensure that the primary commands are connected to the secondary commands. * Disabling new test to cut down on noise until it's fixed to work on all versions (#213) [GitHub Issue](#211) * Renaming Scroller.Child to Scroller.Content (#212) * Scroller.Child renamed to Scroller.Content. * Merging with master. * Fix an animation bug of NavigationViewList. (#188) * Fix an animation bug of NavigationViewList. * modify code to avoid /0 exception. * When in mutiselect make sure that the treeview list supports selection pattern (#210) * When in mutiselect make sure that the treeview list supports selection pattern * Update Helix SDK version (#215) This should improve PR validation from forks. Also publish binlog from RunTestsInHelix.proj to aid debugging. * Fix RatingControl scaling (#187) * Fix spacing * Fix margins * Update comment * Update calculations * Remove inline * Update comments * Update dev/RatingControl/RatingControl.cpp Co-Authored-By: kaiguo <[email protected]> * Remove margin changes in markup * Fix crash when scrolling nested ItemsRepeaters (#152) * fix add pin bug * update fix and also fix unrealiable canbringintoviewelements test * fix unpinning case * Pane Title is completely not visible in HC mode (#177) * The pane title was being obscured by the pane toggle button. Outside of High Contrast that works fine... but in HC the button background is opaque. I could split out resources for that background to be transparent in HC... but fundamentally this is a weird layout to have the text _under_ the button. It means that any non-transparent background is actually doing the wrong thing with regards to layering on the text. So... I opted to change how this was built. The key to fixing this was to have the button support both an icon and a piece of text. Since the control already has a forked template this was pretty easy. Move the ContentPresenter to be to the right of the fixed icon and update the layout accordingly. The search button was riding on the same style as the toggle button for some reason. That wasn't really necessary, as with a couple tweaks it could use a lightly styled standard Button template. * Fix the behaviour of TreeViewItem.HorizontalContentAlignment (#203) (#218) Remove the StackPanel containing the ContentPresenter and move layout and indentation to the containing Grid. Fix #203 * Remove workaround made for a midl bug. (#121) * Fixing ScrollViewer name to turn on its tests in Helix. (#224) * Replacing Scroller and ScrollViewer's IsChildAvailableWidthConstrained/IsChildAvailableHeightConstrained with ContentOrientation (#214) * Step #1: Introduce Scroller/ScrollViewer.ContentOrientation. * Step #2: Replace IsChildAvailableWidthConstrained/IsChildAvailableHeightConstrained usage with ContentOrientation and remove them. * Updating ScrollerDynamicPage test UI. * Fixing test ValidateSupportForScrollerConfigurationChanges.
|
🎉 Handy links: |
StephenLPeters
pushed a commit
that referenced
this pull request
Feb 9, 2019
* Fix spacing * Fix margins * Update comment * Update calculations * Remove inline * Update comments * Update dev/RatingControl/RatingControl.cpp Co-Authored-By: kaiguo <[email protected]> * Remove margin changes in markup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Internal issue
When text scale changes, RatingControl's star scale animation is not following mouse cursor correctly. Also, stars and caption text are not aligned.
This is because item spacing and caption text margin are not scaled correctly together with the star glyphs. Updated the math to account for text scale changes.