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

Sticky Scroll Tree View #198320

Merged
merged 27 commits into from
Nov 21, 2023
Merged

Sticky Scroll Tree View #198320

merged 27 commits into from
Nov 21, 2023

Conversation

benibenj
Copy link
Contributor

Sticky Scroll in Trees! #161207

@benibenj benibenj self-assigned this Nov 15, 2023
handling of tree node indices and rendered elements
@benibenj benibenj changed the title Improved Sticky Scroll Functionality in Tree View Sticky Scroll Tree View Nov 15, 2023
@benibenj
Copy link
Contributor Author

@alexr00 Please have a look at the changes I made in TreeView.ts. The change is that a TreeItem can now be rendered multiple times. A rendered item can be identified by it's handle and templateData.

alexr00
alexr00 previously approved these changes Nov 16, 2023
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

treeView.ts changes look good!

@benibenj benibenj requested a review from joaomoreno November 16, 2023 11:14
@benibenj benibenj marked this pull request as ready for review November 16, 2023 11:14
@benibenj benibenj added this to the November 2023 milestone Nov 16, 2023
src/vs/base/browser/ui/list/listView.ts Outdated Show resolved Hide resolved
src/vs/base/browser/ui/tree/media/stickyScroll.css Outdated Show resolved Hide resolved
src/vs/base/browser/ui/tree/media/stickyScroll.css Outdated Show resolved Hide resolved
src/vs/base/browser/ui/tree/stickyScroll.ts Outdated Show resolved Hide resolved
src/vs/base/browser/ui/tree/stickyScroll.ts Outdated Show resolved Hide resolved
src/vs/platform/list/browser/listService.ts Outdated Show resolved Hide resolved
src/vs/platform/list/browser/listService.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/views/treeView.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/views/treeView.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/views/treeView.ts Outdated Show resolved Hide resolved
improved sticky scrolling configuration, and refactored code for
readability and efficiency.
@benibenj benibenj requested a review from joaomoreno November 17, 2023 09:54
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

Getting there, and feels really good so far.

Here's a bug:

Screen.Recording.2023-11-17.at.14.56.29.mov

Also, did you think about the "now renderers need to worry about renderElement being called twice (once normal and once sticky)" problem that we talked about? We only fixed it on the TreeRenderer._renderedElements property, though this is an established pattern across many tree renderers in the workbench. We need to go around those renderers and understand if they will hit issues as well.

@benibenj
Copy link
Contributor Author

benibenj commented Nov 19, 2023

This is the list of Renderers that probably need to be adapted:

  • TreeView
  • FilesRenderer
  • ResourceMarkersRenderer
  • ActionButtonRenderer
  • InputRenderer
  • ResourceRenderer: No change required, uses template as identifier (is unique across renders of same item)
  • ChatListItemRenderer: No change required

// cc @lszomoru

@benibenj benibenj enabled auto-merge November 20, 2023 08:15
@benibenj benibenj requested a review from joaomoreno November 20, 2023 08:16
@joaomoreno
Copy link
Member

Very cool, getting close.

I think the focus revealing feature should be aware of sticky scrolling somehow. Notice how as I press UpArrow the elements behind sticky scrolling do not get revealed:

Screen.Recording.2023-11-20.at.16.39.24.mov

The same underlying issue causes the rename action on sticky scroll elements to break. In the following recording, the sticky row goes into "editing" mode (I guess this is a bug in itself... should renderers be aware that they are rendering for sticky scroll and avoid doing specific things, like rendering an editable element, in the explorer's case?), and the actual row fails to be properly revealed:

Screen.Recording.2023-11-20.at.16.47.03.mov

Also, the sticky scroll container seems to overlay the list focus outline:

image

We should also make the sticky rows accessible. They should be focusable (via Shift Tab, when focused in the tree) and navigable using the arrow keys.

@benibenj benibenj merged commit 86f09d8 into main Nov 21, 2023
6 checks passed
@benibenj benibenj deleted the benibenj/treeStickyScroll branch November 21, 2023 16:30
@github-actions github-actions bot locked and limited conversation to collaborators Jan 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants