Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fullscreen container dimension detection when not directly on body #205884

Merged
merged 2 commits into from Feb 23, 2024

Conversation

CGNonofr
Copy link
Contributor

If the workbench is rendered in another container than the body, everything works fine except the fullscreen mode.

This PR make the layout service not rely on the parent element size in fullscreen mode as it doesn't make sense and rely on the body size instead.

I don't think it can't break anything as in the browser, the parent element is currently already the main window body

@bpasero
Copy link
Member

bpasero commented Feb 21, 2024

Is there an associated issue that this PR fixes?

@bpasero bpasero added info-needed Issue requires more information from poster and removed triage-needed labels Feb 21, 2024
@CGNonofr
Copy link
Contributor Author

Is there an associated issue that this PR fixes?

Not at all, it's more a code improvement than a bugfix since no issue can be reproduced in VSCode but it may impact forks and custom builds

Should I still create an issue?

@bpasero
Copy link
Member

bpasero commented Feb 21, 2024

But how can we hard-code a reference to document.body when the parent of the workbench might be something else? I am worried about regressions here actually, but I also do not fully understand the issue yet. If there is a repro, that would help.

Note that your change impacts not only web but also VS Code for desktop.

@CGNonofr
Copy link
Contributor Author

But how can we hard-code a reference to document.body when the parent of the workbench might be something else?

In fullscreen mode, the parent doesn't really matter as it doesn't impact the container size at all, so it doesn't make sense to measure it to layout the container

I am worried about regressions here actually

In both cases (web.main.ts and desktop.main.ts) the given parent is mainWindow.document.body so I don't think it can actually change anything

but I also do not fully understand the issue yet. If there is a repro, that would help.

It's not simple to provide a repro

I'm integrating VSCode oss in a webpage between a header and a footer. Here's the result in fullscreen (an empty area at the bottom whose size is the height of the footer and the header combined):
Screenshot from 2024-02-21 17-51-41

Note that your change impacts not only web but also VS Code for desktop.

Indeed, I was confused by the browser directory

@bpasero bpasero removed the info-needed Issue requires more information from poster label Feb 22, 2024
@bpasero bpasero added this to the March 2024 milestone Feb 22, 2024
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the issue now, I think having a comment to explain this would be good.

src/vs/workbench/browser/layout.ts Outdated Show resolved Hide resolved
Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>
@bpasero bpasero merged commit 0f282e2 into microsoft:main Feb 23, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants