Resize tab view items only once the pointer has left the TabViewItem strip #2569

marcelwgn · 2020-05-29T13:05:43Z

Description

Move the resizing of TabViewItems to a PointerExited event of the TabItemStrip container grid.. The scroll buttons are updated after every item to disable them once the number of items is low enough to make scrolling unecessary.

Motivation and Context

Closes #2417

How Has This Been Tested?

Add new interaction test and testpage.

Screenshots (if appropriate):

shaheedmalik · 2020-05-29T16:59:40Z

Is that a glitch in the screen shot 4 clicks in? It seems like it isn't consistent.

marcelwgn · 2020-05-29T17:04:01Z

I am not entirely sure what you mean. The items jump after the 5th click because we hide the scroll buttons since for that number of items, we don't need scrolling anymore.

shaheedmalik · 2020-05-29T17:08:35Z

I am not entirely sure what you mean. The items jump after the 5th click because we hide the scroll buttons since for that number of items, we don't need scrolling anymore.

"Resize tab view items only once the pointer has left the TabViewItem strip"

The close button isn't underneath the the pointer and you didn't move it outside of the tab.

marcelwgn · 2020-05-29T17:13:16Z

Yes the items moved because the scroll buttons on the left and right have disappeared, the items did not resize.

The scroll buttons take up space and move the whole list of tabs (aka TabViewItems) a bit to the right so they are always visible and don't occlude the first and last item. When we remove enough items so that the items wouldn't need scrolling anymore, we hide those buttons, thus making space on the left side. Since we don't want to render an empty gap, the tabs list get's moved to the right.

shaheedmalik · 2020-05-29T17:30:35Z

Yes the items moved because the scroll buttons on the left and right have disappeared, the items did not resize.

The scroll buttons take up space and move the whole list of tabs (aka TabViewItems) a bit to the right so they are always visible and don't occlude the first and last item. When we remove enough items so that the items wouldn't need scrolling anymore, we hide those buttons, thus making space on the left side. Since we don't want to render an empty gap, the tabs list get's moved to the right.

This is different than how Edge handles it. The scroll bars should remain until the user moves the cursor outside the TabViewItem strip,

If I have 30 tabs open and want to close all of them, as soon as the scroll bars removes, I will have to move my mouse back to the correct tab. This behavior doesn't fix the original problem.

Proposal: Tab resizing should be differed in order to maintain the close button under the pointer
How Edge handles it.
https://camo.githubusercontent.com/65c1f251089fa08355549d2110db3d35a3888e09/68747470733a2f2f692e6779617a6f2e636f6d2f31633964326235366264356537393962376562376139666265646235303165392e676966

StephenLPeters · 2020-05-29T17:53:18Z

Yes the items moved because the scroll buttons on the left and right have disappeared, the items did not resize.
The scroll buttons take up space and move the whole list of tabs (aka TabViewItems) a bit to the right so they are always visible and don't occlude the first and last item. When we remove enough items so that the items wouldn't need scrolling anymore, we hide those buttons, thus making space on the left side. Since we don't want to render an empty gap, the tabs list get's moved to the right.

This is different than how Edge handles it. The scroll bars should remain until the user moves the cursor outside the TabViewItem strip,

If I have 30 tabs open and want to close all of them, as soon as the scroll bars removes, I will have to move my mouse back to the correct tab. This behavior doesn't fix the original problem.

Proposal: Tab resizing should be differed in order to maintain the close button under the pointer
How Edge handles it.
https://camo.githubusercontent.com/65c1f251089fa08355549d2110db3d35a3888e09/68747470733a2f2f692e6779617a6f2e636f6d2f31633964326235366264356537393962376562376139666265646235303165392e676966

Edge and chrome do not scroll their tabs, the experience is actually pretty bad in both browsers if you open more tabs than can be displayed in the window. They both hide tabs off screen with no way to navigate to them with mouse. That being said, I think it would be better not to hide the scroll buttons until we relayout. A big benefit of this change is that you can close many tabs without moving your pointer. I think it would be better to disable the scrollbuttons rather than collapse them while the pointer remains in the tab well, and then collapse them when we adjust the tab size.

StephenLPeters · 2020-05-29T17:59:19Z

dev/TabView/TabView.cpp

-    m_tabContainerGrid.set(GetTemplateChildT<winrt::Grid>(L"TabContainerGrid", controlProtected));
+    if (const auto& containerGrid = GetTemplateChildT<winrt::Grid>(L"TabContainerGrid", controlProtected))
+    {
+        m_tabContainerGrid.set(containerGrid);


m_tabContainerGrid [](start = 8, length = 18)

a downside to this approach is that you will never set m_tabContainerGrid to null, even if a second template is later applied which doesn't have a TabContainerGrid. Seems unlikely in this case.

Updated the code to set it to null if we get a nullptr.

StephenLPeters · 2020-05-29T18:00:44Z

dev/TabView/TabView.cpp

+        auto scopeGuard = gsl::finally([this]()
+        {
+            updateTabWidthOnPointerLeave = false;
+        });


Scope guarded flag should be declared before UpdateTabWidths() is called. This is so that if UpdateTabWidths() throws but that exception is later handled we will still update this flag.

Good point, moved the UpdateTabWidths call behind the scopeguard.

stmoy · 2020-05-29T18:01:39Z

I think it would be better to disable the scrollbuttons rather than collapse them while the pointer remains in the tab well, and then collapse them when we adjust the tab size.

Agreed, this would be a reasonable way to do it. We could also just hide them. The end-user scenario is to keep the x-to-close button in the same location.

marcelwgn · 2020-05-29T19:27:09Z

I've updated the code now to disable the scroll buttons instead of hiding them completely:

Note that when we close the TabViewItems, the items get resized to ensure that leaving the pointer on the last item allows you to close multiple tabs without moving the pointer.

shaheedmalik · 2020-05-29T19:31:54Z

I've updated the code now to disable the scroll buttons instead of hiding them completely:

Note that when we close the TabViewItems, the items get resized to ensure that leaving the pointer on the last item allows you to close multiple tabs without moving the pointer.

This is better but it still shouldn't resize until the mouse leaves the TabItem strip.

marcelwgn · 2020-05-29T19:39:56Z

If we close the last item, items should be resized, which is the same behavior as edge and chrome have according to the proposal.

The question is should we expand the other items so that the items fill the space that the items took up before, or should all the sizes stay the same? @stmoy @StephenLPeters your thoughts?

shaheedmalik · 2020-05-29T19:52:52Z

If we close the last item, items should be resized, which is the same behavior as edge and chrome have according to the proposal.

The question is should we expand the other items so that the items fill the space that the items took up before, or should all the sizes stay the same? @stmoy @StephenLPeters your thoughts?

When this is done in Edge the tabs next to it will slide over with the Close X under the pointer until approx 2 tabs or so are left.

This is how Edge does it.

StephenLPeters · 2020-05-29T20:41:20Z

Is the space to the right of the + button owned by the tabview? If so I think we should move the pointer exited handler up the visual tree to the point where that portion of the control is included so the user doesn't get a pointer exited event unintentionally

stmoy · 2020-05-29T21:13:39Z

Is the space to the right of the + button owned by the tabview? If so I think we should move the pointer exited handler up the visual tree to the point where that portion of the control is included so the user doesn't get a pointer exited event unintentionally

+1, makes sense. The space to the right of the + button is also owned by the TabView, yes. It's the TabStripFooter area. However, it is outside of the tab strip area, and could be custom content, so I'm not sure where the handler would go. I imagine the Header, Strip, and Footer are all contained within some container - so maybe there?

marcelwgn · 2020-05-29T21:24:08Z

So I've updated the behavior again. Now it should be completely aligned with edge and chrome:

marcelwgn · 2020-05-29T21:28:47Z

Is the space to the right of the + button owned by the tabview? If so I think we should move the pointer exited handler up the visual tree to the point where that portion of the control is included so the user doesn't get a pointer exited event unintentionally

+1, makes sense. The space to the right of the + button is also owned by the TabView, yes. It's the TabStripFooter area. However, it is outside of the tab strip area, and could be custom content, so I'm not sure where the handler would go. I imagine the Header, Strip, and Footer are all contained within some container - so maybe there?

The current implementation already subscribes to the correct element for that use case. Moving the cursor to the right side (aka PaneFooter), the items do not get resized, only when the pointer moves out of that area:

StephenLPeters

StephenLPeters · 2020-06-02T17:45:01Z

/azp run

azure-pipelines · 2020-06-02T17:45:12Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2020-06-02T17:46:48Z

Eventuallly we should add an animation for the change in size when the pointer exits the well.. I don't think we need to block this change on that though. @chingucoding could you possibly open an issue for that and include one of the gifs here for reference.

marcelwgn · 2020-06-02T18:32:25Z

Eventuallly we should add an animation for the change in size when the pointer exits the well.. I don't think we need to block this change on that though. @chingucoding could you possibly open an issue for that and include one of the gifs here for reference.

Sure thing, created #2588 for tracking.

StephenLPeters · 2020-06-03T19:34:54Z

@chingucoding the TabSizeAndScrollButtonsTest is failing with [Error]: IsFalse - Scroll buttons should disappear [File d:\a\1\s\dev\TabView\InteractionTests\TabViewTests.cs Line: 168]

I think there is an issue with the ADO site that will prevent you from seeing the tests page. We'll look into it.

marcelwgn · 2020-06-03T19:46:38Z

Thanks for the ping @StephenLPeters taking a look now. Also, yes the tests tab is missing, thanks for listing the failing tests here!

marcelwgn · 2020-06-03T23:00:03Z

The test should run successfully now, the issue was the we a) didn't handle non equal mode correctly and b) we did not leave the TabItemStrip to update the scrollbuttons.

StephenLPeters · 2020-06-03T23:34:23Z

/azp run

azure-pipelines · 2020-06-03T23:34:34Z

Azure Pipelines successfully started running 1 pipeline(s).

marcelwgn · 2020-06-04T08:31:56Z

Seems like we still have test failures, however I can't repro that test failure on my machine. @StephenLPeters Did the test fail on a specific Windows 10 Build?

StephenLPeters · 2020-06-04T17:45:59Z

Seems like we still have test failures, however I can't repro that test failure on my machine. @StephenLPeters Did the test fail on a specific Windows 10 Build?

Yes, consistently failing on RS3 and RS4 with this error: [Error]: IsFalse - Scroll buttons should disappear [File d:\a\1\s\dev\TabView\InteractionTests\TabViewTests.cs Line: 172]

marcelwgn · 2020-06-04T18:18:41Z

Could it be that the pointer moving is not reliable enough on those windows version? I highly doubt that, however I don't think that there are features that we are using in those areas that differ on RS3/4 compared to the other Windows 10 versions which would create that test failures.

StephenLPeters · 2020-06-04T18:54:58Z

Looking at a screenshot from the test failure the mouse hasn't been moved from the tab's close button. I think that this is because the element you've chosen to move the mouse cursor to is not on screen. Maybe if you change the test to move cursor to something like the ISClosable on first tab check box it would resolve the issue.

marcelwgn · 2020-06-04T19:06:14Z

Oh right, good idea. I've updated the test now.

Maybe we should have some form of indication in the MUXControlsTestApp that shows what will get rendered inside the CI test run. That way we know how the test page will look like in CI while writing the tests and avoid those errors more easily. Thoughts @ranjeshj @StephenLPeters @kmahone ?

kmahone · 2020-06-04T19:13:30Z

We could add a button to the app somewhere that resizes the root Frame to be 1024x768 (or whatever the correct size is once you account for taskbar, titlebar, etc.). That way the app content will be the same size as what it is in the CI pipeline.

marcelwgn · 2020-06-04T19:26:13Z

I think a button is the best idea for that. I've created a tracking issue for that #2604.

StephenLPeters · 2020-06-04T20:32:56Z

/azp run

azure-pipelines · 2020-06-04T20:33:06Z

Azure Pipelines successfully started running 1 pipeline(s).

@jtippet

See: https://github.com/microsoft/microsoft-ui-xaml/releases/tag/v2.5.0-prerelease.200609001 > ### Notable Changes: > > Resize tab view items only once the pointer has left the TabViewItem strip (microsoft/microsoft-ui-xaml#2569) > Align TabView visuals with Edge (microsoft/microsoft-ui-xaml#2201) > Fix background of MenuFlyout in white high contrast (microsoft/microsoft-ui-xaml#2446) > TabView: Make TabViewItem consume the TabViewItemHeaderForeground theme resource (microsoft/microsoft-ui-xaml#2348) > TabView: Add tooltips to its scrolling buttons. (microsoft/microsoft-ui-xaml#2369) * [x] Related to #5360 (@jtippet confirms that this alone does not close it.) * [x] I work here

@jtippet

See: https://github.com/microsoft/microsoft-ui-xaml/releases/tag/v2.5.0-prerelease.200609001 > ### Notable Changes: > > Resize tab view items only once the pointer has left the TabViewItem strip (microsoft/microsoft-ui-xaml#2569) > Align TabView visuals with Edge (microsoft/microsoft-ui-xaml#2201) > Fix background of MenuFlyout in white high contrast (microsoft/microsoft-ui-xaml#2446) > TabView: Make TabViewItem consume the TabViewItemHeaderForeground theme resource (microsoft/microsoft-ui-xaml#2348) > TabView: Add tooltips to its scrolling buttons. (microsoft/microsoft-ui-xaml#2369) * [x] Related to #5360 (@jtippet confirms that this alone does not close it.) * [x] I work here

marcelwgn added 4 commits May 28, 2020 10:13

Add initial behavior

01f31c4

Hide scroll buttons when they are not needed anymore

dd78553

Add test

85d99b9

Remove comment

41e2f25

msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label May 29, 2020

Switch to tabstrip instead of listview for pointer exited event

4c03af4

Resize tabs none the less if last item got removed

ceb4c77

StephenLPeters reviewed May 29, 2020

View reviewed changes

Keep buttons visible but disabled, CR feedback

a587fd2

Update behavior again, now aligned with edge

89b02d3

StephenLPeters approved these changes Jun 2, 2020

View reviewed changes

marcelwgn mentioned this pull request Jun 2, 2020

Animate TabView tab resizing #2588

Closed

ranjeshj added area-TabView team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Jun 3, 2020

Fix test failure

ebb74f2

Move pointer to different UI element in test

d94dc69

StephenLPeters merged commit 637b380 into microsoft:master Jun 5, 2020

marcelwgn deleted the tabview-sizing-delay branch June 6, 2020 11:33

zadjii-msft mentioned this pull request Jul 7, 2020

Update MUX to 2.5.0-prerelease.200609001 microsoft/terminal#6819

Merged

2 tasks

zadjii-msft mentioned this pull request May 7, 2021

Remaining tabs are not resized when closing tabs with keyboard #4971

Closed

Resize tab view items only once the pointer has left the TabViewItem strip #2569

Resize tab view items only once the pointer has left the TabViewItem strip #2569

Conversation

marcelwgn commented May 29, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

shaheedmalik commented May 29, 2020

marcelwgn commented May 29, 2020

shaheedmalik commented May 29, 2020

marcelwgn commented May 29, 2020

shaheedmalik commented May 29, 2020

StephenLPeters commented May 29, 2020

StephenLPeters May 29, 2020

Choose a reason for hiding this comment

marcelwgn May 29, 2020

Choose a reason for hiding this comment

StephenLPeters May 29, 2020

Choose a reason for hiding this comment

marcelwgn May 29, 2020

Choose a reason for hiding this comment

stmoy commented May 29, 2020

marcelwgn commented May 29, 2020

shaheedmalik commented May 29, 2020

marcelwgn commented May 29, 2020

shaheedmalik commented May 29, 2020 • edited Loading

StephenLPeters commented May 29, 2020

stmoy commented May 29, 2020

marcelwgn commented May 29, 2020

marcelwgn commented May 29, 2020

StephenLPeters left a comment

Choose a reason for hiding this comment

StephenLPeters commented Jun 2, 2020

azure-pipelines bot commented Jun 2, 2020

StephenLPeters commented Jun 2, 2020

marcelwgn commented Jun 2, 2020

StephenLPeters commented Jun 3, 2020

marcelwgn commented Jun 3, 2020

marcelwgn commented Jun 3, 2020

StephenLPeters commented Jun 3, 2020

azure-pipelines bot commented Jun 3, 2020

marcelwgn commented Jun 4, 2020

StephenLPeters commented Jun 4, 2020

marcelwgn commented Jun 4, 2020

StephenLPeters commented Jun 4, 2020

marcelwgn commented Jun 4, 2020

kmahone commented Jun 4, 2020

marcelwgn commented Jun 4, 2020

StephenLPeters commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

marcelwgn commented May 29, 2020 •

edited

Loading

shaheedmalik commented May 29, 2020 •

edited

Loading