Skip to content

Commit

Permalink
Numberbox header aligning (#2117)
Browse files Browse the repository at this point in the history
* Fix issue with header taking up space when not specified/null

* Add NumberBox header test

* Add log comments for test

* Fix behavior of empty string

* Update test
  • Loading branch information
marcelwgn authored Mar 21, 2020
1 parent 290b432 commit 768854a
Show file tree
Hide file tree
Showing 9 changed files with 122 additions and 2 deletions.
10 changes: 9 additions & 1 deletion dev/Generated/NumberBox.properties.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ void NumberBoxProperties::EnsureProperties()
winrt::name_of<winrt::NumberBox>(),
false /* isAttached */,
ValueHelper<winrt::IInspectable>::BoxedDefaultValue(),
nullptr);
winrt::PropertyChangedCallback(&OnHeaderPropertyChanged));
}
if (!s_HeaderTemplateProperty)
{
Expand Down Expand Up @@ -275,6 +275,14 @@ void NumberBoxProperties::ClearProperties()
s_ValueProperty = nullptr;
}

void NumberBoxProperties::OnHeaderPropertyChanged(
winrt::DependencyObject const& sender,
winrt::DependencyPropertyChangedEventArgs const& args)
{
auto owner = sender.as<winrt::NumberBox>();
winrt::get_self<NumberBox>(owner)->OnHeaderPropertyChanged(args);
}

void NumberBoxProperties::OnIsWrapEnabledPropertyChanged(
winrt::DependencyObject const& sender,
winrt::DependencyPropertyChangedEventArgs const& args)
Expand Down
4 changes: 4 additions & 0 deletions dev/Generated/NumberBox.properties.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ class NumberBoxProperties
static void EnsureProperties();
static void ClearProperties();

static void OnHeaderPropertyChanged(
winrt::DependencyObject const& sender,
winrt::DependencyPropertyChangedEventArgs const& args);

static void OnIsWrapEnabledPropertyChanged(
winrt::DependencyObject const& sender,
winrt::DependencyPropertyChangedEventArgs const& args);
Expand Down
30 changes: 30 additions & 0 deletions dev/NumberBox/InteractionTests/NumberBoxTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,36 @@ public void BasicExpressionTest()
}
}

[TestMethod]
public void VerifyNumberBoxHeaderBehavior()
{
using (var setup = new TestSetupHelper("NumberBox Tests"))
{
var toggleHeaderButton = FindElement.ByName<Button>("ToggleHeaderValueButton");
var header = FindElement.ByName<TextBlock>("NumberBoxHeaderClippingDemoHeader");

Log.Comment("Check header is null");
Verify.IsNull(header);

Log.Comment("Set header");
toggleHeaderButton.Invoke();
Wait.ForIdle();

header = FindElement.ByName<TextBlock>("NumberBoxHeaderClippingDemoHeader");
Log.Comment("Check if header is present");
Verify.IsNotNull(header);
Log.Comment("Remove header");
toggleHeaderButton.Invoke();
Wait.ForIdle();
ElementCache.Clear();

Log.Comment("Check that header is null again");
header = FindElement.ByName<TextBlock>("NumberBoxHeaderClippingDemoHeader");
Verify.IsNull(header);
}
}


Button FindButton(UIObject parent, string buttonName)
{
foreach (UIObject elem in parent.Children)
Expand Down
41 changes: 41 additions & 0 deletions dev/NumberBox/NumberBox.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "Utils.h"
#include "winnls.h"

static constexpr wstring_view c_numberBoxHeaderName{ L"HeaderContentPresenter"sv };
static constexpr wstring_view c_numberBoxDownButtonName{ L"DownSpinButton"sv };
static constexpr wstring_view c_numberBoxUpButtonName{ L"UpSpinButton"sv };
static constexpr wstring_view c_numberBoxTextBoxName{ L"InputBox"sv };
Expand Down Expand Up @@ -135,6 +136,12 @@ void NumberBox::OnApplyTemplate()
}
}

if (const auto headerPresenter = GetTemplateChildT<winrt::ContentPresenter>(c_numberBoxHeaderName, controlProtected))

This comment has been minimized.

Copy link
@Kinnara

Kinnara Mar 23, 2020

Contributor

There seem to be a couple issues here:

  1. Calling GetTemplateChild regardless of whether the header should be visible, will always realize the header presenter and make the x:DeferLoadStrategy="Lazy" in the template useless.
  2. If the header is set before OnApplyTemplate, it won't be visible because when OnHeaderPropertyChanged was called m_headerPresenter had not been set yet.

This comment has been minimized.

Copy link
@marcelwgn

marcelwgn Mar 23, 2020

Author Collaborator

Thanks for pointing those regressions out! I've created a PR fixing those issues: #2148

Feel free to check if there is anything missing in the PR.

{
// Set presenter to enable lightweight styling of the headers margin
m_headerPresenter.set(headerPresenter);
}

m_textBox.set([this, controlProtected]() {
const auto textBox = GetTemplateChildT<winrt::TextBox>(c_numberBoxTextBoxName, controlProtected);
if (textBox)
Expand Down Expand Up @@ -298,6 +305,40 @@ void NumberBox::UpdateValueToText()
}
}

void NumberBox::OnHeaderPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args)

This comment has been minimized.

Copy link
@Kinnara

Kinnara Mar 23, 2020

Contributor

TextBox also has the following behavior: if HeaderTemplate is set, then the header presenter is made visible, no matter what the value of Header is.

{
// To enable lightweight styling, collapse header presenter if there is no header specified
if (const auto headerPresenter = m_headerPresenter.get())
{
if (const auto header = Header())
{
// Check if header is string or not
if (const auto headerAsString = Header().try_as<winrt::IReference<winrt::hstring>>())
{
if (headerAsString.Value().empty())
{
// String is the empty string, hide presenter
headerPresenter.Visibility(winrt::Visibility::Collapsed);
}
else
{
// String is not an empty string
headerPresenter.Visibility(winrt::Visibility::Visible);
}
}
else
{
// Header is not a string, so let's show header presenter
headerPresenter.Visibility(winrt::Visibility::Visible);
}
}
else
{
headerPresenter.Visibility(winrt::Visibility::Collapsed);
}
}
}

void NumberBox::OnValidationModePropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args)
{
ValidateInput();
Expand Down
2 changes: 2 additions & 0 deletions dev/NumberBox/NumberBox.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class NumberBox :
// IFrameworkElement
void OnApplyTemplate();

void OnHeaderPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args);
void OnSpinButtonPlacementModePropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args);
void OnTextPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args);

Expand Down Expand Up @@ -87,6 +88,7 @@ class NumberBox :
winrt::SignificantDigitsNumberRounder m_displayRounder{};

tracker_ref<winrt::TextBox> m_textBox{ this };
tracker_ref<winrt::ContentPresenter> m_headerPresenter{ this };
tracker_ref<winrt::Popup> m_popup{ this };

winrt::RepeatButton::Click_revoker m_upButtonClickRevoker{};
Expand Down
1 change: 1 addition & 0 deletions dev/NumberBox/NumberBox.idl
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ unsealed runtimeclass NumberBox : Windows.UI.Xaml.Controls.Control
static Windows.UI.Xaml.DependencyProperty LargeChangeProperty{ get; };
static Windows.UI.Xaml.DependencyProperty TextProperty{ get; };

[MUX_PROPERTY_CHANGED_CALLBACK(TRUE)]
static Windows.UI.Xaml.DependencyProperty HeaderProperty{ get; };
static Windows.UI.Xaml.DependencyProperty HeaderTemplateProperty{ get; };
static Windows.UI.Xaml.DependencyProperty PlaceholderTextProperty{ get; };
Expand Down
4 changes: 3 additions & 1 deletion dev/NumberBox/NumberBox.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@
Foreground="{ThemeResource TextControlHeaderForeground}"
Margin="{ThemeResource TextBoxTopHeaderMargin}"
TextWrapping="Wrap"
VerticalAlignment="Top"/>
VerticalAlignment="Top"
Visibility="Collapsed"
x:DeferLoadStrategy="Lazy" />

<TextBox x:Name="InputBox"
Grid.Row="1"
Expand Down
8 changes: 8 additions & 0 deletions dev/NumberBox/TestUI/NumberBoxPage.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@
<Button x:Name="SetValueNaNButton" AutomationProperties.Name="SetValueNaNButton" Content="Set value to NaN" Click="SetNaNButton_Click" Margin="0,4,0,0"/>

<Button x:Name="SetTwoWayBoundValueNaNButton" AutomationProperties.Name="SetTwoWayBoundValueNaNButton" Content="Set two way bound value to NaN" Click="SetTwoWayBoundNaNButton_Click" Margin="0,4,0,0"/>

<Button x:Name="ToggleHeaderValueButton" AutomationProperties.Name="ToggleHeaderValueButton" Content="Toggle header for clipping issue" Click="ToggleHeaderValueButton_Click" Margin="0,4,0,0"/>
</StackPanel>

<Grid Grid.Column="1">
Expand Down Expand Up @@ -122,6 +124,12 @@
Value="{x:Bind DataModelWithINPC.Value, Mode=TwoWay}" />
<TextBlock x:Name="TwoWayBoundNumberBoxValue" AutomationProperties.Name="TwoWayBoundNumberBoxValue" />
</StackPanel>

<!-- Testing alignment with textbox and without specified header -->
<StackPanel Orientation="Horizontal">
<TextBox MaxHeight="30" VerticalAlignment="Top"/>
<controls:NumberBox x:Name="HeaderTestingNumberBox" MaxHeight="32" VerticalAlignment="Top" PlaceholderText="I should not be clipped without header"/>
</StackPanel>
</StackPanel>
</Grid>

Expand Down
24 changes: 24 additions & 0 deletions dev/NumberBox/TestUI/NumberBoxPage.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.ComponentModel;
using Windows.Globalization.NumberFormatting;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Automation;
using Windows.UI.Xaml.Controls;

namespace MUXControlsTestApp
Expand Down Expand Up @@ -124,6 +125,29 @@ private void SetTwoWayBoundNaNButton_Click(object sender, RoutedEventArgs e)
TwoWayBoundNumberBoxValue.Text = TwoWayBoundNumberBox.Value.ToString();
}

private void ToggleHeaderValueButton_Click(object sender, RoutedEventArgs e)
{
if(HeaderTestingNumberBox.Header is null)
{
var demoHeader = new TextBlock();
demoHeader.SetValue(AutomationProperties.NameProperty, "NumberBoxHeaderClippingDemoHeader");
demoHeader.Text = "Test header";
HeaderTestingNumberBox.Header = demoHeader;
}
else
{
// Switching between normal header and empty string header
if(HeaderTestingNumberBox.Header as string is null)
{
HeaderTestingNumberBox.Header = "";
}
else
{
HeaderTestingNumberBox.Header = null;
}
}
}

private void TextPropertyChanged(DependencyObject o, DependencyProperty p)
{
TextTextBox.Text = TestNumberBox.Text;
Expand Down

0 comments on commit 768854a

Please sign in to comment.