search

mui / base-ui

Base UI is an open-source library of accessible, unstyled UI components for React.
MIT License
291 stars 47 forks source link

[core] Standardize text direction (RTL/LTR) handling #831

Open mj12albert opened 1 week ago

mj12albert commented 1 week ago

We decided to standardize RTL/LTR handling by detecting the html dir attribute, as opposed to using a prop or using a provider, since it's a better DX if it just worked automatically:

// whole app
<html dir="rtl">
  <body>
    <Slider /> {/* automatically applies RTL keyboard behavior */}
  </body>
</html>

or 

// part of an app
<html>
  <body>
    <div>
      <Slider /> {/* a LTR slider */}
      <div dir="rtl">
        <Slider /> {/* a RTL slider */}
      </div>
    </div>
  </body>
</html>

This avoids setting the dir attribute on the component level by default and over-cluttering the DOM.

For styling, the CSS :dir() pseudo-class could be used:

.MySlider:dir(rtl) {
  /* RTL styles */
}

Looking at our .browserslistrc, only Chrome 109 and 119 don't support :dir() and would need an attribute selector instead:

[dir='rtl'] .MySlider {
  /* RTL styles */
}

Relevant components:

oliviertassinari commented 1 week ago

Does this work in the first place? There seem to be two cases:

  1. Greenfield project. I set the direction on the html element, once at the root <html dir="rtl">. I don't want the underlying DOM node to have the dir attribute to avoid bloat in the DOM tree.
  2. Brownfield project. I've added a few new components. I haven't made my whole app RTL-compliant yet. I want to add dir="rtl" a pages at a time.

Now, case 2. likely doesn't make a lot of sense. You wouldn't release RTL partially done. But in case 1. doesn't this hold?

I don't want the underlying DOM node to have the dir attribute to avoid bloat in the DOM tree.

If I set <Slider dir="rtl"> I would expect this to be on the DOM node, but I would likely not want this attribute, only the RTL behavior. If it's not on the DOM node, how do I add it for case 2?

So overall, the core problems seem to be that the dir prop inherits but the direction prop doesn't inherit, so those need to be two different prop names?

mj12albert commented 1 week ago

I don't want the underlying DOM node to have the dir attribute to avoid bloat in the DOM tree

Good point ~ I didn't consider this but with that in mind, what Ariakit does by separating behavior and layout makes sense^1

I think we could expand on what was done in https://github.com/mui/base-ui/pull/825 and apply to all components that need to support RTL behavior:

  1. Detect the CSS direction property (inheritable from <html dir="rtl">) and use that to determine the default text direction and apply LTR/RTL behavior accordingly
  2. Provide a prop (e.g. direction, anything that is not dir to avoid confusion) that will let you override this
  3. Base UI components will never set the dir attribute in the DOM

What do you think? @oliviertassinari @vladmoroz

mj12albert commented 1 week ago

Here's an example of Ariakit's Toolbar with RTL: https://stackblitz.com/edit/vitejs-vite-jth9ne?file=src%2Fstyle.css

The rtl prop applies RTL keyboard behavior but it still appears LTR because the dir attribute is not 'rtl' in the DOM

oliviertassinari commented 1 week ago

Ariakit does by separating behavior and layout

@mj12albert They say in https://ariakit.org/reference/toolbar#rtl "This only affects the composite widget behavior. You still need to set dir="rtl" on HTML/CSS.".

I don't know, Ariakit API seems unintuitive. If I set dir="rtl" on a component, why is the component swallowing the prop? I would expect the attribute to be on the DOM node if the prop is named this way.

mj12albert commented 1 week ago

If I set dir="rtl" on a component, why is the component swallowing the prop? I would expect the attribute to be on the DOM node if the prop is named this way.

I mean we do this without renaming our direction prop: the inherited CSS direction property would determine the default behavior. Users could do that by setting the HTML dir attribute anywhere from the html tag to the component itself, we will not interfere with it. This should cater to both greenfield and brownfield cases. (The direction prop would provide a way to override this if needed)

Ariakit API seems unintuitive

I agree - Ariakit makes you set both dir and their rtl: boolean prop. For us, you'd only need to set the dir attribute

oliviertassinari commented 1 week ago

@mj12albert I'm lost, I don't understand your last comment. What's the solution to handle the html dir attribute that inherits vs. the React prop for the rtl direction that doesn't inherit?

A few ideas on the solution space, options:

  1. we call the prop for the components differently, not dir
  2. we have a Direction React context, no props (I guess we need a context anyway to store this for each component, so maybe it makes sense to unify it. RTL is 2% of the needs, so we can degrades the UX/DX for RTL if it improves LTR)
  3. I have doubts about reading the DOM to figure out the dir works in the first place: a. can this lead to layout trashing? b. can't change the DOM structure based on it, breaks SSR c. CSS isn't enough https://stackoverflow.com/questions/5375799/direction-ltr-rtl-whats-the-difference-between-the-css-direction-and-html-di
mj12albert commented 1 week ago

we call the prop for the components differently, not dir

Yes ~ after considering your point about not cluttering the DOM with dir attributes everywhere, I agree with this @oliviertassinari

What's the solution to handle the html dir attribute

I'm proposing that components can be aware of the dir attribute anywhere higher in the DOM, and set their keyboard interactions based on that by default:

<html dir="rtl">
  <body>
    <Slider /> {/* left/right arrow keys are reversed */}
  </body>
</html>

This would also work, but dir is not a prop, it's just forwarded to the root element as a normal HTML attribute

<html>
  <body>
    <Slider dir="rtl" /> {/* left/right arrow keys are reversed */}
  </body>
</html>

Here's a demo of how this works with ToggleButtonGroup: https://deploy-preview-763--base-ui.netlify.app/experiments/toggle-group (and the source here)

a. can this lead to layout trashing? b. can't change the DOM structure based on it, breaks SSR c. CSS isn't enough

a. in Firefox, where computedStyleMap isn't supported, we do have to use getComputedStyles b./c. this is only about RTL keyboard interactions, especially if we want to avoid putting the dir attribute at the component level, so I don't think it has any SSR implications?

we have a Direction React context

I know Radix does this with a DirectionProvider, though both using that and using a prop on a component will set the HTML dir attribute on the component (sandbox). We can discuss this option as well with the team

oliviertassinari commented 3 days ago

in Firefox, where computedStyleMap isn't supported, we do have to use getComputedStyles

@mj12albert So computedStyleMap wouldn't trigger https://gist.github.com/paulirish/5d52fb081b3570c81e3a?

so I don't think it has any SSR implications

For a Slider, the SSR output depends on the direction, no?

https://github.com/mui/base-ui/blob/345ce7a2801d20a0111ed98492a3c17844b7e958/packages/react/src/Slider/Root/useSliderRoot.ts#L345

I know Radix does this with a DirectionProvider

Oh, I didn't know, option 2.

mj12albert commented 1 day ago

So computedStyleMap wouldn't trigger https://gist.github.com/paulirish/5d52fb081b3570c81e3a?

I believe that is the case, though I cannot find the source of where I read this now 🤦 Nevertheless, we decided that deferring this check to an event handler (as opposed to on mount) should be enough to avoid any performance hit: https://github.com/mui/base-ui/pull/871#pullrequestreview-2460498332

For a Slider, the SSR output depends on the direction, no?

base-ui/packages/react/src/Slider/Root/useSliderRoot.ts

Line 345 in 345ce7a

This is also inside an event handler; for the SSR output some of the built-in styles may have to be adjusted, but this wouldn't change how the value is being rendered

oliviertassinari commented 1 day ago

I believe that is the case, though I cannot find the source of where I read this now 🤦 Nevertheless, we decided that deferring this check to an event handler (as opposed to on mount) should be enough to avoid any performance hit: https://github.com/mui/base-ui/pull/871#pullrequestreview-2460498332

@mj12albert It triggers a reflow, proof: https://gist.github.com/paulirish/5d52fb081b3570c81e3a?permalink_comment_id=5299127#gistcomment-5299127.

It could still be a problem during events if spamming the handler, e.g. Arrow Down

https://github.com/user-attachments/assets/a14e619b-c70f-4775-84b0-571d0789437b

https://mui.com/material-ui/react-autocomplete/

Relevant work on this: https://github.com/radix-ui/primitives/issues/2703#issuecomment-1981119477. I guess he's referring to https://github.com/radix-ui/primitives/blob/toast-demo/packages/react/use-direction/src/useDirection.tsx#L16. It got changed in https://github.com/radix-ui/primitives/issues/749.

This is also inside an event handler; for the SSR output some of the built-in styles may have to be adjusted, but this wouldn't change how the value is being rendered

Ok, maybe we are lucky, I'm not so convinced we won't hit deadends for some components.

Side note, a pointer toward the dir prop name being confusing: https://github.com/radix-ui/primitives/issues/3016.