Fix overflow behavior of sidebars #13483

tortmayr · 2024-03-12T23:24:24Z

What it does

Ensure that last visible element also gets added to ... overflow menu if there is not enough space
(Normally this only happens if there is also a top menu configured for the side bar. Otherwise application specific
min height restrictions typically prevent resizing that far)
Remove special behavior for only one overflowing element
- This behavior would always add a second element to the overflow menu if only one element is currently overflowing. This did not work for the case where only two tabs where open in the first place. In addition, it unnecessarily hide a tab in some cases even if there was enough space to render it. This is inline with the behavior of VS Code.
Also consider potential tab margins in overflow computation

Contributed on behalf of STMicroelectronics

How to test

Precondition:
To fully test this you should have ta at least one top menu in the sidebar. Otherwise application specific minimum size settings might prevent you from reszing the window to a point where all tabs are hidden.

I did this by adding the following code to the configure method of the CommonFrontendContribution

app.shell.leftPanelHandler.addTopMenu({
            id: 'settings-menu1',
            iconClass: 'codicon codicon-settings-gear',
            title: nls.localizeByDefault(CommonCommands.MANAGE_CATEGORY),
            menuPath: MANAGE_MENU,
            order: 1,
        });
app.shell.leftPanelHandler.addTopMenu({
            id: 'settings-menu2',
            iconClass: 'codicon codicon-settings-gear',
            title: nls.localizeByDefault(CommonCommands.MANAGE_CATEGORY),
            menuPath: MANAGE_MENU,
            order: 1,
        });

The test the overflow behavior by resizing the window.
In particular you should ensure that the previously bugged corner cases are now working as expected

Overflow with only two open tabs.
On master the overflow behavior does not work as expected. Instead of showing the
... menu the view icons just flicker.
Ensure that the last not-hidden tab (i.e. the first tab overall) also gets hidden if there is not enough space
Ensure that overflow is working correctly if margins are added via custom styling
You can do this in the dev tools by adding he following rule to the inspector stylesheet

  .p-TabBar-tab{
    margin-top:15px;
    margin-bottom:15px;
}

If the inspector-stylesheet does not show up via ctrl+p you might have to force the creation by navigating to the
elements tabs and applying a new style via the toolbox in the styles section:

After that the stylesheet should be present.

See also this short demo video for reference:

overflow-demo.mp4

Follow-ups

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

tsmaeder · 2024-03-26T10:49:46Z

@tortmayr I still don't know how to reproduce the problem (on Windows). Is this a Linux-only thing?

tortmayr · 2024-03-26T13:48:48Z

app.shell.leftPanelHandler.addTopMenu({
            id: 'settings-menu2',
            iconClass: 'codicon codicon-settings-gear',
            title: nls.localizeByDefault(CommonCommands.MANAGE_CATEGORY),
            menuPath: MANAGE_MENU,
            order: 1,
        });

I have not tested it under Windows, but the issue was originally reported by an adopters that uses Windows so IMO it should be a cross-platform issue.

I have recorded videos with more detailled steps on how to reproduce the bug on master, on how the fixed version based on this PR looks like:
Master:

master_demo.mp4

Fixed:

pr_demo.mp4

tsmaeder · 2024-03-26T15:48:13Z

@tortmayr how do you get the two gear icons at the top? That's not happening for me.

tortmayr · 2024-03-26T21:55:42Z

It's explained in the How to test section:

To fully test this you should add (at least one) top menu to the sidebar. Otherwise application specific minimum size settings might prevent you from reszing the window to a point where all tabs are hidden.

I did this by adding the following code to the configure method of the CommonFrontendContribution

app.shell.leftPanelHandler.addTopMenu({
            id: 'settings-menu1',
            iconClass: 'codicon codicon-settings-gear',
            title: nls.localizeByDefault(CommonCommands.MANAGE_CATEGORY),
            menuPath: MANAGE_MENU,
            order: 1,
        });
app.shell.leftPanelHandler.addTopMenu({
            id: 'settings-menu2',
            iconClass: 'codicon codicon-settings-gear',
            title: nls.localizeByDefault(CommonCommands.MANAGE_CATEGORY),
            menuPath: MANAGE_MENU,
            order: 1,
        });

tsmaeder · 2024-03-27T09:56:37Z

@tortmayr generally, I much prefer written "steps to reproduce" over a video. In a video, it's hard to discern what you mean and to follow the steps. Also, I can't copy/paste text out of a video, so I might make mistakes following the video. You have even written the text, why not just paste it here?

tortmayr · 2024-03-27T10:24:57Z

@tortmayr generally, I much prefer written "steps to reproduce" over a video. In a video, it's hard to discern what you mean and to follow the steps. Also, I can't copy/paste text out of a video, so I might make mistakes following the video. You have even written the text, why not just paste it here?

Most of this was already written down in the How to test section of the PR description.
Nevertheless, I have augmented the PR description and should now cover all steps to reproduce.

@tortmayr when I do ctrl-p=>inspector-stylesheet, nothing happens in my Theia/Electron. How can I test this?

If the inspector-stylesheet does not show up via ctrl+p you might have to force the creation by navigating to the
elements tabs and applying a new style via the toolbox in the styles section (does not matter to which element):

After that the stylesheet should be present.

tsmaeder · 2024-03-27T10:31:48Z

packages/core/src/browser/shell/tab-bars.ts

@@ -1231,28 +1233,20 @@ export class SideTabBar extends ScrollableTabBar {
                    if (iconElements.length === 1) {
                        const icon = iconElements[0];
                        rd.iconSize = { width: icon.clientWidth, height: icon.clientHeight };
-                        actualWidth += icon.clientHeight + paddingTop + paddingBottom + this.tabRowGap;


Do we not care about the tabRowGap anymore? Wouldn't it be relevant?

tsmaeder · 2024-03-27T10:32:08Z

packages/core/src/browser/shell/tab-bars.ts

                    const tabStyle = window.getComputedStyle(hiddenTab);
-                    const paddingTop = parseFloat(tabStyle.paddingTop!);
-                    const paddingBottom = parseFloat(tabStyle.paddingBottom!);
+                    const paddingTop = tabStyle.paddingTop ? parseFloat(tabStyle.paddingTop) : 0;


Was this broken before? In what way?

packages/core/src/browser/shell/side-panel-handler.ts

tsmaeder · 2024-03-27T10:43:51Z

packages/core/src/browser/shell/tab-bars.ts

                            rd.visible = false;
                            if (overflowStartIndex === -1) {
                                overflowStartIndex = i;
                            }
                        }
+                        // Add the tab row gap to the actual width after the visibility check
+                        actualWidth += marginBottom + this.tabRowGap;


Why does paddingBottom get added before the visibility check, but "marginBottom" afterwards?

tsmaeder · 2024-03-27T13:01:08Z

I found what I believe a new problem: here's what's happening:

Open exactly two views in the sidebar; the code is patched to have the two gear icons at the top
Reduce the height of the window until the lower view icon starts to disappear
Observe: the first (upper) view icon disappears as well after the first one disappears

I'm not sure whether I still move the mouse up after the first one disappears or whether the icon disappears without moving another pixel. It's hard to tell.

tsmaeder · 2024-03-27T13:02:31Z

@tortmayr here's a video showing the problem:

2024-03-27.13-56-47.mp4

tsmaeder · 2024-03-27T13:05:24Z

The other problem seem fixed with this PR.

tortmayr · 2024-03-29T09:30:37Z

@tsmaeder Thanks for reviewing, I will look into the bug report.

- Ensure that last visible element also gets added to `...` overflow menu if there is not enough space - Remove special behavior for only one overflowing element - This behavior would always add a second element to the `overflow` menu if only one element is currently overflowing. This did not work for the case where only two tabs where open in the first place. In addition, it unnecessarily hide a tab in some cases even if there was enough space to render it. - Also consider potential tab margins in overflow computation Fixes eclipse-theia#13482 Contributed on behalf of STMicroelectronics

- Extract logic to compute and hide hidden tabs into `hideOverflowingTabs` method - Invoke this method from `computeOverflowingTabsData`. At this point the actual tabs bar is already rendered so we can use the actual position/height information to compute overflowing tabs instead of manually composing the available height with the hidden tabs bar in `updateTabs` - Update `AdditionalViewsMenuWidget` to use a dedicated menu path for each side. This ensures that only the hidden tabs of the corresponding sidebar are displayed when clicking the '...' button

tortmayr · 2024-05-10T14:46:45Z

@tsmaeder I finally got time to work on this again. Sorry for t

he long turnaround.

Regarding the problem you encountered:
This is technically not a bug. If only the last tab is overflowing, we hide the tab and render the additional views widget instead.
The additional views widget has the same size as the previously hidden tab. So in the end we end up with almost the same remaining space as before. For instance lets assume the available height is 300px, the tab height is 48 and the rowgap is 4.
If we now resize to 299px, we hide the last tab and render the additional views menu instead. So this almost nullfies the height gain as we gain 52 px by hiding the tab (height + rowgap) and then lose 48px by rendering the additional menu. So in practice when resizing you have a 4px range in which only the last tab gets hidden, anything higher than that and the second to last tab is hidden as well.

Since this behavior is inline with what VS Code does I opted for not changing it:

vscode.mp4

However, I don't have a strong opinion on that. If you think its better to adjust this and e.g. always hide the last two tabs if we enter overflowing mode I'm happy to adjust this.

In general, I reworked the entire hidden tabs calculation and moved it into the computeOverflowingTabsData method.
At this point the actual tab bar is already rendered and we can use the actual size/position information to compute hidden tabs.
This remove the need to manually compute available and actual size in updateTabs using the information from the hidden tab content node. And we no longer have to fiddle around with extracting margin,borderWidth etc. from the computed styles.
In general this approach is more robust and can handle arbitrary custom styling.

In addition, I encountered a bug with the AdditionalViewMenuWidget. Both the widget for the left and right tab bar used the same menu path. As a consequence hidden tabs from one widget leaked into the widget of the other side.
E.g. In the left side bar ViewA and ViewB are hidden, in the right side bar ViewC is hidden.
Then both menu widgets would offer all views as options (ViewA, ViewB, ViewC).

After my change this works now correctly and the left menu widget only has View A and B and the right only has View C.

JonasHelming · 2024-05-30T12:04:01Z

@tsmaeder Ping

tortmayr requested a review from tsmaeder March 12, 2024 23:24

tortmayr force-pushed the tortmayr/gh-13482 branch from 2b3bc77 to c0cc040 Compare March 26, 2024 21:56

tsmaeder reviewed Mar 27, 2024

View reviewed changes

tortmayr added 2 commits May 5, 2024 23:24

tortmayr force-pushed the tortmayr/gh-13482 branch from c0cc040 to b7fd720 Compare May 10, 2024 14:16

tortmayr requested a review from tsmaeder May 10, 2024 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix overflow behavior of sidebars #13483

Fix overflow behavior of sidebars #13483

tortmayr commented Mar 12, 2024 •

edited

tsmaeder commented Mar 26, 2024

tortmayr commented Mar 26, 2024

tsmaeder commented Mar 26, 2024

tortmayr commented Mar 26, 2024

tsmaeder commented Mar 27, 2024

tortmayr commented Mar 27, 2024

tsmaeder Mar 27, 2024

tsmaeder Mar 27, 2024

tsmaeder Mar 27, 2024

tsmaeder commented Mar 27, 2024

tsmaeder commented Mar 27, 2024

tsmaeder commented Mar 27, 2024

tortmayr commented Mar 29, 2024

tortmayr commented May 10, 2024 •

edited

JonasHelming commented May 30, 2024

Fix overflow behavior of sidebars #13483

Are you sure you want to change the base?

Fix overflow behavior of sidebars #13483

Conversation

tortmayr commented Mar 12, 2024 • edited

What it does

How to test

Follow-ups

Review checklist

Reminder for reviewers

tsmaeder commented Mar 26, 2024

tortmayr commented Mar 26, 2024

tsmaeder commented Mar 26, 2024

tortmayr commented Mar 26, 2024

tsmaeder commented Mar 27, 2024

tortmayr commented Mar 27, 2024

tsmaeder Mar 27, 2024

Choose a reason for hiding this comment

tsmaeder Mar 27, 2024

Choose a reason for hiding this comment

tsmaeder Mar 27, 2024

Choose a reason for hiding this comment

tsmaeder commented Mar 27, 2024

tsmaeder commented Mar 27, 2024

tsmaeder commented Mar 27, 2024

tortmayr commented Mar 29, 2024

tortmayr commented May 10, 2024 • edited

JonasHelming commented May 30, 2024

tortmayr commented Mar 12, 2024 •

edited

tortmayr commented May 10, 2024 •

edited