components created using safelySpreadBoxProps have incorrect TS types

[vankhoawin]

Hi team, there are many components that are a <Box /> under the hood. Generally, these components inherit their respective native HTML tag interfaces (which is fine). However, because <Box /> components use safelySpreadBoxProps, there are many props that can be added to a Paste component (and encouraged by TypeScript Intellisense) but are actually filtered out e.g. className.

I'm wondering if it's possible to filter these props out from all Box-composed components. There might be a way by a combination of an as const assertion on BOX_PROPS_TO_BLOCK and an Omit<TrProps, keyof BOX_PROPS_TO_BLOCK>, but I'll leave those thoughts up to the team.

paste/packages/paste-core/components/table/src/Tr.tsx

Line 11 in 8de49d7

{...safelySpreadBoxProps(props)}

paste/packages/paste-core/primitives/box/src/SafelySpreadProps.ts

Lines 13 to 27 in 8de49d7

	export const BOX_PROPS_TO_BLOCK = [
	...LAYOUT_PROPS,
	...SPACE_PROPS,
	...BACKGROUND_PROPS,
	...BORDER_PROPS,
	...SHADOW_PROPS,
	...POSITION_PROPS,
	...FLEXBOX_PROPS,
	...TYPOGRAPHY_PROPS,
	...Object.keys(PseudoPropStyles),
	'backgroundColor',
	'borderColor',
	'className',
	'style',
	];

Side-note: the more root issue is that (by design) it's difficult to add custom styling to Paste components. For these edge cases I know teams have found less-than-optimal workarounds for their own use-case. Is this an issue that can be re-discussed?

[gloriliale]

Hi Van! We are definitely going to be fixing the typing issue- the "allowing then filtering out" that happens now is going to change asap. The ticket is in our backlog here.

For your side note: the lack of ways to custom style the Paste components is something we consider a feature- it allows for Paste to roll out fixes and design changes w/o breaking the application of anyone who's currently using it. You can read more about that in this discussion thread as well. As always, feel empowered to challenge some of our design choices as we are continually looking for ways to improve!

[vankhoawin]

awesome. thanks for the links!

[SiTaggart]

For a little extra context for those following along at home, I wrote this explanation of the problem for the team internally and we feel like it should live somewhere...

---

A lot of our components are one to one replacements for HTML concepts. As such you want to be able to do with them, what you would ordinarily be able to do with the raw HTML element.

Each HTML element can have a set of global HTML attributes applied to them https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes, they include event handlers such as onclick, ARIA attributes, display attributes and styling attributes such as class, style.

Some Elements have an extra set of attributes that are specific to them. For example input elements have a set of data input related attributes that can't just be applied to a <div />. Even more specific, only input type checkbox and radio can be checked.

With a React component, you more than likely wrap an HTML element and expose an interface to the consumer, as to what they can do with it. You probably want that interface to include all the relevant global and element specific HTML attributes for that component too. After all it's just a replacement for the HTML element, and I wanna do HTML things with my HTML components.

But with React to pass those interface properties to the underlying HTML element, you either need to pluck them off of the props argument:

const Button = props => {
  return (
    <button hidden={props.hidden} onClick={props.onClick} aria-label={props['aria-label']}>{props.children}</button>
  )
}

Or just spread them down, as that list of attributes can be super long and a pain in the butt to maintain. Or worse, you miss one, and someone reports a bug that they can't add a specific data attribute to the rendered DOM and you need to add it.

const Button = props => {
  return (
    <button {...props}>{props.children}</button>
  )
}

In typescript it's even worse as you now need to manage the props in the component and the interface. That would lead to a lot of duplication, so what is commonly done is you extend the type of HTML element you are exposing into the type interface. You'll see a lot of this in Paste:

interface AnchorProps extends React.AnchorHTMLAttributes<HTMLAnchorElement> {
  ...
}

Which says, grab everything that is a valid HTML attribute for an Anchor element plus what ever else we set in the interface as a valid prop.

But here's where it gets interesting. Because everything we build is a Box, and Box comes with a whole bunch of style props, spreading all props down to Box means the consumer can change everything about the look and feel of the component just by setting color="colorTextInverse" backgroundColor="colorBackgroundSuccess" on the React component.

const Button = props => {
  return (
    <Box as="button" {...props}>{props.children}</Box>
  )
}

So to mitigate that, we created safelySpreadBoxProps helper function.

const Button = props => {
  return (
    <Box as="button" {...safelySpreadBoxProps(props)}>{props.children}</Box>
  )
}

All that does is safely spread props on the Box component, by stripping out all the style props and all the Global HTML attributes that are associated with styling such as className and style.

Cool, implementation safe. You can apply any HTML attribute you want, but we block you from changing the styles.

(Why do we want to block you from changing the styles, go here.

So why is it broken? Because we're extending the HTML element types into the interface, typescript thinks you are allowed to apply className and style, even though we pluck them from props inside of safelySpreadBoxProps.

So to the typescript consumer, they've applied a valid prop, typescript compiled and didn't throw an error, but it didn't do anything.

That's what @vnguyen94 is spotting. He can add className, typescript doesn't complain, but it doesn't work because we strip it out.

The JIRA ticket is to create some form of Paste generic type, and automatically strips the props that we block in safely spread, from the HTML attribute type interfaces that we extend from. So that the types match the implementation, and if you try to set a blocked prop, typescript won't compile, but at least you won't think you're doing something wrong.

As an aside, looking at that screenshot @vnguyen94, if you are potentially trying to create some kind of expandable table row, it'll likely be inaccessible unless you're building a Treegrid with full keyboard controls. Which is another reason we stop you from using Paste components like that, you probably shouldn't be doing it. (If you're not, then that's fine, but it seemed like a good example as to why).

[vankhoawin]

Regarding the screenshot: i don't use any a11y tools like aXe but i do use the Chrome extension Vimium, which allows all anchors/buttons to be accessed by a dynamic hotkey. This is how it looks like on my screen:

But yeah I'll +1 on it probably being unaccessible by screen-reading tools regarding ARIA standards; I'll follow up with our designer to see if we can reinvent this flow, thanks

[SiTaggart]

Ah, yeah. That is a design we should not build in product.

Nested tables, or even nested rows like that are a really bad user experience for assistive technology users.