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

Fix rendering of popup scrollbar #9499

Closed

Conversation

flinesse
Copy link

@flinesse flinesse commented Feb 2, 2024

Fixes #9498 and keeps #4313's intention (I believe) of repurposing the border as scrollbar track.

image

@kirawi kirawi added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much A-helix-term Area: Helix term improvements A-gui Area: Helix gui improvements labels Feb 2, 2024
@ath3
Copy link
Contributor

ath3 commented Feb 3, 2024

If i see correctly this breaks the colors of borders on the right side of popups.
Also it hardcodes the right border to a specific border style.
I think i know where the issue is and how to fix it, let me make a pr and see if it works for you.

@ath3
Copy link
Contributor

ath3 commented Feb 3, 2024

PR is #9508

@flinesse
Copy link
Author

flinesse commented Feb 8, 2024

If i see correctly this breaks the colors of borders on the right side of popups.

It does not break colors. I had chosen a darker .bg to illustrate the fixed behavior with a colorable scroll track. Matching ui.menu.scroll.bg to the popup fg text color would bring that back:

image

Also it hardcodes the right border to a specific border style.

I'm not sure what you mean by "hardcodes"—it's simply the nature of working with the Cell tui primitive. If you're referring to defining the scroll thumb and scroll track, I think distinguishing the two elements makes sense in this context and avoids the kind of confusion mentioned here.



I presumed you had intended the border-enabled scrollbar design to be this way, which is what I meant by "keeping [your] intention of repurposing the border as scrollbar track".

In any case, I chose to keep it because of a.) the visual harmony with borders and b.) the design had already found a certain degree of acceptance (it's what people would have been testing with so far in your original PR and master).

For reference, this is what a (bordered) scrollable popup would look like using the original style, which is not what this PR does:

image

cc: @the-mikedavis

@ath3
Copy link
Contributor

ath3 commented Feb 8, 2024

It does not break colors. I had chosen a darker .bg to illustrate the fixed behavior with a colorable scroll track. Matching ui.menu.scroll.bg to the popup fg text color would bring that back:

So it does break colors.
Idk which theme are you using but every theme i tried is having the same color problems as seen in your original screenshot.

I'm not sure what you mean by "hardcodes"—it's simply the nature of working with the Cell tui primitive. If you're referring to defining the scroll thumb and scroll track, I think distinguishing the two elements makes sense in this context and avoids the kind of confusion mentioned #9508 (comment).

We have a default border type which is currently plain, but has four possible values.

My goal was not to have scroll track when borders are enabled, because its too much visually.
Coloring part of the border as scroll track looks very odd.
If for some reason we decide to have scroll tracks then it should look like it does without borders, imo.

But like i said i intentionally didnt want scroll tracks when popup borders are enabled.

@the-mikedavis
Copy link
Member

We discussed this a bit in the Matrix room. When borders are enabled the scroll track looks bad:

With scroll track Without scroll track
with_scroll_track without_scroll_track

I've tested the all of the combinations between master, this branch and #9508 with borders enabled/disabled and I prefer #9508's behavior.

@flinesse
Copy link
Author

flinesse commented Feb 9, 2024

Colors "break" because most themes do not have ui.menu.scroll.bg explicitly set. Try kanagawa, modus_vivendi, or nightfox, or setting your own ui.menu.scroll.bg. An unset theme config can typically rely on setting inheritance to give you a discernible default, but in this case we have an atypical (though understandable) definition for ui.menu.scroll.

To reiterate, this PR:

  • Fixes the scroll track not showing when borders are off
  • Retains the original behavior of turning the wide scroll track off when borders are enabled
  • Allows coloring the section of border that the scroll track occupies by setting (ui.menu.scroll.bg)
popup-border on popup-border off
image image
image image
image image

rose_pine with custom overrides

@pascalkuthe
Copy link
Member

pascalkuthe commented Feb 9, 2024

Closing as #9508 was merged.

@pascalkuthe pascalkuthe closed this Feb 9, 2024
@the-mikedavis
Copy link
Member

this PR... Allows coloring the section of border that the scroll track occupies by setting ui.menu.scroll.bg

This is the part that I don't think is necessary. If we're not drawing the scroll track then theming it seems needless and I would prefer to see the border have a consistent color whether it's part of the scroll track or not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-gui Area: Helix gui improvements A-helix-term Area: Helix term improvements C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popup scroll track (ui.menu.scroll.bg) no longer being colored properly
5 participants