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

View height does not update when adjacent container's height changes #145996

Closed
joyceerhl opened this issue Mar 24, 2022 · 8 comments
Closed

View height does not update when adjacent container's height changes #145996

joyceerhl opened this issue Mar 24, 2022 · 8 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority search Search widget and operation issues verified Verification succeeded
Milestone

Comments

@joyceerhl
Copy link
Contributor

Does this issue occur when all extensions are disabled?: Yes

  • VS Code Version: 1.66-insider
  • OS Version: Windows 11

Steps to Reproduce:

  1. Drag outline section into the search activity bar view container
  2. Run a search with the outline section expanded
  3. Collapse the outline search
  4. 🐛 results appear truncated because the list height isn't updated

Recording 2022-03-24 at 10 24 11

@JacksonKearl
Copy link
Contributor

Able to hit this consistently on my Windows vm but have never seen it on Mac. Perhaps a timing bug where we're measuring the container too early.

@JacksonKearl JacksonKearl added bug Issue identified by VS Code Team member as probable bug search Search widget and operation issues labels Apr 6, 2022
@JacksonKearl
Copy link
Contributor

JacksonKearl commented Apr 8, 2022

Ah, doesn't repro when motion is disabled. So the search tree tries to measure its height right when it gets the layoutBody call, but at that moment the animation hasn't finished so it measures the old height instead of the new one. @sbatten could you signal when the animation is over? layoutBody does get passed height information, but search view prefers to let the tree view measure its container because there can be a lot of other elements taking up differing amounts of space in it (whether includes are expanded, replace, search messages, etc). cc @roblourens too

private reLayout(): void {
if (this.isDisposed || !this.size) {
return;
}
const actionsPosition = this.searchConfig.actionsPosition;
this.getContainer().classList.toggle(SearchView.ACTIONS_RIGHT_CLASS_NAME, actionsPosition === 'right');
this.searchWidget.setWidth(this.size.width - 28 /* container margin */);
this.inputPatternExcludes.setWidth(this.size.width - 28 /* container margin */);
this.inputPatternIncludes.setWidth(this.size.width - 28 /* container margin */);
this.tree.layout(); // The tree will measure its container
}
protected override layoutBody(height: number, width: number): void {
super.layoutBody(height, width);
this.size = new dom.Dimension(width, height);
this.reLayout();
}

@JacksonKearl
Copy link
Contributor

Ended up just measuring element height, ended up with a but of a fuzz factor I rolled into "margin".

@roblourens
Copy link
Member

I think this is basically what it used to do, which apparently had an issue: #116182

I'm not sure why measuring the height outside the tree had this issue with scaling but the measurement inside the tree was ok. But could you test it with a zoom factor like in that issue?

@JacksonKearl
Copy link
Contributor

JacksonKearl commented Apr 9, 2022

And I even verified it back then.

But testing now on MacOS with some different scaling settings and my Windows VM across a variety of resolutions (100,125,150,175,200,225,250,400), I haven't been able to spot that coming up.

Though it's perhaps worth noting my eyes aren't excelling at distinguishing nearby horizontal lines at the moment.

@joaomoreno are you able to verify with your setup?

@joaomoreno
Copy link
Member

Seems OK to me on Insiders today.

@joaomoreno
Copy link
Member

joaomoreno commented Apr 13, 2022

Nope, can't scroll more here, despite there being obvious tree rows in there:

image

Repro:

  • Search for new Promise<void>(resolve, no search flags active

Bug first spotted by @bpasero

@joaomoreno joaomoreno reopened this Apr 13, 2022
@bpasero bpasero added the important Issue identified as high-priority label Apr 13, 2022
@bpasero
Copy link
Member

bpasero commented Apr 13, 2022

Marking important as I lost search results to work on because of this.

JacksonKearl pushed a commit that referenced this issue Apr 13, 2022
@rzhao271 rzhao271 added the verified Verification succeeded label Apr 28, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2022
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 important Issue identified as high-priority search Search widget and operation issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

8 participants
@joaomoreno @roblourens @bpasero @sbatten @rzhao271 @JacksonKearl @joyceerhl and others