feat: add dir="auto" and text-align: justify for RTL support - #2823
Conversation
- Add dir="auto" to <p> elements in ApiInfo and ErrorBoundary - Add dir="auto" to H2 and H3 styled components - Add dir="auto" to SearchInput component - Add text-align: justify to p and h2 styles in Markdown blocks - Add DOMPurify hook to set dir="auto" on rendered p and h2 elements
There was a problem hiding this comment.
(🤖 This review thread by Claude Opus 5) Not a maintainer — I have been working on the translate="no" side of i18n in #2697, so this is adjacent territory and I was curious. Thanks for picking up RTL support; it is a real gap and dir="auto" is the right primitive for it. I checked the branch out and ran it, so the notes below are measured rather than guessed.
The SearchInput change is my favourite thing in here — one line, exactly right, no downside. Typing Hebrew or Arabic into the search box should absolutely flip the field.
I did hit three things that I think need addressing before this can land, plus a design question.
1. The suite does not currently run
npm test on this branch:
Test Suites: 14 failed, 13 passed, 27 total
Tests: 83 passed, 83 total
14 of 27 suites fail to even load, and only 83 of 279 tests execute. The cause is the new module-scope dompurify.addHook(...) — details inline on SanitizedMdBlock.tsx. Since all three boxes are ticked in the description, I suspect this was not visible locally, so it seemed worth flagging concretely rather than vaguely.
For reference, once I locally guarded that call, the remaining state was 3 failed, 24 passed — 6 snapshot failures from the new dir="auto" on H2/H3. Those are expected and legitimate; they just need npm run test:update-snapshot.
2. The markdown half of the feature is inert in the default configuration
The DOMPurify hook only fires inside dompurify.sanitize(), and SanitizedMdBlock calls that conditionally:
const sanitize = (sanitize, html) => (sanitize ? dompurify.sanitize(html) : html);options.sanitize comes from argValueToBoolean(raw.sanitize || raw.untrustedSpec), so it defaults to false. I rendered a Markdown block with default options and confirmed:
options.sanitize default = false | html has dir="auto": false
<p>hello world</p>
<h2 id="a-heading">a heading</h2>
So for anyone who has not explicitly opted into sanitize or untrustedSpec, descriptions get no dir="auto" at all. RTL support ending up coupled to an unrelated security option is almost certainly not the intent.
I would suggest doing this in MarkdownRenderer.ts instead, by overriding renderer.paragraph and renderer.heading. That runs regardless of sanitize, has no global side effect, and is where the markdown HTML is already being shaped. I did exactly this for renderer.code / renderer.codespan in #2697, so I can confirm the shape works and that DOMPurify preserves the added attribute when sanitization is enabled.
3. text-align: justify is a separate change, and I would drop it
This is the design question. Justification is not related to text direction — dir handles direction; text-align handles alignment. As written, justify is applied unconditionally to every <p> in every markdown block and to H2/H3 globally, so every existing LTR deployment gets a typographic change it did not ask for as a side effect of an RTL fix.
Concretely:
- Redoc's middle panel is narrow, and justified text in narrow columns produces uneven word spacing and vertical whitespace rivers.
- WCAG 1.4.8 specifically advises against justified body text, since the irregular spacing is harder to track for readers with dyslexia and some low-vision conditions. Slightly awkward for a change framed as accessibility-adjacent.
- On headings it does nothing for the common single-line case and stretches wrapped headings oddly.
If the goal was alignment that follows the text direction, the logical-property version does that correctly and is a no-op for LTR readers:
text-align: start;That said, I would honestly split alignment out of this PR entirely. The dir="auto" work stands on its own merits and would be easy to approve; bundling a global typographic change invites a much longer debate and will slow it down.
Smaller notes
H1does not getdir="auto"althoughH2andH3do — worth including for consistency.- Heads up that
H3is declared asstyled.h2(pre-existing, not yours). So "H3" actually renders anh2, which interacts confusingly with the hook'snode.tagName === 'H2'check — three different code paths all end up targetingh2. - A test asserting
dir="auto"shows up in rendered markdown would have caught point 2 immediately, and would justify the third checkbox. (All new/updated code is covered with tests) - CONTRIBUTING asks for a topic branch off
main; this one is pushed from your fork'smain, which makes it awkward for you to keep other work separate. - The What/Why/How and Reference sections are empty — linking the motivating issue would help a maintainer weigh the
justifyquestion in particular.
None of this is a knock on the direction. Points 1 and 2 are mechanical, point 3 is a scoping suggestion, and the underlying idea is one Redoc genuinely needs.
| const dompurify = DOMPurify['default'] as DOMPurify.DOMPurify; | ||
|
|
||
| // Add dir="auto" to p and h2 elements for RTL support | ||
| dompurify.addHook('afterSanitizeAttributes', (node) => { |
There was a problem hiding this comment.
This is the cause of the 14 suite failures, and I think it needs a different home regardless.
It throws at import time. Line 10 above is const dompurify = DOMPurify['default'] as .... Under the repo's Jest/CommonJS interop, DOMPurify['default'] is undefined. That was previously harmless because dompurify was only dereferenced inside the sanitize helper, which is called at render time and only when the option is on. Moving a dompurify.addHook(...) call to module scope makes it execute on import:
TypeError: Cannot read properties of undefined (reading 'addHook')
at src/components/Markdown/SanitizedMdBlock.tsx:13:11
at src/components/Markdown/AdvancedMarkdown.tsx:5:1
...
at src/components/__tests__/FieldDetails.test.tsx:4:1
Because almost everything imports Markdown transitively, 14 of 27 suites never load.
It is a global side effect. addHook mutates the shared DOMPurify instance, at import time, for the whole process. Redoc is embedded into other people's applications; if the host app also uses DOMPurify, merely importing Redoc would start adding dir="auto" to every <p> and <h2> the host sanitizes. The hook is also never removed and would accumulate across repeated module evaluation.
It only runs when sanitize is on, which defaults to false — see the main comment; that makes the markdown part of this feature inert for default configurations.
All three go away if you set the attribute during markdown rendering instead. In src/services/MarkdownRenderer.ts the renderer is already customised, so something along these lines:
const renderParagraph = renderer.paragraph.bind(renderer);
renderer.paragraph = (...args) => renderParagraph(...args).replace('<p>', '<p dir="auto">');(or override renderer.heading similarly). No global state, no dependency on sanitize, and it works for the default configuration. I used this same pattern for renderer.code/renderer.codespan in #2697 and verified DOMPurify keeps the added attribute when sanitization is enabled.
| ${headerCommonMixin(2)}; | ||
| color: ${({ theme }) => theme.colors.text.primary}; | ||
| margin: 0 0 20px; | ||
| text-align: justify; |
There was a problem hiding this comment.
The dir: 'auto' on line 23 is good. This text-align: justify I would drop, along with its twin on H3 and the two in Markdown/styled.elements.tsx.
Justification is orthogonal to direction — dir already solves RTL. This line changes heading alignment for every Redoc deployment, LTR included, as a side effect of an RTL PR. On headings specifically it does nothing in the usual single-line case and stretches wrapped headings unevenly.
If the intent is alignment that follows the text direction, the logical property does it properly and is a no-op for existing LTR users:
text-align: start;Either way I would move alignment into its own PR — it is a visual/typographic decision maintainers will want to weigh separately, and it would be a shame to have it hold up the dir="auto" work, which is much less contentious.
| line-height: ${props => props.theme.typography.lineHeight}; | ||
|
|
||
| p { | ||
| text-align: justify; |
There was a problem hiding this comment.
Same point as on headers.ts, but this one worries me more: it justifies every paragraph of every description in the API docs.
Redoc's middle panel is narrow, and justified text in a narrow measure produces uneven word spacing and vertical rivers of whitespace. WCAG 1.4.8 explicitly advises against justified blocks of text because that irregular spacing is harder to track for readers with dyslexia and some low-vision conditions — which cuts against the spirit of the change.
Since this is applied to all users regardless of language, it is a global typographic change rather than RTL support. text-align: start would give you direction-aware alignment with no effect on LTR readers, if that is what you were after.
| <h1>Something went wrong...</h1> | ||
| <small> {this.state.error.message} </small> | ||
| <p> | ||
| <p dir="auto"> |
There was a problem hiding this comment.
Minor, and easy to miss: this <p> wraps <details><summary>Stack trace</summary><pre>{stack}</pre></details>.
dir="auto" infers direction from the first strong directional character in the subtree. Today that is "Stack trace", so you get LTR and everything is fine. But a stack trace is code, and code should be unconditionally LTR rather than inferred — if the surrounding content ever changes such that a strong RTL character comes first, the trace would render mirrored and become quite hard to read.
dir="ltr" on the <pre> (or on this <p>) expresses the intent more precisely. Same reasoning as keeping code out of machine translation in #2697: code is not prose and should not inherit prose's locale behaviour.
|
|
||
| export const SearchInput = styled.input.attrs(() => ({ | ||
| className: 'search-input', | ||
| dir: 'auto', |
There was a problem hiding this comment.
This one is just nice — no notes. An RTL user typing into the search field gets the caret and text alignment they expect, it costs one line, and there is no effect on LTR users. If the PR were trimmed down to only the dir="auto" additions, this is the piece I would keep first.
elements in ApiInfo and ErrorBoundary
What/Why/How?
Reference
Tests
Screenshots (optional)
Check yourself