Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DatePicker and TimePicker flyout visual updates #3947

Merged
merged 14 commits into from Mar 31, 2021

Conversation

jevansaks
Copy link
Member

We want to keep the accent color background for the selection bar but in order to have good contrast we need to change the foreground color of the text that's in the selection bar. There is already a resource we could theoretically override (LoopingSelectorItemForegroundSelected), but that color is only swapped in when the wheels come to rest so while scrolling it doesn't look particularly good.

So I went with a composition-based approach. There is an API to render part of the tree to a composition surface and we can use that in an effect. What I'm doing here is taking the PickerHostGrid and feeding it through CompositionVisualSurface and then into a ColorMatrixEffect. This way I can take the alpha channels of what's in the PickerHostGrid and replace the RGB channels with something else. This lets us do a color swap dynamically even during the scrolling, which gets a really smooth looking "x-ray" kind of effect.

Here it is in action:
Picker Demo

@msftbot msftbot bot added the needs-triage Issue needs to be triaged by the area owners label Jan 14, 2021
@mdtauk
Copy link
Contributor

mdtauk commented Jan 14, 2021

Can I make a couple of suggestions. The Picker box itself is 32px high, but the selected shape is 40px high.
TimePickerFlyoutPresenterHighlightHeight

@chigy has said that it will need WinUI 3.0 before this control can be updated to work with compact sizing, but is it possible to tweak the selected area to match the picker height?

If not, is it possible to adjust the reveal transition, so the selected shape animates from 32px to 40px, to make it seem smoother?
image


Do these use Acrylic like other flyouts?

Once WinUI 3.0 is out, will consideration be made for #918 #905

Also is there anything to be learned or added to the default controls, from the new Alarms & Clock UI?

image

@jevansaks
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jevansaks
Copy link
Member Author

Can I make a couple of suggestions. The Picker box itself is 32px high, but the selected shape is 40px high.
TimePickerFlyoutPresenterHighlightHeight

Now that you point it out, it does look pretty odd to have the height jump. 32px selection rect feels a lot better to me too. @chigy says there may be a separate consistency pass being done for these kinds of sizing things by @anawishnoff .

Do these use Acrylic like other flyouts?

They are supposed to. Maybe something is drawing opaque over top or there's just not a very interesting background behind. I'll check.

@mdtauk
Copy link
Contributor

mdtauk commented Jan 14, 2021

Can I make a couple of suggestions. The Picker box itself is 32px high, but the selected shape is 40px high.
TimePickerFlyoutPresenterHighlightHeight

Now that you point it out, it does look pretty odd to have the height jump. 32px selection rect feels a lot better to me too. @chigy says there may be a separate consistency pass being done for these kinds of sizing things by @anawishnoff .

The TimePickerFlyoutPresenterItemHeight/DatePickerFlyoutPresenterItemHeight is 40px, a balance between the 32px mouse target, and 40/48px touch target.

But the TimePickerFlyoutPresenterHighlightHeight/DatePickerFlyoutPresenterHighlightHeight is visual only, so it can be smaller than the item height, and not affect the functionality.

Edit: In fact it may be a hold over from the change in control sizing back from 2018
image

Do these use Acrylic like other flyouts?

They are supposed to. Maybe something is drawing opaque over top or there's just not a very interesting background behind. I'll check.

In the Gif you posted, it looks almost white #fcfcfc without noise, but it could be the colour compression in creating the Gif.

@jevansaks
Copy link
Member Author

In the Gif you posted, it looks almost white #fcfcfc without noise, but it could be the colour compression in creating the Gif.

It's because I was running in a VM where the acrylic logic forces it to opaque. If I override that then it is using acrylic, so we're good here.

@mdtauk
Copy link
Contributor

mdtauk commented Jan 14, 2021

In the Gif you posted, it looks almost white #fcfcfc without noise, but it could be the colour compression in creating the Gif.

It's because I was running in a VM where the acrylic logic forces it to opaque. If I override that then it is using acrylic, so we're good here.

Good to hear!

@ranjeshj ranjeshj added area-DateTimePickers DatePicker, TimePicker, CalendarDatePicker, CalendarView team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Jan 15, 2021
@jevansaks
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -384,31 +388,35 @@
<RowDefinition Height="*" />
<RowDefinition Height="Auto" />
</Grid.RowDefinitions>
<Grid x:Name="PickerHostGrid">
<Grid>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the new grid needed? I think we could avoid it by setting column span on the HighlightRect to 5

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HighlightRect needs to draw over the PickerGrid because it's actually drawing opaque content to obscure what the PickerGrid drew with the inverted colors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Children draw over there parents and their older siblings though, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but the Panels that host the children are conspicuously absent from the template because they are Append()-ed from code behind in the picker code. :( So no matter what in that single grid you can't draw over those panels from the template.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but the Panels that host the children are conspicuously absent from the template because they are Append()-ed from code behind in the picker code. :( So no matter what in that single grid you can't draw over those panels from the template.

Is that something that could be changed with WinUI 3 or do you think it would very hard to change the behavior here? Having controls appending elements in their code behind does not sound ideal, especially for templated controls so it might be good to change this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw mention of Acrylic Accent Colour brushes, could on of those be used for the selection rectangle?

_effectFactory = nullptr;
}

auto compositor = winrt::Window::Current().Compositor();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compositor [](start = 13, length = 10)

nit: const

@@ -221,7 +221,10 @@
<!-- Dependencies for ImageIcon -->
<PropertyGroup Condition="Exists('InnerLoopAreas.props') And $(SolutionName) == 'MUXControlsInnerLoop' And $(FeatureImageIconEnabled) == 'true'">
</PropertyGroup>
<!-- Dependencies for AnimatedIcon -->
<!-- Dependencies for ColorFilterOverlayControl -->
<PropertyGroup Condition="Exists('InnerLoopAreas.props') And $(SolutionName) == 'MUXControlsInnerLoop' And $(FeatureColorFilterOverlayControlEnabled) == 'true'">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the inner loop feature areas need to be updated as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you be more specific about what is missing or what needs to change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to add this feature to the DatePicker group in line 73 since we need to build this project if want the styles for DatePicker to work as expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats right, DatePicker should not be dependent on ColorFilterOverlayControl so this PropertyGroup needs to add that as a ProductOnly dependency.

almedina-ms and others added 5 commits March 23, 2021 13:12
* Lift datepicker themes

* Fix margin but LoopingSelector style is not respected

* Update DatePicker styles to match design

* Fix colors in control

* Update Highlight color

* Fix Highligh color for TimePicker dark mode

* Fix some colors for dark theme

* Fix highlight color on TimePicker, corner radius on highlighted date and dark mode highlighted foreground

* Fix margins for accept/dismiss buttons

* Fix up/down buttons margin

* Fix color keys and date/time pickers borders

* Fix everything but the plaveholder font color

* Fix focus state for placeholders

* Format all files

* Remove extra variables

* Fix crash for pre 21h1 builds

* Activate DatePicker as test

* Fix border brushes and update timepicker to match datepicker

* Remove extra border on focus + pointer

* Remove accent background color for focus state

* Fix missing reference to caret icons

* Remove left over changes from old focus behaviour

* Remove changes from old focus behaviour for TimePicker

* Fix contrast missing key

* Remove added but unused colors

* Remove another unused resource
@almedina-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@almedina-ms almedina-ms merged commit 7b85ba5 into master Mar 31, 2021
@almedina-ms almedina-ms deleted the user/jevansaks/datepickerhighlightrect branch March 31, 2021 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-DateTimePickers DatePicker, TimePicker, CalendarDatePicker, CalendarView team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants