Allow tabs to wrap to multi-line #106448

jmannanc · 2020-09-11T03:48:04Z

This PR fixes #70413

bpasero

@sneakyfish5 kudos, this is a very elegant solution given it only requires CSS changes 👍

Some feedback after a quick review:

can we expose the setting from IEditorPartConfiguration? This should make it much easier to read the setting and react to changes (e.g. from

vscode/src/vs/workbench/browser/parts/editor/tabsTitleControl.ts

Line 124 in 82d953a

this._register(this.accessor.onDidEditorPartOptionsChange(e => {

)
the color introduced for the border needs to be themable (i.e. cannot be hardcoded in the CSS) and needs some testing together with the existing tab border colors that we have, I am not sure it will work in all cases
when tabs wrap they do not align nicely below the editor action bar (see screenshot below), can we possibly improve this?
there seems to be a bug when trying to drop a tab after the last tab, the drop feedback spans multiple rows (see video below)

jmannanc · 2020-09-11T18:07:13Z

Most of the CSS work was done by people in the original issue; I just pieced it together into a setting, so huge props to them 😃

can we expose the setting from IEditorPartConfiguration?

Done

the color introduced for the border needs to be themable (i.e. cannot be hardcoded in the CSS) and needs some testing together with the existing tab border colors that we have, I am not sure it will work in all cases

Done, I used TAB_BORDER and applied it to the bottom-border of tabs, let me know if that wasn't the correct theme part to use.

when tabs wrap they do not align nicely below the editor action bar (see screenshot below), can we possibly improve this?
there seems to be a bug when trying to drop a tab after the last tab, the drop feedback spans multiple rows (see video below)

I think a neat solution for both of these problems is provided by @fraigo in #70413 (comment), let me know what you think. Here's a visual representation of it included with all the other changes:

Otherwise, setting a max-width while using flex-grow might be another option?

bpasero · 2020-09-14T17:58:52Z

@sneakyfish5

I think a neat solution for both of these problems is provided by @fraigo in #70413 (comment), let me know what you think. Here's a visual representation of it included with all the other changes:

This looks very similar to tabSizing: shrink where tabs shrink to a fixed size when too many are opened. What I do not like about the presented solution is that tabs now consume the entire width even when available space is there to show them normally. Ideally only tabs at the far right end would align vertically by growing or shrinking, right? Not sure how to get that done in pure CSS though... maybe there is a flex property that can help?

fraigo · 2020-09-14T18:43:46Z

@bpasero @sneakyfish5 One solution to make the last row use the space better is adding a 'hidden' flex space after the last tab (in this case, adding space at the end of the tab container. I think 33% is a good value, could be changed to any other for the try the best fit.

.monaco-workbench .part.editor > .content .editor-group-container > .title.tabs> .tabs-and-actions-container.multi-line-tabs-container > .monaco-scrollable-element > .tabs-container::after {
	content: "";
	flex: 1 0 33%;
}

jmannanc · 2020-09-14T21:02:57Z

After some digging as well I couldn't figure out how to make the tabs stay the same size and align properly at the very far end, aside from setting a max-width to an arbitrary percentage, which is a pretty hacky fix. I like the solution that @fraigo is presenting. When testing it out flex: 1 0 0; worked better for me and made a smoother transition to the next line when opening new tabs, would there be any problem with that?

fraigo · 2020-09-15T00:22:07Z

yes @sneakyfish5, using flex:1 0 0 works fine (even using flex: 1 0 auto should work in this case because there is no content to shrink), using the minimum spanning space.

jmannanc · 2020-09-15T02:54:50Z

Awesome, recorded a little snippet here:

@bpasero is that better?

bpasero · 2020-09-15T07:05:44Z

@sneakyfish5 can you add this solution to this PR for me to play with? Easier then to give some feedback. I will also comment on the tab border then, thanks 👍

jmannanc · 2020-09-15T11:39:50Z

Will do 👍

bpasero · 2020-09-16T06:19:47Z

@sneakyfish5 this works great when many tabs are opened and it wraps but is there a way to not have excessively large tabs when you only have few opened:

?

bpasero · 2020-09-16T06:55:31Z

I pushed a few polish changes to your branch and thought about the use of border: I think conceptually it makes a lot of sense to use tab.border for this because according to the docs: Border to separate tabs from each other so it fits.

However, I think the logic of drawing this border should merge with the existing border we already have for active tabs which also draws a border below tabs:

vscode/src/vs/workbench/browser/parts/editor/tabsTitleControl.ts

Lines 1150 to 1156 in 9dc00c6

 if (activeTabBorderColorBottom) { 

 addClass(tabContainer, 'tab-border-bottom'); 

 tabContainer.style.setProperty('--tab-border-bottom-color', activeTabBorderColorBottom.toString()); 

 } else { 

 removeClass(tabContainer, 'tab-border-bottom'); 

 tabContainer.style.removeProperty('--tab-border-bottom-color'); 

 }

This ensures we are using the same logic for rendering a border and more importantly allows to still have a different border color for the active tab compared to others.

jmannanc · 2020-09-16T15:51:16Z

this works great when many tabs are opened and it wraps but is there a way to not have excessively large tabs when you only have few opened

That's the current issue I'm facing that I'm not sure how to solve. I've tried setting a max-width, but I've realized it doesn't work on smaller window sizes. If anyone has any better ideas, please chime in as I'm definitely not the best at CSS.

However, I think the logic of drawing this border should merge with the existing border we already have for active tabs which also draws a border below tabs

Done, it now only draws a bottom-border for inactive tabs when multi-line tabs are enabled. Is that what you meant?

The commits got a little messy there (sorry about that), but I've fixed merge conflicts and changed it to use classList.toggle instead.

bpasero · 2020-09-16T15:57:52Z

@sneakyfish5 one thing we could maybe try is to do the flex trick conditionally depending on wether the tabs wrap or not? E.g. when you open only a few tabs they would appear normally and only when they wrap we start the trick?

Code wise we know exactly how much space we have and how much space tabs consume around here:

vscode/src/vs/workbench/browser/parts/editor/tabsTitleControl.ts

Line 1317 in 90a9985

 if (this.group.stickyCount > 0 && availableTabsContainerWidth < TabsTitleControl.TAB_SIZES.fit) { 

So we could toggle a similar class depending on how much space tabs consume in the tab container?

jmannanc · 2020-09-16T20:07:07Z

That's definitely possible, I've made it so that the multi-line tabs class only added if the "all tabs width" is greater than the "visible tabs width", and is removed if they are equal and the container height is equal to EDITOR_TITLE_HEIGHT.

I agree this probably isn't the most optimal situation and that it would be preferable to have the tabs be somewhat the same size and also expand to fill the container at the end of the row, but I'm not sure how to accomplish that.

bpasero · 2020-09-17T06:25:12Z

src/vs/workbench/browser/parts/editor/editorDropTarget.ts

@@ -501,6 +502,10 @@ class DropOverlay extends Themable {

 private getOverlayOffsetHeight(): number {
 if (!this.groupView.isEmpty && this.accessor.partOptions.showTabs) {
+ const tablist = document.querySelector(DropOverlay.TABS_SELECTOR) as HTMLElement;


This will not work properly once you have more than one editor group opened because then you will have more than one div.tabs-container.

I suggest an alternate solution where the accessor provides a method to ask for the height of the editor title area. The accessor in this case is the EditorPart which can delegate this to the group in question and then the group can forward this to the title control. And I would only use the offsetHeight property if mutli-line tabs are enabled.

Done, please let me know if I've done this incorrectly.

src/vs/workbench/browser/parts/editor/tabsTitleControl.ts

bpasero · 2020-09-17T06:32:49Z

src/vs/workbench/browser/parts/editor/tabsTitleControl.ts

@@ -1174,6 +1188,10 @@ export class TabsTitleControl extends TitleControl {
 tabContainer.style.backgroundColor = this.getColor(isGroupActive ? TAB_INACTIVE_BACKGROUND : TAB_UNFOCUSED_INACTIVE_BACKGROUND) || '';
 tabContainer.style.boxShadow = '';

+ // bottom border when wrapping multi-line


My suggestion was to reuse the tab-border-bottom helper for this case too and not introduce yet another border. Currently the tab-border-bottom is reserved for active tabs because the border was only used for active tabs, but to ensure we get a consistent look, we should be using the same thing for the separator as well. Otherwise we end up with an active border that is not on the exact same line as the separator border.

Done, I misunderstood what you suggested earlier but hopefully it's correct now?

bpasero · 2020-12-27T10:52:20Z

Actually with a53fe3c found a nice solution via CSS variables that does not require to patch any tab element directly. This makes sure we only have a margin-right for the last tab when wrapping is enabled.

I think this PR is close to be done, I would appreciate some testing so that we are certain this works as intended.

cullenjohnson · 2020-12-29T19:42:40Z

This is the PR I have been waiting for for years. Is there any way I can help get this merged faster? :)

jmannanc · 2020-12-30T07:21:25Z

Hey, sorry again I've been very busy recently. Thank you so much @bpasero for pushing all those fixes.

@cullenjohnson maybe you can help with the testing outlined here, or just in general do any sort of testing that you can think of? If not no worries, I'll be able to get some testing done within the next day or so as well, but of course all help is appreciated.

swinder0161 · 2020-12-30T16:44:26Z

Hi, I am up for testing but where will I get a version to test. Donot have development environment to build myself. I need a Mac version btw. Thanks, Swinder

…

On Wed, Dec 30, 2020, 12:51 PM SneakyFish5 ***@***.***> wrote: Hey, sorry again I've been very busy recently. Thank you so much @bpasero <https://github.com/bpasero> for pushing all those fixes. @cullenjohnson <https://github.com/cullenjohnson> maybe you can help with the testing outlined here <#106448 (comment)>, or just in general do any sort of testing that you can think of? If not no worries, I'll be able to get some testing done within the next day or so as well, but of course all help is appreciated. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#106448 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBBZ3QINBMPYNUEI2DSZDSXLIIFANCNFSM4RGQAQ6A> .

jmannanc · 2021-01-01T22:18:47Z

Sorry for the delay again, had a lot of issues with node-gyp when building but was finally able to get it working and do the tests. I did a run through of all things outlined in #106448 (comment) as well as:

Anything I could think of related to pinned tabs
tabCloseButton setting
titleScrollbarSizing setting
Scroll to switch tabs
Theming

I also tested a few more things that I can't quite remember now, however they all worked, and everything looked good in regards to the editor height and layout. If there are any more tests that need to be completed, please let me know.

bpasero · 2021-01-02T12:38:08Z

Thanks a lot @sneakyfish5 I plan to merge this soon to get it into insiders during January for more coverage.

bpasero · 2021-01-04T09:38:11Z

Thanks @sneakyfish5 for pushing this forward 🥂

ghost · 2021-01-04T10:29:27Z

This issue seems to be merged now - Am I right?
When is the next official release of VSCode?

bpasero · 2021-01-04T10:46:27Z

@Tschebbe if you don't want to wait until your next stable release in early February, simply switch to VSCode insiders to get this as early as tomorrow: https://code.visualstudio.com/insiders/

jmannanc · 2021-01-05T00:44:21Z

Thank you as well @bpasero for your continuous effort and assistance in pushing this PR forward!

DynDux · 2021-01-06T00:12:18Z

I think I found a pure css solution for the problem with last tab spanning the whole width!
Just add the following style when tabSizing = fit:

.tabs-container::after{ content: ''; flex-grow: 20; }

This will create an additional tab pseudo-element, that grows 20 times larger than the other tabs. It could be an even higher number, but I think 20 should be sufficient.
The margin-right on the last-child ist still needed though.

bpasero · 2021-01-06T07:02:22Z

@DynDux @sneakyfish5 great, I suggest we take this idea to a new PR and test it out there, what do you think?

DynDux · 2021-01-06T09:22:23Z

Totally ok for me! Just thought it belongs to this PR because this is how it should have been resolved in first place. ;-)
Please do whatever the right workflow is. Thank you so much!

bpasero · 2021-01-06T09:30:22Z

I have added your suggested solution to #113801.

qs-labs · 2021-02-04T19:35:46Z

Great work to those involved! Pumped to have this feature 👏 🎉 👏

Allow tabs to wrap to multi-line

a64e0f6

vscode-triage-bot assigned bpasero Sep 11, 2020

Merge branch 'master' into wrap-tabs

b52a17a

bpasero requested changes Sep 11, 2020

View reviewed changes

bpasero added this to the On Deck milestone Sep 11, 2020

jmannanc force-pushed the wrap-tabs branch from d319228 to 3334251 Compare September 11, 2020 21:29

Address feedback

8139024

jmannanc force-pushed the wrap-tabs branch 2 times, most recently from d9b6152 to 8139024 Compare September 14, 2020 02:26

Add hidden space after last tab

34dd6cf

bpasero added 3 commits September 16, 2020 08:26

Merge branch 'master' into wrap-tabs

717ab2d

some polish for multi-line wrap css class

417e97b

some more polish

c290d7c

Merge branch 'master' into wrap-tabs

f0fed35

jmannanc force-pushed the wrap-tabs branch from 6b583ac to e55c965 Compare September 16, 2020 15:52

bpasero self-requested a review September 17, 2020 06:21

bpasero requested changes Sep 17, 2020

View reviewed changes

introduce css variable for margin-right trick

a53fe3c

bpasero modified the milestones: On Deck, January 2021 Dec 27, 2020

bpasero added 3 commits December 27, 2020 13:10

add border to separate tabs when wrapping

91c6333

💄

46bdb34

Merge branch 'master' into wrap-tabs

c853f2b

bpasero marked this pull request as ready for review January 3, 2021 09:36

bpasero added 3 commits January 3, 2021 10:36

Merge branch 'master' into wrap-tabs

4f948dc

rename setting

ff98891

💄 layout method

e6ee5f6

bpasero merged commit baf15f1 into microsoft:master Jan 4, 2021

jmannanc deleted the wrap-tabs branch January 5, 2021 00:41

swinder0161 mentioned this pull request Jan 5, 2021

Tab wrap - allow to change height of tabs #113832

Closed

bpasero mentioned this pull request Jan 6, 2021

Tab wrap - tab should not fill all spare space when wrap to new line #113801

Closed

microsoft locked and limited conversation to collaborators Feb 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow tabs to wrap to multi-line #106448

Allow tabs to wrap to multi-line #106448

jmannanc commented Sep 11, 2020

bpasero left a comment

jmannanc commented Sep 11, 2020 •

edited

Loading

bpasero commented Sep 14, 2020

fraigo commented Sep 14, 2020

jmannanc commented Sep 14, 2020

fraigo commented Sep 15, 2020

jmannanc commented Sep 15, 2020

bpasero commented Sep 15, 2020

jmannanc commented Sep 15, 2020

bpasero commented Sep 16, 2020

bpasero commented Sep 16, 2020 •

edited

Loading

jmannanc commented Sep 16, 2020

bpasero commented Sep 16, 2020

jmannanc commented Sep 16, 2020

bpasero Sep 17, 2020

jmannanc Sep 17, 2020

bpasero Sep 17, 2020

jmannanc Sep 17, 2020

bpasero commented Dec 27, 2020

cullenjohnson commented Dec 29, 2020

jmannanc commented Dec 30, 2020

swinder0161 commented Dec 30, 2020 via email

jmannanc commented Jan 1, 2021

bpasero commented Jan 2, 2021

bpasero commented Jan 4, 2021

ghost commented Jan 4, 2021

bpasero commented Jan 4, 2021

jmannanc commented Jan 5, 2021

DynDux commented Jan 6, 2021

bpasero commented Jan 6, 2021

DynDux commented Jan 6, 2021

bpasero commented Jan 6, 2021

qs-labs commented Feb 4, 2021

Allow tabs to wrap to multi-line #106448

Allow tabs to wrap to multi-line #106448

Conversation

jmannanc commented Sep 11, 2020

bpasero left a comment

Choose a reason for hiding this comment

jmannanc commented Sep 11, 2020 • edited Loading

bpasero commented Sep 14, 2020

fraigo commented Sep 14, 2020

jmannanc commented Sep 14, 2020

fraigo commented Sep 15, 2020

jmannanc commented Sep 15, 2020

bpasero commented Sep 15, 2020

jmannanc commented Sep 15, 2020

bpasero commented Sep 16, 2020

bpasero commented Sep 16, 2020 • edited Loading

jmannanc commented Sep 16, 2020

bpasero commented Sep 16, 2020

jmannanc commented Sep 16, 2020

bpasero Sep 17, 2020

Choose a reason for hiding this comment

jmannanc Sep 17, 2020

Choose a reason for hiding this comment

bpasero Sep 17, 2020

Choose a reason for hiding this comment

jmannanc Sep 17, 2020

Choose a reason for hiding this comment

bpasero commented Dec 27, 2020

cullenjohnson commented Dec 29, 2020

jmannanc commented Dec 30, 2020

swinder0161 commented Dec 30, 2020 via email

jmannanc commented Jan 1, 2021

bpasero commented Jan 2, 2021

bpasero commented Jan 4, 2021

ghost commented Jan 4, 2021

bpasero commented Jan 4, 2021

jmannanc commented Jan 5, 2021

DynDux commented Jan 6, 2021

bpasero commented Jan 6, 2021

DynDux commented Jan 6, 2021

bpasero commented Jan 6, 2021

qs-labs commented Feb 4, 2021

jmannanc commented Sep 11, 2020 •

edited

Loading

bpasero commented Sep 16, 2020 •

edited

Loading