TabView: Fix all tabs squeezed into viewport when last tab is closed

[davidjohnoliver]

Description

When a large number of tabs are present (ie enough to scroll), closing the last tab will cause all of the tabs to squeeze into the visible viewport (see screencap below), before resetting to correct widths when pointer is removed from the tab header bar.

This is happening because the code path that's called when closing the last tab (fillAllAvailableSpace = true) isn't applying min (and max) tab widths as expected. This PR fixes the bug by clamping the applied tabWidth within those values.

Motivation and Context

How Has This Been Tested?

Tested manually in the MUXControlsTestApp

Screenshots (if appropriate):

Behaviour before the fix:

Behaviour after the fix:

[marcelwgn]

I'm not sure how feasible it is, but I think this would be worth adding a test for.

[davidjohnoliver]

I'm not sure how feasible it is, but I think this would be worth adding a test for.

Sure, I'll take a shot at making one.

[marcelwgn]

Awesome, if there's something I can help you with, let me know.

[ranjeshj]

@stmoy as FYI

[ghost]

All CLA requirements met.

[davidjohnoliver]

Added a test for this one, let me know if it needs anything else.

[StephenLPeters]

VerifyHeaderSizeWhenClosingLastTab [](start = 20, length = 34)

Does this test fail without your change?

[davidjohnoliver]

Yes, the TabViewItem Width gets set to ~79 without the fix (because all tabs are compressed into the visible viewport)

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

This looks great, however the VerifySizingBehaviorOnTabCloseComingFromNonScroll test is failing on all OS versions. @davidjohnoliver Do you need help investigating this?

[davidjohnoliver]

@StephenLPeters I can repro the test failure locally. I think the bug is in the test itself.

This is what I'm seeing for that test, when the second-last tab is closed:

• Before the change: the TabView doesn't resize right away, because the remaining tab is resized to take the whole viewport. At this point TabViewWidth.Text is set to "500". The TabView resizes to 283px when the pointer exits the tab area, when readTabViewWidthButton.Click() is called. Note however that the button doesn't actually get clicked from what I can see - the UI test seemingly tries to click on where it was, but it moves, because the TabView is resized. So the value is still "500" when it's read, even though the TabView has resized at this point.
• After the change: The TabView resizes as soon as the second-last tab closes (because the remaining tab is clamped to maxTabWidth). TabViewWidth.Text is set to "283". The test is expecting "500" so it fails.

I think that modifying the expected value to "283" should get the test to pass and it would actually align with VerifySizingBehaviorOnTabCloseComingFromScroll(), which just seems to be closing the tabs in a different order. (https://github.com/microsoft/microsoft-ui-xaml/blob/master/dev/TabView/InteractionTests/TabViewTests.cs#L735)

Does this make sense @StephenLPeters and @chingucoding ? Maybe there's an unwanted change in behavior that I'm missing here.

[StephenLPeters]

To verify, It is intended for TabView to have very similar behavior has Edge's tab closing behavior. That is, when closing a tab the x button of the tab that takes its place should be under the cursor. This means that when there are many tabs and a non-last tab is closed by the x button that the tabs should not resize until the pointer leaves the tab well. This way the tab to the right of the closed tab is repositioned to the location of the closed tab and its x button is under the cursor.

When the last tab is closed, we should exapand the tab size so that the tab to the left is now under the cursor, unless that expansion would bring the remaining tab item's size above the max size.

Your change doesn't remove the first behavior and improves/enables the second behavior; is that correct? Its a little bit different because our tabview scrolls while edge's does not but the goal of the x button being under the cursor is the same.

Your claim is that the test that is failing now has never tested the scenario properly because the test app doesn't actually click the ReadTabViewWidth button and that with your change the tab view behavior changed in a different way that exposed this inadequacy of the test while not actually changing the behavior the test was testing for? (This question might sound harsh but that isn't intended :) )

@chingucoding I'm noticing that when the tabs resize because the user moves the mouse out of the tab well, they don't animate to their new size. Do you know why that is?

[marcelwgn]

@chingucoding I'm noticing that when the tabs resize because the user moves the mouse out of the tab well, they don't animate to their new size. Do you know why that is?

As in why there is no animation and they just jump? I think that's because we are using ListView and animating that wasn't a priority at the time (if it would have been achievable at all).

[StephenLPeters]

@chingucoding I'm noticing that when the tabs resize because the user moves the mouse out of the tab well, they don't animate to their new size. Do you know why that is?

As in why there is no animation and they just jump? I think that's because we are using ListView and animating that wasn't a priority at the time (if it would have been achievable at all).

They do animate when we close the right most tab though I wonder what the difference is.

[StephenLPeters]

@davidjohnoliver FYI about my message above, I realize now I didn't @ you.

[marcelwgn]

@chingucoding I'm noticing that when the tabs resize because the user moves the mouse out of the tab well, they don't animate to their new size. Do you know why that is?

As in why there is no animation and they just jump? I think that's because we are using ListView and animating that wasn't a priority at the time (if it would have been achievable at all).

They do animate when we close the right most tab though I wonder what the difference is.

That's really interesting. I don't see that we would have done anything different in the code paths so this is probably the ListView behavior. Given that in the animated case the resizing is combined with the removal of an item, ListView is probably animating this as part of the collection change, not the width change.

[davidjohnoliver]

Your change doesn't remove the first behavior and improves/enables the second behavior; is that correct? Its a little bit different because our tabview scrolls while edge's does not but the goal of the x button being under the cursor is the same.

Yes. For non-last tabs it should work exactly the same as before, and for the last tab it should still resize if needed, just now respecting the min/max width settings.

Your claim is that the test that is failing now has never tested the scenario properly because the test app doesn't actually click the ReadTabViewWidth button and that with your change the tab view behavior changed in a different way that exposed this inadequacy of the test while not actually changing the behavior the test was testing for? (This question might sound harsh but that isn't intended :) )

Yes and no - I think most of the test works properly. 😄 But yes, the final assert (https://github.com/microsoft/microsoft-ui-xaml/blob/master/dev/TabView/InteractionTests/TabViewTests.cs#L786) is failing because the test wasn't working exactly as designed, and the behaviour under test at L786 (the actual width of TabView, in an infinite-width horizontal StackPanel, with only one remaining tab, after removing the pointer from the tab area) has in reality not changed.

[StephenLPeters]

awesome, thanks. I agree with your assessment and think the test should be updated to check for 283 as you suggested, can you add that change to this PR?

[davidjohnoliver]

awesome, thanks. I agree with your assessment and think the test should be updated to check for 283 as you suggested, can you add that change to this PR?

Done!

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]