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

NumberBox: Background of the SpinButton Popup cannot be distinguished from a default page background in Dark theme #2843

Closed
Felix-Dev opened this issue Jul 6, 2020 · 26 comments · Fixed by #3130
Labels
accessibility Narrator, keyboarding, UIA, etc area-NumberBox NumberBox Control help wanted Issue ideal for external contributors team-Controls Issue for the Controls team

Comments

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Jul 6, 2020

Describe the bug
The background of the NumberBox SpinButton Popup cannot be distinguished from a default page background in Dark theme.

Steps to reproduce the bug

  1. Open the XAML Controls Gallery.
  2. Go to the NumberBox page and the specific NumberBox SpinButton example.
  3. Make sure the NumberBox is in compact SpinButtonPlacementMode and focus the NumberBox.

image

Expected behavior
Since the Popup is a transient UI like the ComboBox's dropdown menu or the MenuBarItem flyout menu, it should have the same background like these, as per the acrylic guidance:

ComboBox NumberBox
image image

NuGet package version:
Current WinUI master builds.

Windows 10 version Saw the problem?
Insider Build (xxxxx)
May 2020 Update (19041) Yes
November 2019 Update (18363)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop Yes
Xbox
Surface Hub
IoT

Additional Context

If I were to work on a PR for this I would likely want to tie this into #2844 as I would create a dedicated theme resource here for the Popup background.

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Jul 6, 2020
@StephenLPeters StephenLPeters added accessibility Narrator, keyboarding, UIA, etc area-NumberBox NumberBox Control team-Controls Issue for the Controls team help wanted Issue ideal for external contributors needs-cherrypicktorelease PR tagged for cherry-pick to the current release branch (but not yet picked) and removed needs-triage Issue needs to be triaged by the area owners labels Jul 6, 2020
@StephenLPeters
Copy link
Contributor

@chigy can you weigh in on the appropriate color for this? @SavoySchuler FYI

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Jul 7, 2020

For reference, I looked at the ComboBox theme resource file and just copied them over to the NumberBox:

<contract7NotPresent:StaticResource x:Key="ComboBoxDropDownBackground" ResourceKey="SystemControlBackgroundChromeMediumLowBrush" />

Here are the resulting NumberBox theme resources then (Dark theme example)

<contract7NotPresent:StaticResource x:Key="NumberBoxPopupBackground" ResourceKey="SystemControlBackgroundChromeMediumLowBrush" />
<contract7Present:StaticResource x:Key="NumberBoxPopupBackground" ResourceKey="SystemControlTransientBackgroundBrush" />
<contract7NotPresent:StaticResource x:Key="NumberBoxPopupBorderBrush" ResourceKey="SystemControlForegroundChromeHighBrush" />
<contract7Present:StaticResource x:Key="NumberBoxPopupBorderBrush" ResourceKey="SystemControlTransientBorderBrush" />

where SystemControlTransientBackgroundBrush maps to

<AcrylicBrush x:Key="SystemControlTransientBackgroundBrush" BackgroundSource="HostBackdrop" TintColor="{StaticResource SystemChromeAltHighColor}" TintOpacity="0.8" FallbackColor="{StaticResource SystemChromeMediumLowColor}" />

and SystemControlTransientBorderBrush to

<SolidColorBrush x:Key="SystemControlTransientBorderBrush" Color="#000000" Opacity="0.36" />

in Dark theme.

Theme Current Updated - 1809+ (contract 7 present) Updated - 1803 and less (contract 7 not present)
Light image image image
Dark image image image

I used the ComboBox dropdown theme resources here, as said above. I picked them for some initial experimenting here due to the similarity as both are a popup/flyout and thus their background appearance should probably match.

@mdtauk
Copy link
Contributor

mdtauk commented Jul 7, 2020

The shadow does not show up well with the pure black of the Dark Theme - so if this were to use Acrylic and borders, it would improve the experience.

@chigy
Copy link
Member

chigy commented Jul 7, 2020

@kikisaints , I agree with the suggestion but I want to make sure acrylic does not cause problem with this small surface. Kiki, is the size of this popup big enough not to cause the problem we had with Tooltip that made us decide not to use acrylic there?

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Jul 7, 2020

@chigy @kikisaints As a helper, here's how the acrylic brush looks with colored background given this design:
image

Light Theme Dark Theme
image image

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Aug 6, 2020

@chigy Do you have an update on this?

I checked the CommandBarFlyout control and that is also using an in-app background for its primary command surface:
image

(And while that CommandBarFlyout has for buttons in the pic above for illustration purposes, it could also have less, such as 2-3 so it would also represent a small surface like the NumberBox spin buttons popup here.)

@chigy
Copy link
Member

chigy commented Aug 7, 2020

@Felix-Dev , I was waiting for @kikisaints for her response but she is on vacation at the moment.
Her respond pending, the right design style that match our guidance and existing control would to do acrylic and do 1px border which matches the color that we use on controls like MenuFlyout.

@Felix-Dev
Copy link
Contributor Author

@chigy Thanks for the update. I will wait for @kikisaints then to give her view on this.

@kikisaints
Copy link

I believe less than 32x32 on both axes was the issue. If this new suggestion is bigger than 32 on either axis, it should be fine.

It's also not that we can't do it, it's more that something that small is spending a lot of cycles/performance to render something that's virtually undetectable. So if we feel this acrylic surface (which is in-line with the guidance by being a popup) is actually noticeable as an acrylic surface than I'm all for it.

@Felix-Dev
Copy link
Contributor Author

@kikisaints The popup spin buttons are currently of size 40x32 (WidthxHeight):

<Style x:Name="NumberBoxPopupSpinButtonStyle" TargetType="RepeatButton">

I will hopefully be able today to create another screenshot showing how the design will look with the guidance given by @chigy.

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Aug 14, 2020

Her respond pending, the right design style that match our guidance and existing control would to do acrylic and do 1px border which matches the color that we use on controls like MenuFlyout.

@kikisaints @chigy For demo purposes only I modified the NumberBox to show three SpinButton popups next to each other. The MenuFlyout and ComboBox styles use the SystemControlTransientBackgroundBrush theme resource for their transient UIs. The CommandBarFlyout style uses the SystemControlAcrylicElementBrush. So we have three transient UIs but the CommandBarFlyout uses a different background brush for its transient UI.

The SystemControlTransientBackgroundBrush is defined like this for dark mode in the Windows 1903 SDK's Generic.xml file:

<AcrylicBrush x:Key="SystemControlTransientBackgroundBrush" BackgroundSource="HostBackdrop" TintColor="{StaticResource SystemChromeAltHighColor}" TintOpacity="0.8" FallbackColor="{StaticResource SystemChromeMediumLowColor}" />

whereas SystemControlAcrylicElementBrush is defined like this:

<AcrylicBrush x:Key="SystemControlAcrylicElementBrush" BackgroundSource="Backdrop" TintColor="{StaticResource SystemChromeAltHighColor}" TintOpacity="0.8" FallbackColor="{StaticResource SystemChromeMediumColor}" />

As you can see, one major difference between these two brushes is that SystemControlTransientBackgroundBrush uses a background source of "HostBackdrop" and SystemControlAcrylicElementBrush explicitly uses in-app backgroudn acrylic with "Backdrop" as the background source. The fallback color also differs between those two brushes.

Below I've compared both brushes and how they would look like for the NumberBox spin buttons popup:

Theme SystemControlTransientBackgroundBrush SystemControlAcrylicElementBrush
Light image image
Dark image image

As you can hopefully see there is a slight visual difference here depending on which brush I am using.

I've also made a comparison without between these two brushes without any fancy modifications to the NumberBox or specially colored backgrounds to show the color contrast with the default page backgrounds:

Theme SystemControlTransientBackgroundBrush SystemControlAcrylicElementBrush
Light image image
Dark image image

Which brush should I use here?

@mdtauk
Copy link
Contributor

mdtauk commented Aug 14, 2020

@Felix-Dev the border in the Dark Theme, could do with being lighter, to help the buttons pop from the dark background

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Aug 14, 2020

@mdtauk The border I am currently using here is the same one used for the MenuFlyout and CommandBarFlyout - SystemControlTransientBorderBrush:

image
(A CommandBarFlyout shown on the default page background in dark theme)

As the SystemControlTransientBorderBrush is slightly visible in Light theme but not in Dark theme, we could update that theme resource for the dark theme to make the border slightly visible here as well. The new borderbrush would then be reflected across the board for the different popups and flyouts used in WinUI.

@chigy @kikisaints FYI

@mdtauk
Copy link
Contributor

mdtauk commented Aug 14, 2020

Maybe the top edge highlight should be used for these controls, as with 10X

image

@chigy
Copy link
Member

chigy commented Aug 14, 2020

@mdtauk , @Felix-Dev ,
We use the same brush for the borders across all of our controls. Please stick with the same value.

@mdtauk
Copy link
Contributor

mdtauk commented Aug 14, 2020

@mdtauk , @Felix-Dev ,
We use the same brush for the borders across all of our controls. Please stick with the same value.

That makes sense, however it is still harder to see the Popup Buttons in the Dark Theme, as it uses a dark border and not a lighter one.

@Felix-Dev
Copy link
Contributor Author

@chigy I believe @mdtauk's suggestion here is to look at the borderbrush used in dark theme (SystemControlTransientBorderBrush) for transient controls and modify it slighty for better visibility. For example, see the color of the SystemControlTransientBorderBrush in Light theme: You can clearly see the border of the transient control. That's not the case in dark theme for now. The suggestion here is to look at the SystemControlTransientBorderBrush value in dark mode and perhaps update it for better border visibility.

Modifying this brush would not break consistency within WinUI as I believe all transient UIs use that theme resource as their borderbrush.

@chigy
Copy link
Member

chigy commented Aug 14, 2020

@mdtauk , @Felix-Dev ,
What I am asking is please don't design new behavior for this control. If you are suggesting to update the border, then please open a separate new GitHub item and address it for all controls. We do not want to introduce one-off.

@Felix-Dev
Copy link
Contributor Author

@chigy It seems you have missunderstood me. I was not planning to update the border just for the NumberBox control here.

@mdtauk
Copy link
Contributor

mdtauk commented Aug 14, 2020

image
image

It seems to me that if the border is there to improve visibility, the dark theme should use a lighter colour

image

@Felix-Dev
Copy link
Contributor Author

@mdtauk Can we move updating the borderbrush for transient UIs in dark mode into its separate issue? It's not a NumberBox spin buttons popup specific issue (though if you plan to open a tracking issue for this you can reference the NumberBox as an example).

@mdtauk
Copy link
Contributor

mdtauk commented Aug 14, 2020

Feel free to use the images when setting up a new issue.

@chigy
Copy link
Member

chigy commented Aug 14, 2020

@chigy It seems you have missunderstood me. I was not planning to update the border just for the NumberBox control here.

@Felix-Dev , I understood you perfectly.
I am asking, please do not introduce a change that impacts all of the controls through an issue that is specifically about NumberBox. Please open a separate one and we will discuss it separately.

@Felix-Dev
Copy link
Contributor Author

@chigy Ok, then it seems i have missunderstood you :)

Anyway, rest assured, I wasn't thinking about doing that here since that would be an update much broader in scope than just the NumberBox. This will be tracked in its own issue which with @mdtauk's consent I would open later today or tomorrow.

Back to the NumberBox specific issue here: Do you have an opinion on whether I should use the SystemControlTransientBorderBrush theme resource or the SystemControlAcrylicElementBrush theme resource?

@chigy
Copy link
Member

chigy commented Aug 14, 2020

@Felix-Dev , I don't know the specific brushes used but please match the color exactly with the MenuFlyout.
image

@Felix-Dev
Copy link
Contributor Author

@chigy Alright, will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Narrator, keyboarding, UIA, etc area-NumberBox NumberBox Control help wanted Issue ideal for external contributors team-Controls Issue for the Controls team
Projects
None yet
7 participants