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

Should sticky scroll apply the focused class to the sticky scroll items? #207657

Closed
TylerLeonhardt opened this issue Mar 14, 2024 · 1 comment · Fixed by #207703
Closed

Should sticky scroll apply the focused class to the sticky scroll items? #207657

TylerLeonhardt opened this issue Mar 14, 2024 · 1 comment · Fixed by #207703
Assignees
Labels
insiders-released Patch has been released in VS Code Insiders tree-sticky-scroll Sticky Scroll in Trees
Milestone

Comments

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Mar 14, 2024

I was surprised to see the sticky scroll element have the focused class:

<div class="monaco-tree-sticky-row monaco-list-row focused" data-index="29" data-parity="odd" id="list_id_6_29" role="option" aria-label="tasks.json, tasks.json" aria-selected="false" aria-expanded="true" style="top: 0px; height: 24px; line-height: 24px;">

caused by:

this.stickyScrollFocus.updateElements(elements, state);

mostly because, well, it doesn't feel "focused" to me and because it is focused, the Action bars get colored using --vscode-quickInputList-focusIconForeground instead of --vscode-icon-foreground.

To me the icons should probably just use --vscode-icon-foreground... although, I admit, I do want the icons to show up while it's sticky... I haven't thought long about how I would do that if we stopped marking these rows as "focused"

@benibenj
Copy link
Contributor

Sticky Scroll supports being focused which is why an element can have .focused. However, we can improve it such that it is only set when sticky scroll actively has focus (compared to the list which has .focused set even when it doesn't have active focus). I've made an attempt to fix this and from my testing it seems to work well now, but there is a lot of complexity to list and sticky scroll focus so please let me know if you see anything weird regarding this.

benibenj added a commit that referenced this issue Mar 14, 2024
@vscodenpa vscodenpa added the unreleased Patch has not yet been released in VS Code Insiders label Mar 14, 2024
@vscodenpa vscodenpa added this to the March 2024 milestone Mar 14, 2024
TylerLeonhardt added a commit that referenced this issue Mar 14, 2024
* Make sure action bar is visible in sticky (change needed after #207657)
* Go back to list style of highlights (#207744)
* When you click on a separator, we scroll it up to the top of the list and focus the first item
* Hide the twistie is a programmatic way instead

Fixes #207744
TylerLeonhardt added a commit that referenced this issue Mar 14, 2024
* Make sure action bar is visible in sticky (change needed after #207657)
* Go back to list style of highlights (#207744)
* When you click on a separator, we scroll it up to the top of the list and focus the first item
* Hide the twistie is a programmatic way instead

Fixes #207744
@vscodenpa vscodenpa added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Mar 15, 2024
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
insiders-released Patch has been released in VS Code Insiders tree-sticky-scroll Sticky Scroll in Trees
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants