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

Using setter for TextBlock.Text property breaks some other TextBlocks #7982

Closed
grayed opened this issue Apr 14, 2022 · 19 comments · Fixed by #10617
Closed

Using setter for TextBlock.Text property breaks some other TextBlocks #7982

grayed opened this issue Apr 14, 2022 · 19 comments · Fixed by #10617
Labels
by-design The behavior reported in the issue is actually correct.

Comments

@grayed
Copy link

grayed commented Apr 14, 2022

Describe the bug
When I define a Style for TextBlock with Setter for Text property, it removes (not even replaces) text on some other TextBlock controls.

To Reproduce
Minimal XAML example I've came up with:

<Window xmlns="https://github.com/avaloniaui"
		xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
		xmlns:vm="using:TextBlockTextSetterBug.ViewModels"
		xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
		xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
		mc:Ignorable="d" d:DesignWidth="800" d:DesignHeight="450"
		x:Class="TextBlockTextSetterBug.Views.MainWindow"
		Icon="/Assets/avalonia-logo.ico"
		Title="TextBlockTextSetterBug">
	<Window.Styles>
		<Style Selector="TextBlock.myautotext">
			<Setter Property="Text" Value="Text from Style" />
		</Style>
	</Window.Styles>

	<Design.Height>200</Design.Height>
	<Design.Width>300</Design.Width>
	<Design.DataContext>
		<vm:MainWindowViewModel/>
	</Design.DataContext>

	<StackPanel>
		<!-- This text IS visible (style works as expected) -->
		<TextBlock Classes="myautotext" />
		
		<!-- This text IS visible (bindings outside DataTemplate are working) -->
		<TextBlock Text="{Binding Greeting}" />
	
		<!-- This text is NOT visible (direct setting of text doesn't work) -->
		<TextBlock Text="Can't see me" />
	
		<!-- Text of items is NOT visible (using explicit DataTemplate changes nothing) -->
		<ListBox Items="{Binding SomeStrings}"/>
	</StackPanel>
</Window>

Expected behavior
TextBlock controls not matching the selector must not be affected.

Screenshots
Expected:
expected

Actual:
actual

Desktop

  • OS: Windows 10
  • Avalonia 0.10.13 (latest from NuGet)

Additional context
Add any other context about the problem here.

@grayed grayed added the bug label Apr 14, 2022
@maxkatz6 maxkatz6 removed the bug label Apr 14, 2022
@maxkatz6
Copy link
Member

Text is a direct property and not styled property. It's not supposed to be used in styles.
We should generate a compile time error here.

@grayed
Copy link
Author

grayed commented Apr 15, 2022

Thank you for the answer.

Does it mean there's no (clean) way to apply the same text to a bunch of TextBlock or Label controls using a style?

@maxkatz6
Copy link
Member

Think about properties as it can be either styleable or data. Text is a data property and isn't styleable by itself, while foreground, font size and similar are.
Similarly, Button.Command is not styleable. ListBox.Items too.
At least it's the way how it was designed.

Does it mean there's no (clean) way to apply the same text to a bunch of TextBlock or Label controls using a style?

At once - no. You can bind them to the same property though, as you can in WPF.

@grayed
Copy link
Author

grayed commented Apr 15, 2022

Well, it's hard to distinguish between "data" and "style" sometimes. In my case, I have many TextBlock controls containng the same small text (like "mm" or "Ohm") near other objects. Since I already have a style for them, setting font, position etc., it was logical to set the text from style, too, just to make XAML code look clearer.

Having to write the text is not an issue, and there's no reason to complicate things with bindings.

Thank you again for your time. I'm keeping this issue open until the compile time error you mentioned comes in then. :-)

@robloo
Copy link
Contributor

robloo commented Apr 15, 2022

TextBlock.Text absolutely should be styleable. @maxkatz6 This can be done in every-single other XAML framework, correct?

Edit: Here is WPF source

https://github.com/dotnet/wpf/blob/fdf613d73e468e558d263faf4a055edbdff803f9/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/TextBlock.cs#L586-L610

It definitely can be used like any other property. Unless I'm missing something so obvious I never thought about it before, I would expect the above XAML to work.

You might be able to argue with a TextBox Text should be direct and not styleable (some like me would disagree) but that makes less sense for a TextBlock which is read-only and not an input control.

@grokys
Copy link
Member

grokys commented Apr 15, 2022

The reason is kind of lost in the mists of time, but I think it was done for performance reasons. Reading a CLR (i.e. direct) property is about 40x faster currently than reading a styled property. That combined with:

  • It will almost always be set
  • The need to style it is uncommon
  • TextBlocks are very common and there will likely be a lot of them
  • In CSS you can't style element contents

I think lead us to decide that it was worth sacrificing some flexibility for performance.

@robloo
Copy link
Contributor

robloo commented Apr 15, 2022

@grokys Hm, in my opinion this feature shouldn't be sacrificed. It is something I would expect to just work and does in all other XAML frameworks. That said, I don't know the performance issues encountered at the time, or if they are still valid. I think this should also be documented some place along with why it was designed this way.

If the styling system is really this slow, and this is still a problem enough to keep Text a direct property, I guess there are more fundamental concerns that should be addressed. WPF can handle this type of thing just fine and it's performance was well enough -- obviously it never ran on all the form factors Avalonia does though.

Overall, this is pretty unintuitive and has/will cause a lot of tricky debugging. I definitely think compile warnings are needed but I think this should just be a StyledProperty for TextBlock (leave TextBox as-is). Optimizations have since been made to the styling system over the years as well.

Finally, are we sure there isn't more going on?:

		<!-- This text is NOT visible (direct setting of text doesn't work) -->
		<TextBlock Text="Can't see me" />

There is no way this should ever not work. I guess it is related to the other issue of priority that might be fixed after your PR.

@grayed
Copy link
Author

grayed commented Apr 17, 2022

  • In CSS you can't style element contents

Wrong. See, e.g.:

foo:before {
  content: "In a galaxy far, far away... ";
}
foo:after {
  content: "That's all, folks!";
}

See the definition of content CSS property for more details. While technically it doesn't allow to set actual content, it at least allows what I wanted to do initially. :)

@robloo
Copy link
Contributor

robloo commented Apr 17, 2022

Since TextBlock.Text ONLY supports strings and not general-purpose content, and its also not an input like TextBox.Text. I have to think this isn't really a "Content" property -- at least its a special case. I realize that technically speaking it is marked as a content property though for XAML purposes.

@Gillibald
Copy link
Contributor

Technically TextBlock will be able to host everything that is of type String,Inline and IControl

@maxkatz6 maxkatz6 added the by-design The behavior reported in the issue is actually correct. label May 25, 2022
@maxkatz6
Copy link
Member

@Gillibald should we make TextBlock.Text a styled property without inlines support and instead add RichTextBlock with direct property that also can work with inlines?

@Gillibald
Copy link
Contributor

Yes. I think that is the best solution.

@robloo
Copy link
Contributor

robloo commented Jun 1, 2022

While I have no idea how you already support rich text and inlines in a property declared as type string, I actually don't think this should change. It should function like WPF where you absolutely can do inclines and complex text formatting directly as TextBlock content.

@Gillibald
Copy link
Contributor

This is purely for optimization. TextBlock has an InlinesProperty marked as Content and a TextProperty that can be used to add text to current text layout. Introducing a RichTextBlock allows us to have some performance hits. We could support text selection etc. A regular TextBlock is unformatted most of the time so it makes sense to optimize the primitive in that way.

@robloo
Copy link
Contributor

robloo commented Jun 1, 2022

@Gillibald That does make some sense. However, I think you are now making the difference between Label and TextBlock even more of a grey area. I think the performance impact of supporting inlines in TextBlock is negligible as well. If the content value is a string (a very simple check) it can bypass all inlines processing.

Bottom line, I'm not sure we need Label, TextBlock and now RichTextBlock. Plus we are breaking WPF compatibility which is somewhat bad in fundamental cases like this. However, RichTextBlock does conceptually match with a future RichTextBox as well.

@maxkatz6
Copy link
Member

maxkatz6 commented Jun 1, 2022

I'm not sure we need Label, TextBlock and now RichTextBlock

We will need RichTextBlock pretty much anyway. At least for text selection. WPF TextBlock doesn't have that. And in UWP only RichTextBlock has text selection, but not TextBlock.
UWP also has one nice feature - OverflowContentTarget, which I personally liked for some simple scenarios with overflow.

And about inlines. In the stable branch we don't have inlines, so it won't be a breaking change anyway. Not a huge difference for developers coming from WPF either. But it still will cause some confusion, you are right.

Though, I don't really know much details about perf-related reasoning, so I won't argue on that too much. And if we can get styled property with inlines or not.

@robloo
Copy link
Contributor

robloo commented Jun 2, 2022

Well, text selection could be added pretty easily (at least in API) to TextBlock with a new enable selection property. However, I see some appeal in separating the advanced formatting out like RichTextBox has historically. I just:

  1. Caution against removing functionality that is supported in WPF like <bold /> and <Italic/> which are used sometimes. The UWP docs have a good comparison https://docs.microsoft.com/en-us/windows/apps/design/controls/rich-text-block#is-this-the-right-control.
  2. Label vs. TextBlock should also probably be revisited too and a Target property could be added to get rid of Label entirely... there isn't much other difference now.

Anyway, I don't have very strong feelings about this so if you all think it's a good idea I won't put up any more resistance. I'll get used to the cleaner separation of concerns with plain text in TextBlock and formatted text in RichTextBlock sooner rather than later.

@robloo
Copy link
Contributor

robloo commented Jan 25, 2023

@maxkatz6 Why was this closed? It seemed to be OK according to @Gillibald comments who only cautioned against changing TextBox.Text from direct to styled?

@maxkatz6
Copy link
Member

@robloo I am not against of this change, aside from technical challenges with validation which is important for this property.
But I believe we could have single issue as an umbrella for this kind of problems - #9944.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
by-design The behavior reported in the issue is actually correct.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants