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

TextBox/RichEditBox Shouldn't Force Light Theme on Focus #142

Closed
adamhewitt627 opened this issue Jan 3, 2019 · 23 comments · Fixed by #2046
Closed

TextBox/RichEditBox Shouldn't Force Light Theme on Focus #142

adamhewitt627 opened this issue Jan 3, 2019 · 23 comments · Fixed by #2046
Assignees
Labels
area-Styling area-TextBox TextBox, RichEditBox feature proposal New feature proposal

Comments

@adamhewitt627
Copy link

Describe the bug
Both TextBox and RichEditText include a visual state like this:

<VisualState x:Name="Focused">
    <ObjectAnimationUsingKeyFrames Storyboard.TargetName="ContentElement" Storyboard.TargetProperty="RequestedTheme">
        <DiscreteObjectKeyFrame KeyTime="0" Value="Light" />
    </ObjectAnimationUsingKeyFrames>
</VisualState>

This interferes with lightweight styling, most notably when trying to theme the editor to remain dark in a dark theme.

Steps to reproduce the bug

  1. New UWP Application
  2. Use this XAML for the main page (modify the x:Class to match your project)
<Page x:Class="App6.MainPage"
      xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
      xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
      Background="{ThemeResource ApplicationPageBackgroundThemeBrush}"
      RequestedTheme="Dark">

    <Grid>
        <Grid.Resources>
            <SolidColorBrush x:Key="TextControlBackground" Color="Transparent" />
            <SolidColorBrush x:Key="TextControlBackgroundPointerOver" Color="Transparent" />
            <SolidColorBrush x:Key="TextControlBackgroundFocused" Color="{ThemeResource SystemAltHighColor}" />
            <SolidColorBrush x:Key="TextControlForegoundFocused" Color="{ThemeResource SystemBaseHighColor}" />
        </Grid.Resources>
        
        <TextBox Text="Here is some text that I want to edit in a dark editor" FontSize="30"
                 TextWrapping="Wrap" />
    </Grid>
</Page>

Expected behavior
I expect the TextBox to use SystemBaseHighColor from the dark theme, but it resolves to the Light theme instead. Even worse, TextControlBackgroundFocused resolves to dark because its theme isn't changed in the Style.

Screenshots
Screenshot showing the dark text (with some text selected in order to show that text exists)
image

@YuliKl
Copy link

YuliKl commented Jan 3, 2019

Agreed that these controls don't do a great job of supporting lightweight styling and require re-templating for app authors to make modifications to their appearance. This issue is similar to an internal bug I had opened.

@YuliKl YuliKl added the feature proposal New feature proposal label Jan 4, 2019
@mdtauk
Copy link
Contributor

mdtauk commented Jan 15, 2019

You could add properties/brushes for
TextFieldFocusedBackground
TextFieldFocusedBorder
TextFieldFocusedForeground
TextFieldFocusedSelectedBackground
TextFieldFocusedSelectedForeground

If the Fluent teams still believe Dark Theme needs to have Dark text on Light background, then this would be applied by those brushes, and can be easily overridden in XAML Styles by developers.

@marb2000 marb2000 added the area-TextBox TextBox, RichEditBox label Feb 13, 2019
@lukeblevins
Copy link
Contributor

I think by default, user input controls like TextBox should not force light theme when dark mode is enabled. In many apps, it looks jarring and quite inconsistent for this control to light up white when focus is invoked.

@robloo
Copy link
Contributor

robloo commented Aug 15, 2019

I seem to recall reading some place a few years ago that this limitation has something to do with the caret itself. The caret color does not respect the foreground brush and therefore cannot change from the system 'black'. This is why when editing text everything switches to light mode.

If that is true, I think it's time to get this fixed. WPF caret color followed the foreground brush and it should do so in UWP as well.

Bottom line I agree that dark mode text input needs to keep the dark mode look -- dark text box background with light text and caret. Switching to light theme for text input is really a strange design decision.

Edit: I should also add that WinUI 3 is likely the best opportunity to make a change like this. Everyone is going to be going through and making updates at least to namespaces so some breaking changes are to be expected. Not sure when such an opportunity will present itself again.

@mdtauk
Copy link
Contributor

mdtauk commented Aug 15, 2019

Better than making the Caret use the Foreground colour, it should be configurable perhaps as a property on all Text Boxes and selectable Text Blocks.
<TextBlock CaretColor="{ThemeResources TextCaretColorBrush}" />

By way of an example...
image

@marcelwgn
Copy link
Collaborator

I totally agree that the TextBox and RichEditBox should not force light theme on focus.

In addition to breaking the overall design of an app and disturbing the overall look of an app, having it "switch" themes introduces the following problem:

When we have a RichEditBox with text that is created with RTF, changing the color of those letters will not be restored when the user leaves and reenters the RichEditBox.
So what this behavior does is overwrite the styling of RTF text, which is definitely not desired behavior when enabling the user to style the text.
See this and this pull request for workarounds to this behavior.

tl;dr: The current behavior of TextBox and RichEditBox does not blend in very well and is counterproductive with RTF text as it overwrites styling made by user if the developer does not provide (a lengthy) workaround.
I would love to have those components not switch to light theme when they receive focus.

@shaheedmalik
Copy link

The switch from dark mode to light is really jarring.

image

@jevansaks jevansaks added area-Framework needs-triage Issue needs to be triaged by the area owners labels Nov 7, 2019
@robloo
Copy link
Contributor

robloo commented Jan 11, 2020

Any updates on this? It would be a great one to do with WinUI 3.0

@chrisglein
Copy link
Member

No updates yet. This is one that I don't believe requires WinUI 3.0 (it seems like the default template could be changed to work better with the lightweight styling, and many of those templates are already in WinUI 2.x). But timing-wise for us it does, as our hands are quite busy with WinUI 3.0.

Edit: I should also add that WinUI 3 is likely the best opportunity to make a change like this. Everyone is going to be going through and making updates at least to namespaces so some breaking changes are to be expected. Not sure when such an opportunity will present itself again.

The goal with WinUI 3.0 is to have the fewest possible breaking changes possible, so that the move from inbox XAML to WinUI 3.0 is smooth. Later we can explore breaking changes with future versions, but we want to avoid the slippery slop of bundling breaking changes with that first version udpate.

@chrisglein chrisglein added area-Styling and removed needs-triage Issue needs to be triaged by the area owners labels Jan 24, 2020
@marcelwgn
Copy link
Collaborator

@jevansaks , @ranjeshj , @chrisglein If this an change that the WinUI team approves, I would be happy to try to make a PR for this!

@robloo
Copy link
Contributor

robloo commented Feb 1, 2020

@chingucoding

I mentioned earlier:

I seem to recall reading some place a few years ago that this limitation has something to do with the caret itself. The caret color does not respect the foreground brush and therefore cannot change from the system 'black'. This is why when editing text everything switches to light mode.

Have you checked this? If the caret color cannot be changed you won't be able to fix this with styling.

@marcelwgn
Copy link
Collaborator

Have you checked this? If the caret color cannot be changed you won't be able to fix this with styling.

Yes this is a change we can do in WinUI 2. The following gif was made by modifying WinUI 2 source code (please ignore the unchanged button):

TextBox in dark theme

@YuliKl
Copy link

YuliKl commented Feb 2, 2020

Neat 😎 Would love to get feedback from @chigy about the change @chingucoding is proposing above.

@chigy
Copy link
Member

chigy commented Feb 3, 2020

@YuliKl and @chingucoding ,
Thank you for your idea. I will check with the design to see if we want this design instead. Originally, there was a concern about usability to go with complete dark because dark theme was relatively new concept when this was introduced, but looks like Fabric is doing this and more people are used to dark theme.

@chigy
Copy link
Member

chigy commented Feb 3, 2020

@YuliKl and @chingucoding ,
I remembered another reason why we have the current design with white...

We typically recommend using dark theme controls over images so the control appears more clearer over multiple colors, etc. This will mean that the text field over image would be black if that guidance would have been followed. Something that need to be thought through.

@chingucoding , would you mind creating some prototype gif where this is done over image?

@marcelwgn
Copy link
Collaborator

@chingucoding , would you mind creating some prototype gif where this is done over image?

Sure!

darkthemetextbox

@marcelwgn
Copy link
Collaborator

marcelwgn commented Feb 24, 2020

@chigy Have you had time to check with design whether we want the proposed design instead of the current? 😅

Edit: Also there currently are multiple Microsoft apps using a retemplated version with dark background on hover.

@chigy
Copy link
Member

chigy commented Feb 26, 2020

@chingucoding , yes as a matter of fact.
@YuliKl , we can consider this change for the controls update for the future.

That said, please don't add the vertical line you added. That's not part of the design.

@marcelwgn
Copy link
Collaborator

Thanks for the update @chigy !

Is this proposal a change the WinUI team would like to do now, or something that should be done later?

If this is something that would be fine to do now, I would love to implement this!

@YuliKl
Copy link

YuliKl commented Feb 28, 2020

@chingucoding please go ahead and fix this soon 😀

@mdtauk
Copy link
Contributor

mdtauk commented Feb 29, 2020

@chingucoding , would you mind creating some prototype gif where this is done over image?

Sure!

darkthemetextbox

The Rest and Hover states for the TextBox uses semi-transparent backgrounds, with it going 100% opaque on Focus/Typing states

@marcelwgn
Copy link
Collaborator

Yes @mdtauk , good catch! The preview was just to demonstrate that it was possible, and how a dark caret would look on an image. The opaqueness of the "unfocused" will (of course) not change.

@msft-github-bot
Copy link
Collaborator

🎉This issue was addressed in #2046, which has now been successfully released as Microsoft.UI.Xaml v2.4.0-prerelease.200422001.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Styling area-TextBox TextBox, RichEditBox feature proposal New feature proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.