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

ComboBox Empty Selection should not Generate a TextBlock as SelectionBoxItem #16748

Merged
merged 5 commits into from
Oct 7, 2024

Conversation

gehongyan
Copy link
Contributor

@gehongyan gehongyan commented Aug 21, 2024

What does the pull request do?

Fixes #16747

As demonstrated in issue #16747, for a ComboBox, when specifying PlaceholderText and DisplayMemberBinding simultaneously, the placeholder text always disappears, regardless of whether there is a selected item or not.

This issue was introduced by #11227.

What is the current behavior?

When updating the selection box item in a ComboBox, where a DisplayMemberBinding is set leaving ItemTemplate and SelectionBoxItemTemplate null, a TextBlock will be generated as the SelectionBoxItem:

if(ItemTemplate is null && SelectionBoxItemTemplate is null && DisplayMemberBinding is { } binding)
{
var template = new FuncDataTemplate<object?>((_, _) =>
new TextBlock
{
[TextBlock.DataContextProperty] = item,
[!TextBlock.TextProperty] = binding,
});
var text = template.Build(item);
SelectionBoxItem = text;
}

The code above will always generate a non-null object of TextBlock assigned to SelectionBoxItem, which causes the ObjectConverters.IsNull converter on IsVisible of the PlaceholderTextBlock in the control template of ComboBox always returns false, hence, the issue.

<TextBlock x:Name="PlaceholderTextBlock"
Grid.Column="0"
HorizontalAlignment="{TemplateBinding HorizontalContentAlignment}"
VerticalAlignment="{TemplateBinding VerticalContentAlignment}"
Margin="{TemplateBinding Padding}"
Text="{TemplateBinding PlaceholderText}"
Foreground="{TemplateBinding PlaceholderForeground}"
IsVisible="{TemplateBinding SelectionBoxItem, Converter={x:Static ObjectConverters.IsNull}}" />

image

What is the updated/expected behavior with this PR?

The placeholder should always display when none are selected.

image

How was the solution implemented (if it's not obvious)?

The SelectionBoxItem should be set to null (the argument item passed to UpdateSelectionBoxItem in fact, but it will be a null under this circumstance), to allow the ObjectConverters.IsNull converter to succeed in returning true to let the PlaceholderTextBlock display.

Checklist

Breaking changes

Since 11.0.0-preview8, for a ComboBox, if the DisplayMemberBinding is set and ItemTemplate and SelectionBoxItemTemplate are left null, the SelectionBoxItem will always return a TextBlock, whose Text will be the selected member, or a null inside a TextBlock when none are selected.

Since this PR, in the same scenario, when none are selected, the SelectionBoxItem will return null (the passed-in argument item itself) instead.

Obsoletions / Deprecations

None.

Fixed issues

Fixes #16747

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0051412-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented Aug 21, 2024

  • All contributors have signed the CLA.

@gehongyan
Copy link
Contributor Author

@cla-avalonia agree

@rabbitism
Copy link
Contributor

Should we expose an :selection-empty like pseudo class for SelectingItemsControl? it should be very helpful.

@gehongyan
Copy link
Contributor Author

Should we expose an :selection-empty like pseudo class for SelectingItemsControl? it should be very helpful.

I am not sure. If so, I may implement it under this PR or a new one.

@MrJul MrJul added the bug label Oct 3, 2024
@MrJul
Copy link
Member

MrJul commented Oct 3, 2024

Good catch! it's likely an oversight when DisplayMemberBinding support was implemented in #11227.

The issue reproduces only when the selection has been set to a valid item before, so a unit test would be nice.

I wanted to ask you to write that test, but I've already done so while checking the behavior of this PR.
(For some reason I could push the master branch merge to your branch, but not another commit. )
Can you please add it to ComboBoxTests?

[Fact]
public void DisplayMemberBinding_Is_Not_Applied_To_SelectionBoxItem_Without_Selection()
{
    var target = new ComboBox
    {
        DisplayMemberBinding = new Binding(),
        ItemsSource = new[] { "foo", "bar" }
    };

    target.SelectedItem = null;
    Assert.Null(target.SelectionBoxItem);

    target.SelectedItem = "foo";
    Assert.NotNull(target.SelectionBoxItem);

    target.SelectedItem = null;
    Assert.Null(target.SelectionBoxItem);
}

@MrJul MrJul added the backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch label Oct 3, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052377-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@gehongyan
Copy link
Contributor Author

Can you please add it to ComboBoxTests?

Hi, @MrJul,

Thank you for your reply! I have appended this code snippet to the ComboBoxTests. The test passes successfully when run locally.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052417-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@MrJul MrJul added this pull request to the merge queue Oct 7, 2024
Merged via the queue into AvaloniaUI:master with commit 770d31f Oct 7, 2024
10 checks passed
@gehongyan gehongyan deleted the combobox-placeholder branch October 8, 2024 02:08
maxkatz6 pushed a commit that referenced this pull request Oct 27, 2024
…BoxItem (#16748)

* ComboBox empty selection should not generate a TextBlock as SelectionBoxItem

Fixes: #16747

* Add test for ComboBox DisplayMemberBinding behavior without selection.

---------

Co-authored-by: Julien Lebosquain <[email protected]>
@maxkatz6 maxkatz6 added backported-11.2.x and removed backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Placeholder Disappears when Setting DisplayMemberBinding on ComboBox
6 participants