Fix tab row doesn't grow

[beviu]

Summary of the Pull Request

Fix the bug where resizing the window to a smaller size makes the tab row shrink but it doesn't grows back to its original size when the window is resized to its original size.

References

PR Checklist

• Closes Tab bar doesn't grow when you resize the window, it only shrinks #3300
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

I don't really understand the bug but I managed to solve it by changing things so I don't know if this is a good PR.

Anyways, here is how I fixed the bug:

We need to make sure that both the tab row and the drag bar grow as much as they can, but we give priority to the tab row.

Before, the solution was to manually set the tab row's maximum size by taking the title bar's size and subtracting the drag bar and min-max-close buttons' size.

I think I found a better solution that for some reason fixes this bug: I decided to put the drag bar inside the tab row and let the tab row extend as much as it can.

Other changes:

• I also removed TabRowControl::OnNewTabButtonClick because it was not used anywhere (the TerminalPage listens for the click itself)? I can add it back or make a separate PR if needed.
• Finally, I was going to move TitleBar::DragBar_DoubleTapped into the TabRowControl but I ran the terminal and it worked without it. So I just removed it and double click to maximize still works. Maybe DefWindowProc is already implementing double click on title bar to maximize based on what WM_NCHITTEST returns? I tried returning HTNOWHERE everytime for WM_NCHITTEST and the double click on title bar to maximize no longer worked. Also, when I put a breakpoint in TitleBar::DragBar_DoubleTapped, I found that it was not even called anyways. EDIT: TitleBar::DragBar_DoubleTapped is not called because the Xaml doesn't handle the drag bar because of Apply a GDI region to the top level Island window to allow dragging with a single Xaml Island #929.

Validation Steps Performed

I checked if it was fixed.

Before:

After:

[biltongza]

Anyone else notice the header text getting cut off in the gifs you posted? (not related to this issue I know)

[zadjii-msft]

@greg904 So I think initially we had some reluctance to merge this PR, since there 's some plans we've been throwing around to refactor how AppHost and TerminalApp own the titlebar. However, this is still goodness, and the refactoring probably won't happen for a while. So we should just get this in now, and deal with refactoring later. I'll start reviewing this now.

Looks like there's a fair amount of merge conflicts (probably from #3394), if you want to get started on resolving those.

Sorry for the delay 😔

[zadjii-msft]

I have a lot of thoughts here:

1. I miss the abstraction that the TitlebarControl is just something that could hold any content, and we just so happen to be putting a TabRowControl there. I suppose the theoretical plans to re-use this work elsewhere never really panned out, but still, it was a nice abstraction.
2. I dislike this bug more than I like that abstraction
3. I'm glad to be rid of the MaxWidth hack, even if this doesn't fix Terminal crashes when dragged to another monitor #3303
4. Let's get this merged 😀

[DHowett-MSFT]

Hey @greg904! Thanks for looking into this, and sorry it took us forever to review.
We're tracking the original issue upstream in WinUI now, which is great... but I'm going to hold this PR until we know whether upstream can fix it for us. We shouldn't need to work too hard to get around their layout issues. 😄

[DHowett-MSFT]

FWIW, this is microsoft/microsoft-ui-xaml#1581

[beviu]

Ok. BTW if the bug is fixed upstream, then this maybe this is still good because it makes the code cleaner IMO (it's subjective of course). It's also more like the docs: https://docs.microsoft.com/en-us/windows/uwp/design/controls-and-patterns/tab-view#display-tabview-tabs-in-a-windows-titlebar. In that case I could rename the PR to "Refactor title bar : put drag region inside tab row and remove useless code".

[DHowett-MSFT]

@greg904 I personally owe you an apology here, as I've been the one holding up this pull request. I'm sorry.

My major concern is perhaps academic: Our original design for NonClientIslandWindow was that it would contain a TitlebarControl containing a MinMaxCloseControl and a ContentPresenter. This would allow the windowing layer to remain largely agnostic to the content that was inside it¹. This goes somewhat against that design by requiring that the NCIW subjugate one of its UI elements to another master -- here the TerminalPage.

Now, no initial battle plan survives first contact with the enemy. That much I get. I do wonder, though, if there's a different design that brings us even closer to the CoreTitleBar stuff called out in your link? Maybe NonClientIslandWindow can contain a Canvas, and juxtapose the MinMaxCloseControl over top of the Page, while announcing through an event how much space Page should leave? Giving Page the ability to size the drag bar as necessary? Is this still purely academic, and not a hill worth my dying on? 😄

[1]: we've been entertaining the idea of working with the WinUI 3.0 folks to turn this into an official Xaml hosting edifice for consumers who want to draw in the non-client area, or at least contribute to the design of one.

[beviu]

Ok. No worries for the delay. I don't really know XAML and UWP unfortunately, this PR is definitely not the best way to do things. It looks like the issue will be fixed upstream microsoft/microsoft-ui-xaml#1744 so I should close it now.