🐛 Fixed RTL languages rendering left-to-right in newsletter emails#27357
🐛 Fixed RTL languages rendering left-to-right in newsletter emails#27357betschki wants to merge 5 commits intoTryGhost:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughThe changes introduce bi-directional text direction support to email rendering. The 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.1)ghost/core/test/unit/server/services/email-service/email-renderer.test.jsThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Newsletters for sites with an RTL publication language (Persian, Arabic, Hebrew, Urdu) rendered left-to-right because the email template never emitted lang/dir on the root <html> element. Portal already calls i18next's built-in dir() for the same purpose; the email renderer just never wired it through. The renderer now passes the resolved locale and direction into the template context, the template sets html lang/dir, the inline body style picks up direction, and the feedback button row mirrors with the document. Defaults to ltr when no dir helper is provided so existing renders are byte-identical for LTR locales.
5dd2763 to
7afd15c
Compare
The email preview API endpoint snapshot still expected the old <html> shape; the renderer now emits lang/dir and a body direction style. Regenerating the snapshot here so the new wrapper output matches.
The integration snapshot test calls MemberWelcomeEmailRenderer directly with a fixture siteSettings that doesn't include locale, which made the new wrapper render <html lang dir="ltr"> (empty lang attribute). Production goes through service.js#getSiteSettings which already falls back to en, so this only affected the test path, but the renderer now defends against any direct caller missing locale by defaulting to en. Snapshot regenerated with the corrected output.
|
I'd love to see this PR merged soonest, as I currently have an issue with Ghost newsletter RTL support in production. Please let me know if there's anything I can do in support of moving this PR along. |
The "Manage subscription" cell in the subscription details box was hardcoded to align right via both an HTML attribute and CSS. Combined with the table column flip that comes from <html dir="rtl">, that pushed the link inwards against the subscription details block instead of out to the email margin. The HTML align attribute is now conditional on site.direction (Outlook still wants the literal value), the CSS uses text-align: end so it flips automatically in modern clients, and the mobile-stacking media rule that forced text-align: left switches to start so stacked content reads in the document direction.
|
EvanHahn
left a comment
There was a problem hiding this comment.
Mostly looks good but I have a few comments. Thanks for doing this!
| font-size: 15px; | ||
| font-weight: 400; | ||
| text-align: right; | ||
| text-align: end; |
There was a problem hiding this comment.
text-align: end and text-align: start are unsupported in some email clients. We may need to work around this by interpolating a value.
| * @param {object} dependencies.labs | ||
| * @param {{Post: object}} dependencies.models | ||
| * @param {Function} dependencies.t | ||
| * @param {Function} [dependencies.dir] Returns 'rtl' or 'ltr' for a given locale (i18next's `i18n.dir`) |
There was a problem hiding this comment.
- nit: could you use a proper type like
() => 'rtl' | 'ltr'instead of justFunction? - nit: could you make this function required? I see no reason to omit it and handle that case. That'll also let you remove the "Falls back to ltr when dir helper is not provided" test below.
| -ms-text-size-adjust: 100%; | ||
| -webkit-text-size-adjust: 100%; | ||
| color: #15212A; | ||
| direction: {{site.direction}}; |
There was a problem hiding this comment.
I don't know much about this property, but MDN discourages it in favor of the dir HTML attribute (which you've set). Is this necessary?
| it('Sets site.direction to rtl for Persian (fa)', async function () { | ||
| settings.locale = 'fa'; | ||
| const data = await templateDataWithSettings({}); | ||
| assert.equal(data.site.locale, 'fa'); | ||
| assert.equal(data.site.direction, 'rtl'); | ||
| }); | ||
|
|
||
| it('Sets site.direction to rtl for Arabic, Hebrew, and Urdu', async function () { | ||
| for (const locale of ['ar', 'he', 'ur']) { | ||
| settings.locale = locale; | ||
| const data = await templateDataWithSettings({}); | ||
| assert.equal(data.site.locale, locale, `expected locale ${locale}`); | ||
| assert.equal(data.site.direction, 'rtl', `expected rtl for ${locale}`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
nit: can you combine these two tests into one? I think you can remove the first test and add fa into the locale array.
| it('Renders RTL <html> attributes when site locale is Persian (fa)', async function () { | ||
| customSettings.locale = 'fa'; | ||
| const post = createModel(basePost); | ||
| const newsletter = createModel(baseNewsletter); | ||
| const response = await emailRenderer.renderBody(post, newsletter, null, {}); | ||
| assert.match(response.html, /<html lang="fa" dir="rtl">/); | ||
| assert.match(response.html, /direction:\s*rtl/); | ||
| // The feedback button row should also flip direction | ||
| assert.match(response.html, /class="feedback-buttons-container" dir="rtl"/); | ||
| }); | ||
|
|
||
| it('Renders RTL <html> attributes for Arabic, Hebrew, and Urdu', async function () { | ||
| for (const locale of ['ar', 'he', 'ur']) { | ||
| customSettings.locale = locale; | ||
| const post = createModel(basePost); | ||
| const newsletter = createModel(baseNewsletter); | ||
| const response = await emailRenderer.renderBody(post, newsletter, null, {}); | ||
| assert.match(response.html, new RegExp(`<html lang="${locale}" dir="rtl">`), `expected rtl <html> for ${locale}`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
nit: can you combine these two tests into one? I think you can remove the first test and add fa into the locale array.



Newsletters for sites with an RTL publication language (Persian, Arabic, Hebrew, Urdu) rendered left-to-right because the email template never emitted lang/dir on the root
<html>element.Portal already calls i18next's built-in dir() for the same purpose; the email renderer just never wired it through. The renderer now passes the resolved locale and direction into the template context, the template sets html lang/dir, the inline body style picks up direction, and the feedback button row mirrors with the document.
Defaults to ltr when no dir helper is provided so existing renders are byte-identical for LTR locales.
Before:

After:

Got some code for us? Awesome 🎊!
Please take a minute to explain the change you're making:
Please check your PR against these items:
We appreciate your contribution! 🙏