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

[ScrollArea] Create new ScrollArea component #665

Merged
merged 72 commits into from
Nov 5, 2024

Conversation

atomiks
Copy link
Contributor

@atomiks atomiks commented Sep 30, 2024

@mui-bot
Copy link

mui-bot commented Sep 30, 2024

Netlify deploy preview

https://deploy-preview-665--base-ui.netlify.app/

Generated by 🚫 dangerJS against a9823fe

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 30, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 30, 2024

It's about mui/mui-x#510, right? I have added the reference so developers can find a clear cross-link:

SCR-20241001-bikq

We could envision this component under the Base UI X brand umbrella in the future because most applications don't have a custom scrollbar. It seems more niche relative to nailing a menu, or combo box use case. Now, it's doesn't match with the criteria for Base UI X we defined (>=4 pages, >=1-2 person full time), still, I find it interesting that it shows the potential opportunity cost of having it in the core, or even open-source.

@colmtuite
Copy link
Contributor

colmtuite commented Sep 30, 2024

@oliviertassinari

It's about mui/mui-x#510, right?

No we didn't see this issue prior to planning this work. It's just a basic building block of high-quality web dev, so I had planned to add it from the beginning.

most applications don't have a custom scrollbar

Very many web apps (Slack, Linear, Radix docs etc. etc.) have custom scrollbars. It's not possible to build a high-quality web UI without them.

We could envision this component under the Base UI X brand umbrella

Do you think this belongs under the premium components? We need it for our new docs (for the sidebar and code blocks). It's not a huge investment, and Radix provides one for free.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 6, 2024

No we didn't see this issue prior to planning this work. It's just a basic building block of high-quality web dev, so I had planned to add it from the beginning.

@colmtuite In any way, I mostly wanted to create a backlink between these two so it's easier to discover the link.

Very many web apps (Slack, Linear, Radix docs etc. etc.) have custom scrollbars. It's not possible to build a high-quality web UI without them.

For peak execution, I very much agree.
For the average UIs, I don't think it's a big need, it's common to not see it used in average UIs, e.g. I personally never implemented a custom scrollbar 😄.

I think to be careful about the opportunity cost: downloads https://npm-stat.com/charts.html?package=overlayscrollbars&package=react-dropzone&from=2018-10-05&to=2024-10-05 vs. effort to do it right: https://github.com/KingSora/OverlayScrollbars/issues?q=is%3Aissue+is%3Aclosed.

The platform exposes some primitives to customize this, e.g. https://codepen.io/oliviertassinari/pen/PoMzdJa

Screen.Recording.2024-10-06.at.22.37.01.mov

but until they fix w3c/csswg-drafts#9826, w3c/csswg-drafts#10591, it doesn't feel like a viable option, far from it.

Do you think this belongs under the premium components? We need it for our new docs (for the sidebar and code blocks). It's not a huge investment, and Radix provides one for free.

Premium plan: no. Pro plan: maybe for the styled layer it would make sense? It might be pushing it too far to have this as a pro feature in Base UI.

I get the feeling that 3 years from now, this component is no longer needed, built-in into the platform.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@romgrk implemented a custom scroll container for the data grid https://github.com/mui/mui-x/blob/master/packages/x-data-grid/src/components/virtualization/GridVirtualScrollbar.tsx. Maybe this can be of any help. For example, can be seen in https://mui.com/x/react-data-grid/#pro-plan.

<div
ref={tableWrapperRef}
style={{
display: 'table',
Copy link
Member

Choose a reason for hiding this comment

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

Built-in inline style? Not sure this will fly with CSP.

Comment on lines +7 to +12
*,
*::before,
*::after {
box-sizing: border-box;
}

Copy link
Member

@oliviertassinari oliviertassinari Oct 6, 2024

Choose a reason for hiding this comment

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

Great. In the visual regression, we do the opposite, so a great way to ensure that our style works with both models:

box-sizing: content-box;

Edit: oh, wait, it's broken, fixed: #707.

docs/next-env.d.ts Outdated Show resolved Hide resolved
@atomiks atomiks marked this pull request as ready for review October 8, 2024 01:16
@michaldudak
Copy link
Member

michaldudak commented Oct 11, 2024

The CSB version of the intro demo (https://codesandbox.io/p/sandbox/pydrhc?file=%2Fsrc%2Findex.tsx) looks slightly broken:
image

Likely a box-sizing issue?


As for the implementation, I couldn't spot any issues. Good work!

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 14, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Could you folow the patterns from #735?
(undefined instead of null and error message)

Same in ScrollAreaRootContext.

@vladmoroz
Copy link
Contributor

Took it for a spin. Many notes, but overall it's great. I love how much control I have over when exactly the scrollbar would be shown or hidden.

  • Are we sure we don't want to remove the built-in scrollbar-width and ::-webkit-scrollbar styles by default? Having to add them manually is going to be a hurdle when Radix users migrate, especially if they use Tailwind, which doesn't provide classes for either property.
  • type="inlay" is an unusual value. I appreciate the symmetry with "overlay" but I don't think I had ever typed the word "inlay" in the code editor, or referred to these scrollbars in a conversation like that. "inset" or "classic" is what I'd expect to call that scrollbar type instead.
    • Since we provide little built-in styles here already, I almost wonder if gutter and type should be implemented by the user? I assume the difference is mainly in having position: absolute and setting the padding manually. Where do we draw the line between functional styles like this and styles like removing default scrollbar-width that you seem to have to do in all cases?
  • Arrow keys don't scroll the scroll area after clicking the scrollbar (works with native scrollbars)
  • Scrollbar visibility and size doesn't update when Root height changes (e.g. due to an imperative style change. It does update when children change).
  • overscroll-behavior: none on Root doesn't affect the Scrollbar part. In other words, scrolling with pointer over the Scrollbar part retains the default overscroll behaviour. This is problematic in designs where overscroll-behavior is important to remove.
  • It was a bit unintuitive to figure out that to always display scrollbars, you'd need to use hidden={false} (I assume this is the way?). I expected keepMounted or something like that coming from Radix. Not a big deal, just seems to be slightly inconsistent with how force rendering other components works.
  • Should it be data-hovered to converge with React Aria's data attribute and our own data-pressed?

@atomiks
Copy link
Contributor Author

atomiks commented Oct 15, 2024

Are we sure we don't want to remove the built-in scrollbar-width and ::-webkit-scrollbar styles by default? Having to add them manually is going to be a hurdle when Radix users migrate, especially if they use Tailwind, which doesn't provide classes for either property.

I think we can remove them by default, but Safari requires a <style> tag, right?

type="inlay" is an unusual value. I appreciate the symmetry with "overlay" but I don't think I had ever typed the word "inlay" in the code editor, or referred to these scrollbars in a conversation like that. "inset" or "classic" is what I'd expect to call that scrollbar type instead.

I got this naming from VS Code: it has a feature called "inlay hints" which are inline with the flow of the code

Since we provide little built-in styles here already, I almost wonder if gutter and type should be implemented by the user? I assume the difference is mainly in having position: absolute and setting the padding manually. Where do we draw the line between functional styles like this and styles like removing default scrollbar-width that you seem to have to do in all cases?

I think we can remove the scrollbar styles by default now, but removing the scrollbar is a lot easier than defining the padding (and the element node is not actually exposed by default).

Scrollbar visibility and size doesn't update when Root height changes (e.g. due to an imperative style change. It does update when children change).

There's a ResizeObserver but it might need to be lifted up to the root if it's defined on the viewport. Will check this.

overscroll-behavior: none on Root doesn't affect the Scrollbar part. In other words, scrolling with pointer over the Scrollbar part retains the default overscroll behaviour. This is problematic in designs where overscroll-behavior is important to remove.

I don't think this is possible to fix since it uses a wheel event, since the pointer events block scrolling otherwise. Radix also has this issue

It was a bit unintuitive to figure out that to always display scrollbars, you'd need to use hidden={false} (I assume this is the way?). I expected keepMounted or something like that coming from Radix. Not a big deal, just seems to be slightly inconsistent with how force rendering other components works.

They're visible by default, there's no hidden or anything. You can hide it by default then use the hovering/scrolling style hooks to show it.

Should it be data-hovered to converge with React Aria's data attribute and our own data-pressed?

hovering matches scrolling verb, but hover alone would match the CSS state. hovered sounds like you've potentially left the element already and doesn't seem accurate.

@vladmoroz
Copy link
Contributor

I don't think this is possible to fix since it uses a wheel event, since the pointer events block scrolling otherwise. Radix also has this issue

Is this approach too heavy-handed? I asked specifically because I had run into the overscroll-behavior issue before with Radix and it's really unpleasant with no workaround.

I think we can remove them by default, but Safari requires a <style> tag, right?

Yeah Safari is the main problem. We don't have a precedent for rendering a <style> tag yet, do we?

They're visible by default, there's no hidden or anything. You can hide it by default then use the hovering/scrolling style hooks to show it.

I'm seeing a hidden attribute when there is no overflow:

image

I'd expect to either not have this element rendered at all and force mount it, or have to handle the visibility myself.

hovering matches scrolling verb, but hover alone would match the CSS state. hovered sounds like you've potentially left the element already and doesn't seem accurate.

I think it's best when data attributes describe the state of the component itself rather than what the user is doing; in this sense data-hovered reads more like "the element is hovered" to me, same as data-selected, data-active, etc. Can go either way on data-scrolling since it's a transitory state though. Maybe best if we leave these discussions for the Monday calls

@atomiks
Copy link
Contributor Author

atomiks commented Oct 16, 2024

I'm seeing a hidden attribute when there is no overflow:

There are two separate "hidden" cases:

  1. There is no overflow, so no scrollbar should ever be shown - that's where hidden comes in
  2. There is overflow, but you need to decide when to show it: hovering, scrolling, or both?

I considered the option 1 to mean you never want to show it. The gutter prop keeps layout stable, so hidden is applied for that case (or we could just unmount the entire component).

Due to transitions/animations, it makes sense to keep it mounted at all times as they're easier to create this way. Unlike floating components that can contain expensive content and create lots of elements in the DOM, scrollbars are cheap. For consistency with other components, if we adopt the keepMounted API, it could be set to true by default.

Is radix-ui/primitives#2200 too heavy-handed? I asked specifically because I had run into the overscroll-behavior issue before with Radix and it's really unpleasant with no workaround.

While this works for none/contain, it still doesn't support the browser's default auto value which supports native scroll-chaining. We could do some kind of detection here but it wouldn't be as good as the native implementation.

It looks like this also unfortunately breaks being able to create a "hit area" around the thumb as Radix does, in order to be able to click it more easily.

Edit: I think preventing scroll of the parent at all times regardless of overscroll-behavior might be the best solution. This ensures that the hit area can still work, and to me feels like a better trade-off for the default auto overscroll on the first momentum scroll/wheel try, even though it still blocks subsequent attempts to scroll.

Yeah Safari is the main problem. We don't have a precedent for rendering a <style> tag yet, do we?

I don't think we've used it yet, but I suppose Radix's approach will work.

@colmtuite colmtuite requested a review from vladmoroz October 17, 2024 10:01
@atomiks atomiks force-pushed the feat/ScrollArea branch 2 times, most recently from df1ba95 to dde1b59 Compare October 30, 2024 01:11
Copy link
Contributor

@vladmoroz vladmoroz left a comment

Choose a reason for hiding this comment

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

Looks incredible, can't wait to use it

@atomiks atomiks merged commit 2d1cd9e into mui:master Nov 5, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ScrollArea] Implement ScrollArea
6 participants