[TabView] Fix bug where removing tabs from tabcollection would not resize tabs #6160
Conversation
ghost
added
the
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
StephenLPeters
removed
the
needs-triage
|
Thanks for getting on this one so fast! |
|
Closing a tab via the close button is not suppose to resize the tabs until you move your cursor outside of the tab strip. This is so you can close many tabs by quickly pressing your mouse button as the tabs animate under your cursor. The bug here is that when a tab closes we assume that the pointer is over the tab strip and wait until the next pointer exited event to resize the tabs. The correct solution is to resize the tabs immediately if the pointer is not over the items. |
|
Yes you are totally right @StephenLPeters, this should be fixed now. Unfortunately, we cannot differentiate between user clicking on the tab close button or the user removing from the collection. The work around we are now using is that we only resize tabstrip items if the pointer is not in the control, otherwise we delay resizing upon pointer exiting the tab strip. This leaves us with the niche case where removing items from the TabItems collection while user has the pointer in the tabstrip does not resize the control. Not sure though if it is worth trying to fix this use case or we can accept this oddity. |
This is a deviation from Edge's behavior... I think we could handle the close button click and the middle mouse button close in a seperate code path to the generic collection modified event. That being said this change gets us closer to correct for sure. |
Well sort of yes, but I think it would be a rather ugly way of doing that since we would somehow know that users clicked on the button so that we can treat the next tabitems collection change as a "user input remove" instead of collection modification. The thing is that a developers can hook into the TabRequestedEvent to remove the tab themselfes which makes this a lot more difficult. But maybe I'm missing the easy solution here. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@chingucoding I think these test failures might be expected given your change. Can you take a look? |
dev/TabView/TabView.cpp
Outdated
| if (TabWidthMode() == winrt::TabViewWidthMode::Equal) | ||
| { | ||
| m_updateTabWidthOnPointerLeave = true; | ||
| if (args.Index() == TabItems().Size()) | ||
| if (!m_pointerInTabstrip) |
There was a problem hiding this comment.
I think we also want to resize the items if it was the last item that was removed, even if the pointer is in the tab strip.
There was a problem hiding this comment.
Good point, that is also the reason of the test failures (all TabView interaction tests ran fine on my machine).
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
thanks Marcel!!!!!! |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
Description
There was a condition that only removing the last tab would resize the tabs which was not the right behavior since tabs should always be resized when one gets removed from tab collection.
Motivation and Context
Closes microsoft/terminal#9822
How Has This Been Tested?
Added new interaction tests
Screenshots (if appropriate):
@zadjii-msft @beervoley FYI