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

right nav toc gives no visual indication its scrollable #1746

Open
1 task done
techfg opened this issue Apr 12, 2024 · 7 comments · May be fixed by #1770
Open
1 task done

right nav toc gives no visual indication its scrollable #1746

techfg opened this issue Apr 12, 2024 · 7 comments · May be fixed by #1770

Comments

@techfg
Copy link
Contributor

techfg commented Apr 12, 2024

What version of starlight are you using?

0.21.5

What version of astro are you using?

4.6.0

What package manager are you using?

pnpm

What operating system are you using?

Linux

What browser are you using?

Chrome, Edge, FIrefox

Describe the Bug

In desktop mode, the toc appears on the right side of the screen. When the number of items in the list exceeds the viewport height, a couple of issues arise:

  1. There is no visual indication that the element is scrollable.
  2. If the number of items in the list exceeds the page content, when scrolled to the bottom of the window, the bottom of the nav is not visible and you must scroll the nav to get to the bottom of it.

Steps to reproduce:

  1. Open Overrides Page in desktop mode on a screen larger than 72rem and ensuring the height of the window is smaller than the amount of items in the toc
  2. Using the window vertical scrollbar, scroll down to bottom of the page

Expected Result:

  1. The toc should give some visual indication that it separately scrollable; OR
  2. The toc should scroll with the window

Actual Result:
The toc does not move when scrolling, the bottom of the toc is not visible and there is no visual clue that it can be scrolled separately.

Proposed Solutions:

  1. Add a vertical scroll (e.g., https://react.dev/reference/react-dom/components/common)
  2. Make it sticky (e.g., https://vuejs.org/glossary/)
  3. Add scroll listener to keep main-pane & starlight-toc always in sync (e.g., https://vercel.com/docs/projects/overview)
  4. Some combination of the above

Link to Minimal Reproducible Example

https://starlight.astro.build/reference/overrides/

Participation

  • I am willing to submit a pull request for this issue.
@techfg techfg changed the title right nav toc gives no visual indication is scrollable right nav toc gives no visual indication its scrollable Apr 12, 2024
@delucis
Copy link
Member

delucis commented Apr 12, 2024

Thanks for the detailed issue @techfg! I agree, we should improve this.

I think to start with, the table of contents should have a scroll bar respecting user preferences in cases where it overflows. That’s important for accessibility.

A scroll listener keeping it in sync could also be an option, but it would be a follow-up/complement to a proper scrollbar, because even if it keeps things in sync, users may want to use the scrollbar to explore the table of contents.

@HiDeoo
Copy link
Member

HiDeoo commented Apr 17, 2024

Started experimenting a bit with this.

This is not as easy as removing the scrollbar-width: none; in the .right-sidebar class as the width of the fixed element is way larger than the actual right sidebar.

One decision that would also need to be made is the position of this scrollbar. For example, after experimenting with a sticky approach rather than fixed, the scrollbar would be on the right side of the screen next to the global page scrollbar (maybe too close?):

scrollbar

I guess another valid approach would be to have the scrollbar closer to the actual table of contents. For example, here the Docusaurus approach:

docusaurus

Not sure if there are any recommendations on this, I'll have to research a bit more.

@techfg
Copy link
Contributor Author

techfg commented Apr 17, 2024

@HiDeoo - Appreciate you looking in to this, funny, I was working on this last night and had some ideas/questions as well for final implementation 😄 Should have a POC to share some thoughts later today!

@techfg
Copy link
Contributor Author

techfg commented Apr 18, 2024

@HiDeoo @delucis -

I was able to finish the POC, details below. As @HiDeoo mentions, there are some open questions that would need to be answered prior to finalizing. Apologize in the advance for the lengthy post, lots to cover 😄

Disclaimer

  1. I am not a css expert so if there are better ways to approach this, I'm all ears
  2. The POC is intended for demonstration purposes only, it is not fully tested nor approach finalized

POC Details

The POC is available at https://stackblitz.com/edit/withastro-starlight-5ct6eh.

How to use

  1. Open https://stackblitz.com/edit/withastro-starlight-5ct6eh in a browser - this will open /reference/example page initially which has a longer toc
  2. Ensure your browser window is wide enough and make the preview pane wide enough (> 72rem) to ensure the right sidebar is visible
  3. Near the top of the page, there is a Select list (green background), use this to toggle across the six (6) style options and see how the right sidebar style changes. Make sure to scroll the window top to bottom using browser scrollbar and when right sidebar is visible, scroll top to bottom there as well to see how everything plays together
  4. Click Getting Started in left nav to see a page with a shorter toc

TOC sidebar styles

  1. original - This is the current style of Starlight 0.21.5
  2. sticky - TOC is sticky and will scroll with the window, similar to VueJS.
  3. sticky scroll scrollbar standard - TOC contains its own standard styled scrollbar but does not scroll with window, similar to React.dev.
  4. sticky scroll scrollbar webkit - Same as above but using legacy webkit scrollbar styling which offers more control of styling options (e.g., no top/bottom buttons, different hover color, etc.)
  5. sticky scroll listener scrollbar standard - TOC contains its own scrollbar and will scroll with window, similar to Vercel and nextra.
  6. sticky scroll listener scrollbar webkit - Same as above but using legacy webkit scrollbar styling which offers more control of styling options (e.g., no top/bottom buttons, different hover color, etc.)

Changes required

TL;DR - Very little. For Style 1, 2, 3 & 4, the only change required was to right-sidebar css class. For 6 & 7, the only additional change was in starlight-toc.js to support syncing window scrolling with sidebar scroll position.

Each change made, is annotated in the code with the following:

/* POC Start */
// ...
/* POC End */

The POC has several files from Starlight but only a few were modified from original. In order to make it all work on StackBlitz without grabbing the entire repo, I had to pull in a few files that went unmodified. Here's the breakdown:

  1. ./src/components/starlight/TableOfContents/starlight-toc.ts
    • import 'scroll-into-view-if-needed' dependency
    • obtain sidebar & detect if mobile
    • add logic for scrolling (if applicable)
  2. ./src/components/starlight/TableOfContents/TableOfContentsList.astro
    • Unmodified
  3. ./src/components/starlight/MobileTableOfContents.astro
    • add data-is-mobile attribute to mobile-starlight-toc element
  4. ./src/components/starlight/TableOfContents.astro
    • Unmodified
  5. ./src/components/starlight/TwoColumnContent.astro
    • add data-sidebar-style to div.right-sidebar to support POC style selector
    • add/adjust styles for .right-sidebar
  6. ./src/components/starlight/SidebarStyleSelect.astro
    • select to support POC only

Why all this information, why didn't you just pick something and submit a PR? 🙋

Well, as @HiDeoo mentioned, there are questions to be answered and based on the input from @delucis previously, I wasn't sure where exactly to take this so wanted to gather some input/feedback first 😃

Background/Thoughts

@delucis had mentioned implementing a scrollbar (POC sticky scroll scrollbar [standard|webkit]) as a first step for accessibility, however my thoughts were that if we start simple, a sticky (POC sticky) solution may be preferred as I felt the most common usage would be the window scrolling up/down and when doing so with a separate toc scrollbar, you can't see the bottom of the toc. For example, I felt the VueJS experience is preferrable to the React.dev experience. I did some research on accessibility of sidebars and couldn't find anything definitive stated. With that said, w3c's own website uses the sticky approach without any scrollbar (granted, they don't have long toc's) as does USWDS. The WCAG spec, also on the w3c site, however, does use sticky scroll scrollbar for its left navigation.

Open Questions

  1. Which approach is preferred OR should we add a new style option to tableOfContents Configuration and let the user choose?
    1. If not a new config option, which approach is preferred?
    2. If add a new config option:
      1. What should it be named?
      2. Of the five (5) new styles, which should be supported?
  2. If option involves a scrollbar (Option 3, 4, 5, 6 from POC):
    1. Scrollbar Width
      1. Should scrollbar extend to right most portion of window; OR should it be narrowed?
      2. If narrowed what width should it be?
        1. Width required by content (each page could have different position of scrollbar based on content in the toc) OR;
        2. Fixed and at what width?
    2. Scrollbar Style - I believe the only place Starlight has customized scrollbars is in the expressive code integration which is represented in the webkit POC options.
      1. Should the scrollbar be consistent with expressive code (when browser supports webkit falling back to standard if not); OR;
      2. Should scrollbar always be standard
  3. If option involves scroll listener (Option 4 & 6 from POC), is it OK to take an external dependency (< 1kB) on scroll-into-view-if-needed or should I roll my own implementation?
  4. What are the officially supported browsers for Starlight as this will drive the way the css is implemented?
  5. Testing requirements - What are they for UI/UX changes? Possibly I'm overlooking but I couldn't find any tests in/around the right sidebar or much of anything related to straight UI/UX in other tests nor the contributing guide . If just manual testing, what browsers, operating systems, mobile devices, etc. should be tested and is there anything available to support configurations I don't have?

My $0.02

  1. Which Option - I would recommend sticky scroll listener scrollbar webkit (Option 6) as it blends everything together, is rather straightforward to implement and consistent with expressive code scrollbars. I think having a new config option would be ideal (and easy enough to implement) but it might be overkill (would be nice though and happy to include it!)
  2. Scrollbar -
    1. Width - I would recommend fixed width of 300px (fairly common) to ensure the scrollbar always appears in the same place from page to page regardless of width of content
    2. Style - I would recommend being consistent with expressive code and preferring webkit style when available
  3. Scrollbar Listener - Assuming taking an external dependency (< 1kB) is OK, I would recommend this. If taking an external dependency is not desirable, I would recommend postponing this part of the feature and going with sticky (Option 2) to start with since I believe having the toc move with the window is more desirable than sticky scrollbar scrollbar [standard|webkit] as described above in background/thoughts section, while still supporting accessibility.
  4. Officially supported browsers - Was unable to find anything definitive on either the Starlight or Astro docs sites.
  5. Testing requirements - Happy to do whatever is required

@HiDeoo @delucis & others - Look forward to your thoughts/input!

@HiDeoo
Copy link
Member

HiDeoo commented Apr 18, 2024

Thanks a lot for your very thorough experimentations and explanations. Definitely way better than my small experiment just adding a sidebar without modifying any behavior or exploring more options. Super appreciated!

I'll try to give my thoughts about the different options and questions you raised, but of course, this is just my opinion and would love to hear more thoughts from others (and also more people experimenting with this).

  1. I am personally in favor, at least to begin with, to not add any option and just fix the initial accessibility issue. Not sure that for this specific case, a lot of customization is required, people could still manually override this if they really want and supporting more than 1 behavior would definitely add a maintenance cost.
    i. I personally lean towards the option 3 (sticky scroll scrollbar standard).

    Option 2 (sticky) is pretty annoying to me when I cannot scroll to the bottom of the right sidebar without scrolling to the bottom of the page.
    Option 4 (sticky scroll scrollbar webkit) feels a bit weird as this would introduce a different scrollbar compared to the left sidebar and the main content.
    Option 5 (sticky scroll listener scrollbar standard) and 6 (sticky scroll listener scrollbar webkit) feels very distracting to me visually (and I guess this could be toned down but still not sure it's worth it).

  2. Scrollbar:
    i. I'm still unsure personally about this one yet, but it's really nice you went with the opposite approach compared to my screenshot from the previous post as we can clearly see the difference.
    ii. I am personally leaning towards standard scrollbar to match the left sidebar and the main content.

  3. You can find a list of supported browsers and their versions using this browserslist query (only referenced in the contributing guide at the moment).

  4. The first tests close to what you're describing are being implemented in Add support for synced tabs #640 at the moment with Playwright, so there is indeed nothing yet available for this. When we get more feedback and get closer to a solution, something that really helps this kind of PR is using Astro Talking & Doc'ing sessions to have people joining the sessions to actually test and play with it using their various devices and browsers. I think this change would be a great candidate for this kind of session too.

I'll still continue to think about this and play more with the amazing playground you provided but if anyone else has any thoughts, please share them!

@techfg
Copy link
Contributor Author

techfg commented Apr 19, 2024

HI @HiDeoo -

Thanks for the taking the time to review the POC and providing your thoughts, appreciated! Great to have a reference for the supported browsers, thank you!

You raise a very good point about the left sidebar scrollbar, agreed with you that if we end up with a scrollbar on the right sidebar, it should be identical in styling to the left. I do like how Docusaurus has their sidebar scrollbars styled thin vs standard as to not consume too much space (since its secondary nav) and also differentiate from the window scrollbar. I think it makes sense to modify the current left sidebar scroll to thin along with whatever changes we make to right sidebar - thoughts?

Regarding adding a new config option, agreed, not needed - let's pick a single path and implement it, can always add config option in future if there's demand for it and that outweighs the increased maintenance cost.

Based on your input and the above, I updated the POC to provide some additional options and control. Here's the new control panel:

image

  1. Left Nav
    1. Scrollbar Style
      1. original - This is the current style of Starlight 0.21.5 (standard browser scrollbar)
      2. use scrollbar settings - Will apply whatever settings are specified in Scrollbar options column
    2. Show Space Filler Item - Added a left nav menu item to make the nav overflow so that a scrollbar will appear
      1. yes - Show the filler items
      2. no - Hide the filler items
  2. Scrollbar
    1. Type
      1. standard - Will use default browser scrollbar
      2. prefer webkit - Will prefer custom style applied via -wekbit-scrollbar-* if browser supports it, else falls back to standard
    2. Width
      1. default - browser default scrollbar width
      2. thin - browser thin scrollbar width
  3. Right Nav
    1. Style
      1. original - This is the current style of Starlight 0.21.5
      2. sticky - TOC is sticky and will scroll with the window, similar to VueJS
      3. scollbar - TOC is sticky with a scrollbar
    2. Sync w/ Window Scroll - If Style is scrollbar, the following applies:
      1. no - window & nav scrolling are separate, similar to React.dev
      2. yes - window scrolling syncs with nav scroll, similar to Vercel and nextra

The above changes provide a way to see how things would look if we treat the styling of the right & left sidebar the same, or different along with the different styles of scrollbars. One thing to note is that the webkit styled scrollbars have a hover color applied, forgot to mention that last night.

My preference currently sits:

  1. Left & Right nav scrollbars styled the same
  2. Prefer webkit over standard
  3. Sync right scroll with window scroll
  4. Fixed width of right side of 18.75rem (--sl-sidebar-width)

However, based on the feedback from @HiDeoo & @delucis thus far, I think where things are heading is the following:

  1. Left & Right nav scrollbars styled the same
  2. Standard thin scrollbars
  3. Do not sync right scrollbar with window scroll
  4. Fixed width of right side of 18.75rem (--sl-sidebar-width)

The changes required for this are very small, only CSS and about 5 lines of it. Given that, and the guidance you provided on browser compatibility & testing, I'll work up an initial PR and we can iterate as needed.

Of course, if anyone else has any input, feel free to share!

@techfg
Copy link
Contributor Author

techfg commented Apr 19, 2024

Regarding above, after reviewing the code more, I found several places that have scrollbars beyond left & (soon to be) right nav (e.g., dropdowns) so going to shelve the thin style and stick to standard style to remain consistent. I do think implementing a proper thin style makes sense but it should be across the board and only on devices that support pointer: fine so will leave that for a separate PR on its own if there is an appetite from the team to introduce that.

For now, going with:

  1. Left & Right nav scrollbars styled the same
  2. Standard scrollbar width
  3. Do not sync right scrollbar with window scroll
  4. Width of right side of 18.75rem (--sl-sidebar-width) - Note that this will need to adjust dynamically based on screen width as otherwise the main content section shrinks more than current.

Look forward to any additional input, thanks!

techfg added a commit to techfg/starlight that referenced this issue Apr 19, 2024
@techfg techfg linked a pull request Apr 19, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants