The align property is stripped from td elements, breaking styling

[jorroll]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and couldn’t find anything (or linked relevant results below)

Affected packages and versions

rehype-react 7.1.2

Link to runnable example

https://stackblitz.com/edit/react-ts-whdlcz?file=App.tsx

Steps to reproduce

Open this stackblitz link and inspect the <td /> element in the dom. Notice how it's properties don't match the input HTML. It appears as though align="center" is being converted to style="text-align: center;", which isn't equivalent. I suspect that this was an intentional decision because the align property is deprecated on td elements and the MDN docs suggest that it can be replaced with text-align: center css, but as previously mentioned the two are not equivalent (regardless of what MDN says). Here is a video demonstrating how converting align="center" to text-align: center breaks the CSS styling in an email.

• https://www.loom.com/share/95759a08773041b2afea63791ff499eb

Expected behavior

I expect the output JSX to match the input HTML. Specifically, I expect input HTML of <td align="center"></td> to be output as JSX <td align="center" />.

Actual behavior

The output JSX does not match the input HTML. Specifically, input HTML of <td align="center"></td> is output as JSX <td style="text-align: center;" />.

Runtime

Node v16

Package manager

yarn 1

OS

macOS

Build and bundle tools

Vite

[wooorm]

Hi!

They are not the same but they are equivalent in React as far as I am aware.
The main difference is that React does not support align, which is why https://www.npmjs.com/package/@mapbox/hast-util-table-cell-style is used.

For you actual use case:

• If you don’t need to process HTML, you can use dangerouslySetInnerHTML
• If you do need to parse HTML, and use this in browsers (and node), you might want to use https://github.com/syntax-tree/hast-util-from-html-isomorphic
• If you are looking for a modern replacement of the internally used utility, see https://github.com/syntax-tree/hast-util-to-jsx-runtime. I haven‘t had the time to move this project to that. It doesn’t yet handle align like this project does. But it might in the future, with an option

[jorroll]

Thanks for the information @wooorm

The main difference is that React does not support align

This may have been true at one point, but modern React (e.g. both React 17 and 18) supports align (and the Typescript types reflect this). Separately, I believe modern React supports the passthrough of any property so long as the name is all lowercase (this is to support custom properties and custom elements). For example, I've updated the demo with a <td align="center" /> JSX element which works as expected.

I went ahead, forked this package, and simply removed

import tableCellStyle from '@mapbox/hast-util-table-cell-style'

And now everything works as expected. I can certainly imagine that sometimes it would be desirable to transform deprecated properties/css/syntax into more modern equivalents, but in my experience it's not desirable for email (the use of align is very common in emails I've received from an array of companies and email providers).

Would you accept a PR to add an option to disable the problematic behavior?

For example

  function compiler(node) {
    /** @type {ReactNode} */
    // @ts-expect-error: assume `name` is a known element.
    let result = toH(h, options.transformTableCellStyles ? tableCellStyle(node) : node, options.prefix)

They are not the same but they are equivalent in React as far as I am aware.

I'm not sure what you mean by this? As demonstrated in the video above, align="center" and style="text-align: center;" are not equivalent.

If you don’t need to process HTML, you can use dangerouslySetInnerHTML

Unfortunately I do need/want to process the HTML.

Thanks!

[wooorm]

I believe modern React supports the passthrough of any property

Where does it say that on that page?

modern React

Do you have a source?

I’ve changed behavior in these packages several times because it didn‘t emit warnings in production build (stackblitz maybe?). And then after a month someone mentioned that it actually still emits warnings. So I’d rather not do the same again.

Ref: https://github.com/syntax-tree/hast-util-to-jsx-runtime/blob/12f0c52fd106196ceb732c9eff76e502c7ee2db7/lib/index.js#L218-L228

are not equivalent.

They are equivalent according to the HTML spec. The HTML spec specifies how align should be treated:

The thead, tbody, tfoot, tr, td, and th elements, when they have an align attribute whose value is an ASCII case-insensitive match for either the string "center" or the string "middle", are expected to center text within themselves, as if they had their 'text-align' property set to 'center' in a presentational hint, and to align descendants to the center.

[…]

— https://html.spec.whatwg.org/multipage/rendering.html#tables-2

the video above

Interesting! Thanks for the video! What type of tool is that, which you are showing?

I guess the thing here, with emails, is the bit about and to align descendants to the center in the HTML spec.

Would you accept a PR to add an option to disable the problematic behavior?

Sure

[jorroll]

Would you accept a PR to add an option to disable the problematic behavior?

Sure

👍

I believe modern React supports the passthrough of any property

Where does it say that on that page?

It's at the very bottom:

I updated the demo to include an example of this.

<td youcanactuallyaddanycustompropanditwillberetainedintheoutput="custom"></td> is included in the output rendered by React. The demo shows this using React 18, but I've also tested it in React 17. This React documentation page leads me to believe that this behavior has existed since React 16.

I’ve changed behavior in these packages several times because it didn‘t emit warnings in production build (stackblitz maybe?). And then after a month someone mentioned that it actually still emits warnings. So I’d rather not do the same again.

My fork doesn't appear to be causing any React warnings. I'll note that in the current (non-forked) package I do see warnings like the following:

These warnings also exist in my forked version of the package, but there are no new warnings.

I guess the thing here, with emails, is the bit about and to align descendants to the center in the HTML spec.

Seems like a reasonable hypothesis. Ultimately I'm not too concerned with why. Even if Chrome wasn't following the spec, I'd still defer to whatever Chrome did (obviously this won't apply to everyone--hence making the behavior togglable seems prudent in this case).

Interesting! Thanks for the video! What type of tool is that, which you are showing?

The video was taken with a tool called Loom, the "correct" behavior was demonstrated with a gmail client called superhuman (the Gmail web app also renders the content the "correct" way), the problematic behavior was shown in an unpublished email client I'm working on.

Thanks!

[wooorm]

is included in the output

As far as I am aware, the reason this exists isn’t about whether align is included in the output.
It’s I believe because it emits warnings. And because humans should not use align.

For the warnings, for lowercase attributes, it doesn’t seem to happen looking here: https://github.com/facebook/react/blame/b6c423daadaa35da3f34048628df9635505eecb1/packages/react-dom/src/shared/ReactDOMUnknownPropertyHook.js (line 155) for 6 years.

Ultimately I'm not too concerned with why

Understandable, but I am: because if your tool is broken, that’s where the fix should go, not here.

the "correct" behavior was demonstrated with a gmail client called

I particularly mean the tool that does not align things if text-align is present. Because that should align things? What is previewing those emails? Superhuman?

Do you have cells in cells (in more cells)? Then I think it’s the “align descendants to the center” thing that the spec describes.

[jorroll]

I particularly mean the tool that does not align things if text-align is present. Because that should align things? What is previewing those emails?

Ah, that was/is Google Chrome Version 111.0.5563.146 (a.k.a. my web browser). Sorry, I didn't realize that was unclear from the video but now I can see that I cropped out the important identifying information.

Do you have cells in cells (in more cells)? Then I think it’s the “align descendants to the center” thing that the spec describes.

Here are two emails exhibiting the behavior that you can inspect.

• https://www.getdrip.com/broadcasts/250429105/3c141a98d0b486e73c262
• https://mailchi.mp/sirenimmigrantrights/this-holiday-season-give-to-a-family-in-need-6251907

Note that I think that these "view email in browser" links will expire and stop working at some point in the not-too-distant future.

Ultimately I'm not too concerned with why

Understandable, but I am: because if your tool is broken, that’s where the fix should go, not here.

I now realize that it was unclear that the problematic behavior was being demoed using Google Chrome.

[jorroll]

Sidenote: if you haven't had the misfortune of working with emails before, it's very common for emails to be rendered with HTML like it's 1995.

[wooorm]

Here are two emails exhibiting the behavior that you can inspect.

So this is indeed table cells (in tables) in table cells.
The second example does not exhibit your bug from what I can see, because the table itself also has align=center, which aligns itself in the middle.
The first example doesn’t: text-align: center on the outer cell does not apply to the table, so the table is flush left.
In the second example, I can remove all text-align:centers and it works fine. But removing align="center" on the table, or setting display: block on it, moves it left.

Sidenote: if you haven't had the misfortune of working with emails before, it's very common for emails to be rendered with HTML like it's 1995.

Yeah, I just wonder how much I should care about things that are removed from the HTML spec.