NumberBox: Visual updates #5032

teaP · 2021-05-17T20:43:05Z

The main change here is that the inline spin buttons are hovering above the inner TextBox, so that the TextBox border extends around the entire control. The good news is that corners are now no big deal and we can remove corner-specific code. The bad news is that when the spin buttons are disabled (a likely scenario at least part of the time) we don't want the TextBox taking hover as input, so I created an InputEater button in the template which solves the problem. (I don't love that it's a button; if you have alternate suggestions I'm all ears.)

mdtauk · 2021-05-17T20:44:36Z

Could you please provide screenshots of the changes.

mdtauk · 2021-05-17T21:13:41Z

Also, here is a quick thought on how Number Box could handle its buttons

There is treating the buttons like the Search Button and Clear Buttons - contained within the TextBox, with smaller backplate visual but still 32 x 32 hit target.

Then there are the buttons with visual separators, where the backplates would probably be full height.

marcelwgn · 2021-05-18T18:43:57Z

dev/NumberBox/NumberBox.cpp

+    auto state = L"SpinButtonsCollapsed";

    if (spinButtonMode == winrt::NumberBoxSpinButtonPlacementMode::Inline)
    {
-        winrt::VisualStateManager::GoToState(*this, L"SpinButtonsVisible", false);
+        state = L"SpinButtonsVisible";
    }
    else if (spinButtonMode == winrt::NumberBoxSpinButtonPlacementMode::Compact)
    {
-        winrt::VisualStateManager::GoToState(*this, L"SpinButtonsPopup", false);
+        state = L"SpinButtonsPopup";


Should we use const expr's for this?

I never have before, so 🤷

Oh interesting, in other controls, we do that all the time🤔

Yeah - that's up to you. We typically define the state names in the .h file, like
static constexpr std::wstring_view s_scrollBarsSeparatorExpanded{ L"ScrollBarsSeparatorExpanded"sv };

dev/NumberBox/NumberBox.xaml

marcelwgn · 2021-05-18T21:38:20Z

dev/NumberBox/NumberBox.xaml


+                            <VisualStateGroup x:Name="SpinButtonStates">


nit: Add empty line here :)

marcelwgn · 2021-05-18T21:40:19Z

dev/NumberBox/NumberBox.cpp

+    auto state = L"SpinButtonsCollapsed";

    if (spinButtonMode == winrt::NumberBoxSpinButtonPlacementMode::Inline)
    {
-        winrt::VisualStateManager::GoToState(*this, L"SpinButtonsVisible", false);
+        state = L"SpinButtonsVisible";
    }
    else if (spinButtonMode == winrt::NumberBoxSpinButtonPlacementMode::Compact)
    {
-        winrt::VisualStateManager::GoToState(*this, L"SpinButtonsPopup", false);
+        state = L"SpinButtonsPopup";
    }


We could convert this to a lambda and have a constant string, but not sure if that adds more value. @teaP @StephenLPeters thoughts?

marcelwgn

Is it expected that I can't reach the inline increase/decrease buttons with keyboard anymore? It jumps to the input eater, however I can't actually use the increase/decrease buttons. Clicking on them works though.

marcelwgn · 2021-05-18T22:20:43Z

dev/NumberBox/NumberBox.xaml

@@ -165,25 +238,40 @@
                            </Grid>
                        </Popup>

+                        <Button x:Name="InputEater"


If you hover over the inline buttons and slowly move to the left, at some point this will eat the hover input and it seems that nothing is receiving the hover. Do we need to size this better?

I left a 4px border around the buttons, so there is an area where nothing is getting hover between the buttons, but I can trim it on the left side so that there's no dead space there.

There are aso small regions above, below, to the right and between those buttons that have the same effect. We hardly can fix the issue with the area between the split buttons without having two buttons right? I think it is fine right now.

dev/NumberBox/NumberBox.cpp

teaP · 2021-05-19T18:07:52Z

Is it expected that I can't reach the inline increase/decrease buttons with keyboard anymore? It jumps to the input eater, however I can't actually use the increase/decrease buttons. Clicking on them works though.

Oops, good catch! I'll fix that. The up/down buttons are not supposed to get keyboard focus either (since you can just use the arrow keys inside numberbox).

marcelwgn

Looks good to me. For @mdtauk, here the screenshots:

Bit surprised that disabled buttons don't receive focus, the workaround with the input eater seems reasonable here even if it's not the fanciest one.

marcelwgn · 2021-05-19T19:14:40Z

dev/NumberBox/NumberBox.xaml

@@ -165,25 +238,40 @@
                            </Grid>
                        </Popup>

+                        <Button x:Name="InputEater"


There are aso small regions above, below, to the right and between those buttons that have the same effect. We hardly can fix the issue with the area between the split buttons without having two buttons right? I think it is fine right now.

mdtauk · 2021-05-19T19:22:37Z

Looks good to me. For @mdtauk, here the screenshots:

Bit surprised that disabled buttons don't receive focus, the workaround with the input eater seems reasonable here even if it's not the fanciest one.

Thanks, I wasn't sure if the borders were to be retained or not. Are the hover and pressed backplates full height, or rounded with padding, like the clear and search buttons.

Oh also, shouldn't those chevrons be updated to the FluentUI System Icons

teaP · 2021-05-20T21:31:24Z

/azp run

azure-pipelines · 2021-05-20T21:31:34Z

Azure Pipelines successfully started running 1 pipeline(s).

RBrid · 2021-05-20T23:09:24Z

dev/NumberBox/NumberBox.cpp

+    auto state = L"SpinButtonsCollapsed";

    if (spinButtonMode == winrt::NumberBoxSpinButtonPlacementMode::Inline)
    {
-        winrt::VisualStateManager::GoToState(*this, L"SpinButtonsVisible", false);
+        state = L"SpinButtonsVisible";
    }
    else if (spinButtonMode == winrt::NumberBoxSpinButtonPlacementMode::Compact)
    {
-        winrt::VisualStateManager::GoToState(*this, L"SpinButtonsPopup", false);
+        state = L"SpinButtonsPopup";


Yeah - that's up to you. We typically define the state names in the .h file, like
static constexpr std::wstring_view s_scrollBarsSeparatorExpanded{ L"ScrollBarsSeparatorExpanded"sv };

RBrid · 2021-05-20T23:16:11Z

dev/NumberBox/NumberBox.xaml


+                            <VisualStateGroup x:Name="SpinButtonStates">


Should that be 'SpinButtonsStates' since you use 'SpinButtonsXXX' for the states?

I think one plural is probably enough?

…don't need to worry about which elements have corners anymore.

teaP · 2021-05-21T00:50:49Z

/azp run

azure-pipelines · 2021-05-21T00:51:00Z

Azure Pipelines successfully started running 1 pipeline(s).

Visual updates

4c5505a

ghost added the needs-triage Issue needs to be triaged by the area owners label May 17, 2021

beervoley added area-NumberBox NumberBox Control team-Controls Issue for the Controls team area-UIDesign UI Design, styling labels May 17, 2021

StephenLPeters removed the needs-triage Issue needs to be triaged by the area owners label May 17, 2021

Quick accessibility fix -- we need this to run AFTER m_textBox is set.

963ab44

marcelwgn reviewed May 18, 2021

View reviewed changes

dev/NumberBox/NumberBox.xaml

<VisualStateGroup x:Name="SpinButtonStates">

Copy link

Collaborator

marcelwgn May 18, 2021

Choose a reason for hiding this comment

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

nit: Add empty line here :)

marcelwgn reviewed May 18, 2021

View reviewed changes

ranjeshj reviewed May 19, 2021

View reviewed changes

dev/NumberBox/NumberBox.cpp Show resolved Hide resolved

ranjeshj requested a review from RBrid May 19, 2021 13:30

Adjusted the size of popup buttons, fixed keyboard focus issue.

7f93c2f

marcelwgn approved these changes May 19, 2021

View reviewed changes

RBrid approved these changes May 20, 2021

View reviewed changes

Removed test -- now that corners are only applied to the TextBox, we …

1f916f3

…don't need to worry about which elements have corners anymore.

teaP merged commit a43a2ab into main May 21, 2021

teaP deleted the user/teaP/NumberBox branch May 21, 2021 06:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NumberBox: Visual updates #5032

NumberBox: Visual updates #5032

teaP commented May 17, 2021

mdtauk commented May 17, 2021

mdtauk commented May 17, 2021

marcelwgn May 18, 2021

teaP May 18, 2021

marcelwgn May 18, 2021

RBrid May 20, 2021

marcelwgn May 18, 2021

marcelwgn May 18, 2021

marcelwgn left a comment

marcelwgn May 18, 2021

teaP May 19, 2021

marcelwgn May 19, 2021

teaP commented May 19, 2021

marcelwgn left a comment •

edited

Loading

marcelwgn May 19, 2021

mdtauk commented May 19, 2021 •

edited

Loading

teaP commented May 20, 2021

azure-pipelines bot commented May 20, 2021

RBrid May 20, 2021

RBrid May 20, 2021

teaP May 21, 2021

teaP commented May 21, 2021

azure-pipelines bot commented May 21, 2021

NumberBox: Visual updates #5032

NumberBox: Visual updates #5032

Conversation

teaP commented May 17, 2021

mdtauk commented May 17, 2021

mdtauk commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelwgn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teaP commented May 19, 2021

marcelwgn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdtauk commented May 19, 2021 • edited Loading

teaP commented May 20, 2021

azure-pipelines bot commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teaP commented May 21, 2021

azure-pipelines bot commented May 21, 2021

marcelwgn left a comment •

edited

Loading

mdtauk commented May 19, 2021 •

edited

Loading