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

automaticLayout broken on master? #1855

Closed
orta opened this issue Mar 2, 2020 · 9 comments · Fixed by microsoft/vscode#93630
Closed

automaticLayout broken on master? #1855

orta opened this issue Mar 2, 2020 · 9 comments · Fixed by microsoft/vscode#93630
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug
Milestone

Comments

@orta
Copy link
Contributor

orta commented Mar 2, 2020

Repro:

  • Working: Drag the sidebar left of right on 3.7.5
  • Not Working: 3.8.3

3.7.5 is from about a month ago, 3.8.3 is from yesterday.

They both have the same outer code for the playground, but refer to different Monaco-editors and Monaco TypeScripts. I'll know I can move off automaticLayout by telling the editor when the sidebar changes, but I'll leave it this way for a few days so there's a a working repro.

@alexdima
Copy link
Member

alexdima commented Mar 2, 2020

It is possible that microsoft/vscode#90111 has broken things. On master the editor now tries to use a MutationObserver to decide if it should rescan the container dom node.

Do you have a pointer to the outer code?

@alexdima alexdima added the info-needed Issue requires more information from poster label Mar 2, 2020
@orta
Copy link
Contributor Author

orta commented Mar 3, 2020

The code is a little complex because there's a few moving parts...

Monaco lives in a #monaco-editor-embed which is a child of #playground-container (because it also has the toolbar of the same width) - so changes in width are applied 2 divs up, and then the toolbar and monaco would be resized to fit using CSS block's usual "fill the whole thing horizontally"

@alexdima
Copy link
Member

alexdima commented Mar 3, 2020

Aha! I believe this is due to the usage of calc here -- https://github.com/microsoft/TypeScript-Website/blob/0babe4b37d3d82485138d908065798926acbd018/packages/playground/src/index.ts#L212-L213

So the height of the container can change without any properties being changed... The MutationObserver listens to properties getting changed.

@orta
Copy link
Contributor Author

orta commented Mar 3, 2020

Nice sleuthing, I'm open to this issue being closed - I'm not really sure what else you could do from Monaco's side for something like this other than have an optional fallback to the old mode

@alexdima
Copy link
Member

alexdima commented Mar 3, 2020

Well, I think we're using the wrong concept. I just found https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver

@alexdima alexdima added bug Issue identified by VS Code Team member as probable bug and removed info-needed Issue requires more information from poster labels Mar 3, 2020
@nrayburn-tech
Copy link
Contributor

Resize Observer is the best way to do it, but lacks Safari support and I think at the time Monaco was still supporting IE, which also doesn’t support Resize Observer.
Maybe we should implement Resize Observer and then use the interval timer as a fallback for Safari?

@rcjsuen
Copy link
Contributor

rcjsuen commented Mar 27, 2020

See #1884 for another issue that seems to be related to automaticLayout.

@nrayburn-tech
Copy link
Contributor

I’ll try to get a PR submitted this weekend to work as described above.

@aeremin
Copy link

aeremin commented Apr 24, 2020

Any chance for a new bugfix release including the fix above?
AFAIU version in master branch of this repo still has this problem (but maybe I don't understand how microsoft/vscode and microsoft/monaco-editor relation works).

@vscodebot vscodebot bot locked and limited conversation to collaborators May 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants