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

revamp resize logic in demo #4113

Open
jerch opened this issue Sep 13, 2022 · 4 comments · Fixed by #4233
Open

revamp resize logic in demo #4113

jerch opened this issue Sep 13, 2022 · 4 comments · Fixed by #4233
Labels
area/demo help wanted type/bug Something is misbehaving

Comments

@jerch
Copy link
Member

jerch commented Sep 13, 2022

If the browser has a zoomlevel (prolly for any pixelDeviceRatio != 1) the element resizing in client.ts does not yield the same values as FitAddon.fit leading to additional resize adjustments done by the fit addon (see #4112 (comment)). This prolly also happens in the opposite direction for shrinking and a zoomlevel <100% (not tested though).

We might want to fix that, so ppl dont c&p faulty code resulting in nonsense resize events with poor performance in the end.

(@Tyriar - you might want to test if something similar happens in vscode, if the pixelDeviceRatio is not perfectly 1).

@Tyriar
Copy link
Member

Tyriar commented Sep 13, 2022

These inconsistencies are mainly a result of #702. I don't think it happens in VS Code, but it also reaches into private API to handle resizing similar to the fit addon.

@jerch
Copy link
Member Author

jerch commented Sep 13, 2022

So should we fix that in client.ts as well? I dont see an urgent need (know how to work around), just wonder if it makes embedders life harder, if they just c&p stuff from client.ts.

@Tyriar
Copy link
Member

Tyriar commented Sep 13, 2022

Yep we should fix, resize as a whole is a bit of a mess due to no dimensions being exposed though. VS Code has its own inconsistent size calculation and I suspect other embedders do something similar when fit doesn't meet their needs: https://github.com/microsoft/vscode/blob/9b80ed652434a6e82b3ac28c1a9a01132c8faea3/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts#L591-L653

@jerch jerch changed the title demo.ts col enlarge issue revamp resize logic in demo Sep 13, 2022
@jerch
Copy link
Member Author

jerch commented Sep 13, 2022

So to summarize - lets revamp resize logic in client.ts, to it is in line with fit addon.

Tyriar added a commit to Tyriar/xterm.js that referenced this issue Oct 27, 2022
A double resize could happen due to the mismatch in canvas vs device
pixel widths

Fixes xtermjs#4113
@Tyriar Tyriar added this to the 5.1.0 milestone Oct 27, 2022
@Tyriar Tyriar reopened this Oct 27, 2022
@Tyriar Tyriar modified the milestones: 5.1.0, 5.2.0 Dec 19, 2022
@Tyriar Tyriar removed this from the 5.2.0 milestone Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/demo help wanted type/bug Something is misbehaving
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants