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: add right sidebar scrollbar (#1746) #1770

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

techfg
Copy link
Contributor

@techfg techfg commented Apr 19, 2024

Description

Before:

image

After:

image

Additional Information

  • I was unable to decipher why the sl-container had a width specified but then a max-width set when min-width: 72rem when it only appears that the entire component would ever be visible at min-width: 72rem. I needed to ensure width: auto now that the parent (.right-sidebar) has a max-width, otherwise a horizontal scrollbar would display which wasn't necessary so to minimize risk in case I'm overlooking something, I just set width to auto when min-width: 72rem to be consistent with styles applied on .right-sidebar. Based on what I'm seeing in this component, the both the sl-container width and max-width could likely be removed completely but decided to play it safe - let me know if you think these can be removed.
  • The margin-right on right-sidebar is to provide spacing away from the window scrollbar so they don't end up immediately next to each other.
  • Manual testing on Windows Chrome/Edge/Firefox/Brave & Linux Chrome

Copy link

vercel bot commented Apr 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Apr 19, 2024 6:53pm

Copy link

changeset-bot bot commented Apr 19, 2024

🦋 Changeset detected

Latest commit: de7ce88

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@BryceRussell
Copy link
Member

Awesome stuff! Here is a preview of what the scrollbars look like for me on Windows/Firefox:

image
image

@HiDeoo
Copy link
Member

HiDeoo commented May 9, 2024

Sharing a few more screenshots and context. We took a look at your amazing contribution today in a Talkign & Doc'ing session on Discord. It's a great way to have feedback from more people and it's particularly useful for visual changes as people have different browsers, screen sizes, OS, etc.

macos-mouse
macOS with the default scrollbar settings and a mouse plugged in
macos-trackpad
macOS with the default scrollbar settings and no mouse plugged in
sarah
Linux with an extension to tweak the sidebar appearance

To summarize the feedback we got:

  • It can definitely feels weird to have, depending on your browser window size, a potential floating scrollbar for the table of contents.
  • It maybe could be helpful if "On this page" was always visible, maybe sticked to the top.

Personally, I think this can be pondered with the fact that the comparison is between no sidebar at all at the moment and in the PR preview, having one, so it's definitely a big change. It's also something that would only be visible on pages with a fairly long table of contents, which is not the majority of the pages.

I'll try to research a bit more the topic and see if some other websites have other approaches to this, but so far I did not find any that would be particularly relevant so I think the PR is already an amazing step forward.

@techfg
Copy link
Contributor Author

techfg commented May 9, 2024

@BryceRussell @HiDeoo - Thanks for sharing the screenshots on other devices/browsers and for reviewing the PR in Talking and Docs, seems like a great way to collect feedback!

Regarding @HiDeoo's thoughts:

  1. Agreed on it feeling weird on having a scrollbar on the page. I do think it's a necessary step forward for accessibility and UX in general but, especially on smaller window sizes, it is a bit overwhelming to be honest. This is part of the reason why I was suggesting in right nav toc gives no visual indication its scrollable #1746 that a "thin" or "styled" scrollbar might be beneficial as I think it's a bit "less" overwhelming given the width. If you haven't yet, take a look at the updated POC (details here) I put together to get a sense of what this would look like. The more I thought about this, if we do add "thin" styling, I think adding to just left nav & right nav as opposed to everywhere throughout Starlight anywhere a scrollbar could appear would be most appropriate. Another thought here is possibly to consider adding a frontmatter (or Starlight) configuration for "disable scroll" so users have the choice of whether or not they want scrollbar (could always be overridden via CSS as well).
  2. "On this Page" - I did consider sticking this to the top originally but after reviewing other sites, came to the decision to let it scroll. I landed on that decision for a couple of reasons: 1) Other than React.dev, all the other sites (VueJS, Vercel, nextra, etc.) I reviewed do not have it stick (Docusaurus doesn't even have a header at top of TOC); and 2) The important part of the TOC is the list items themselves as users know what the sidebar is for once they initially see "On this Page" so it seemed more appropriate to show as many of the items as possible since that it why users would be scrolling that area.

Look forward to hearing more thoughts on this and happy to make any adjustments to the PR, thank you!

@techfg
Copy link
Contributor Author

techfg commented May 9, 2024

I'll add that, especially given the feedback shared thus far, my recommendation would be to default to having scrollbars when content overflows (as opposed to a config option since user could override via CSS if they want to disable) and add "thin" styling to the scrollbar, defaulting to standard thin (scrollbar-width: thin) with a fallback to webkit for browsers do not yet support scrollbar-width). It would behave like the following config from the POC.

image

@HiDeoo
Copy link
Member

HiDeoo commented May 9, 2024

Adding a small reminder for myself as I forgot to do that today: I would like to see what is the end result with thin scrollbars when using an extension such as the one rendering the gold scrollbars and if the larger size configured in the extension is respected in such cases.

@HiDeoo
Copy link
Member

HiDeoo commented May 20, 2024

Just got a screenshot based on this webpage from someone using for accessibility purpose a scrollbar related extension:

screenshot of CSS thin scrollbars used with an extension to override scrollbars UI for accessibility purposes

scrollbar-width: thin are either not supported or override the extension settings so this pretty much means that, with this kind of extensions not working as expected, this option should not be used for now in Starlight.

If I am not forgetting anything, this pretty much leaves us with the current implementation used in this pull request.

@techfg
Copy link
Contributor Author

techfg commented May 20, 2024

Hi @HiDeoo -

Happy to take this in either direction as you see best, however my guess is that in this particular case, this is an issue in the extension not having caught up with the spec as scrollbar-width is a valid css property dating back to 2018 and a part of the latest editors draft.

Even Chrome which previously didn't support scrollbar-width and instead utilized the -webkit::scrollbar* css properties now has official support for scrollbar-width and scrollbar-color and applies them if they exist over the -webkit::scrollbar* properties.

Would be curious if the extension used in the screenshot handles the -webkit::scrollbar* properties since they are likely much more prevelent. Do you know which extension was used in the screenshot?

If we did implement scrollbar-width: thin, it would include a fallback to -webkit::scrollbar*. All the above said, happy to stick with standard scrollbars to avoid any potential issues for extensions/browsers that don't support either scrollbar-width or -webkit::scrollbar*.

@HiDeoo
Copy link
Member

HiDeoo commented May 22, 2024

I just tried with ::-webkit-scrollbar which includes a width and height property as I assume this would be the approach used to mimic thin scrollbars? If yes, the extension still doesn't seem to work and the "thin" scrollbar is always used instead of the custom ones.

I'm also not quite sure to understand how a fallback approach would work in this case as scrollbar-width: thin is supported by the browser so I guess -webkit::scrollbar* would be ignored anw?

The extension in the screenshots is Rescroller but maybe I missed something or misunderstood the idea?

@techfg
Copy link
Contributor Author

techfg commented May 22, 2024

Hi @HiDeoo -

Thanks for the follow-up and testing some situations out. When using the -webkit::scrollbar* settings, unfortunately things are a little more complex than only setting height/width - in fact, if you set them vs. not setting them it changes the behavior slightly but either way you need to set some of the other -webkit::scrollbar* pseudos' to make it work properly. See the section at the bottom of this article that starts with the text "Note that when you set the width or height of ::-webkit-scrollbar..." for more details on width/height and the article in general for ways to achieve modern vs. legacy solution.

If you take a look at the CSS of the POC I built, you'll see various rules configured based on the options in the Control Panel. If you toggle the "Scrollbar Type" field to "prefer webkit", the scrollbars will change to webkit style falling back to scrollbar-width if the webkit properties aren't supported. Detection of support for scrollbar-width and -webkit::scrollbar* is done with @supports rules.

I took a look at the Rescroller extension and determined why it doesn't work when scrollbar-width is thin. In short, the way Chrome behaves now is that if there is a scrollbar-width property, Chrome will use it and ignore any -webkit::scrollbar* properties completely even if they are marked with !important. The extension injects -webkit::scrollbar* rules to the page (this is how it achieves the custom scrollbars) but does not account for scrollbar-thin being present. The extension hasn't had any significant updates in over 8 years so I'm not suprised by this. What needs to occur within the extension itself is to apply scrollbar-width: unset to ensure that the -webkit::scrollbar* properties it injects are always honored (since it won't know if the site visited is using scrollbar-width or not).

You can see this in action as follows:

  1. Install rescroller
  2. After installing its "settings" page should appear but if not, navigate to its settings page
  3. You'll notice the scrolbars on your page have changed to the style selected in settings.
  4. Open dev tools and on the html element, add scrollbar-width: thin
  5. Scrollbar width should change to thin
  6. Add scrollbar-width: unset under the thin
  7. Scrollbar should change back to rescroller settings

You can run a similar test with the POC.

In short, the extension has a bug and does not account for scrollbar-width which makes sense since it was introduced after the extension was last updated. Anyone using the extension would need to add a custom CSS rule to the extension settings to unset scrollbar-width and apply the styles of their choosing.

If we did apply thin, the CSS would look something like the below which would mean scrollbar-width would be used/applied by the browser unless it didn't support it in which case -webkit::scrollbar* rules would be used/applied (unless browser doesn't support in which case standard scrollbars would be used/applied). There isn't a need for @supports since if the rule contains properties that the browser doesn't support, it just ignores it (we could wrap with @supports for clarity though.

@media (pointer: fine) {
  :root {
    --sl-scrollbar-size-legacy: 7px;
    --sl-scrollbar-color-track: var(--ec-frm-edBg);
    --sl-scrollbar-color-thumb: var(--ec-frm-edTabBarBrdBtmCol);
    --sl-scrollbar-color-thumb-hover: var(--sl-color-gray-4);
  }

  .right-sidebar,
  #starlight__sidebar {
    scrollbar-width: thin;
  }

  /* Legacy browsers with `::-webkit-scrollbar-*` support */
  .right-sidebar::-webkit-scrollbar,
  #starlight__sidebar::-webkit-scrollbar {
    height: var(--sl-scrollbar-size-legacy);
    width: var(--sl-scrollbar-size-legacy);
  }
  .right-sidebar::-webkit-scrollbar-track,
  #starlight__sidebar::-webkit-scrollbar-track {
    background: var(--sl-scrollbar-color-track);
    border-radius: 10px;
  }
  .right-sidebar::-webkit-scrollbar-thumb,
  #starlight__sidebar::-webkit-scrollbar-thumb {
    background: var(--sl-scrollbar-color-thumb);
    border-radius: 10px;
  }
  .right-sidebar::-webkit-scrollbar-thumb:hover,
  #starlight__sidebar::-webkit-scrollbar-thumb:hover {
    background: var(--sl-scrollbar-color-thumb-hover);
  }
}

All the above said, I think the decision to apply thin or not comes down to the guidelines that Starlight follows in terms of browser support as any CSS rule (not just scrollbars) has the potential to break an extension if the extension doesn't support it properly (assuming the browser itself supports the rule). As mentioned above, I'm good either way, whichever you and the Starlight team think is best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

right nav toc gives no visual indication its scrollable
3 participants