-
Notifications
You must be signed in to change notification settings - Fork 94
Added fixes to properly align paragraphs when loading RTL documents #2352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
36824e4
9795ed1
5bca37e
d04acad
d15ac88
ccc6dc5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,15 @@ | ||||||
| /** | ||||||
| * RTL Paragraph — rendering feature module | ||||||
| * | ||||||
| * Centralises all right-to-left paragraph logic used by DomPainter: | ||||||
| * - Detecting whether a paragraph is RTL | ||||||
| * - Applying dir="rtl" and the correct text-align to an element | ||||||
| * - Resolving text-align for RTL vs LTR (justify → right/left) | ||||||
| * - Deciding whether segment-based (absolute) positioning is safe | ||||||
| * | ||||||
| * @ooxml w:pPr/w:bidi — paragraph bidirectional flag | ||||||
| * @ooxml w:rPr/w:rtl — run-level right-to-left flag | ||||||
| * @spec ECMA-376 §17.3.1.1 (bidi), §17.3.2.30 (rtl) | ||||||
| */ | ||||||
|
|
||||||
| export { isRtlParagraph, resolveTextAlign, applyRtlStyles, shouldUseSegmentPositioning } from './rtl-styles.js'; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,65 @@ | ||||||||||||||||||
| /** | ||||||||||||||||||
| * RTL paragraph style helpers for DomPainter. | ||||||||||||||||||
| * | ||||||||||||||||||
| * All RTL-aware rendering decisions live here so the main renderer | ||||||||||||||||||
| * doesn't need to re-derive direction in multiple places. | ||||||||||||||||||
| * | ||||||||||||||||||
| * @ooxml w:pPr/w:bidi — paragraph bidirectional flag | ||||||||||||||||||
| * @spec ECMA-376 §17.3.1.1 (bidi) | ||||||||||||||||||
| */ | ||||||||||||||||||
| import type { ParagraphAttrs } from '@superdoc/contracts'; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Returns true when the paragraph attributes indicate right-to-left direction. | ||||||||||||||||||
| * Checks both the `direction` string and the legacy `rtl` boolean flag. | ||||||||||||||||||
| */ | ||||||||||||||||||
| export const isRtlParagraph = (attrs: ParagraphAttrs | undefined): boolean => | ||||||||||||||||||
| attrs?.direction === 'rtl' || attrs?.rtl === true; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Compute the effective CSS text-align for a paragraph. | ||||||||||||||||||
| * | ||||||||||||||||||
| * DomPainter handles justify via per-line word-spacing, so 'justify' | ||||||||||||||||||
| * becomes 'left' (LTR) or 'right' (RTL) to align the last line correctly. | ||||||||||||||||||
| * When no explicit alignment is set the default follows the paragraph direction. | ||||||||||||||||||
| */ | ||||||||||||||||||
| export const resolveTextAlign = (alignment: ParagraphAttrs['alignment'], isRtl: boolean): string => { | ||||||||||||||||||
| switch (alignment) { | ||||||||||||||||||
| case 'center': | ||||||||||||||||||
| case 'right': | ||||||||||||||||||
| case 'left': | ||||||||||||||||||
| return alignment; | ||||||||||||||||||
| case 'justify': | ||||||||||||||||||
| return isRtl ? 'right' : 'left'; | ||||||||||||||||||
| default: | ||||||||||||||||||
| return isRtl ? 'right' : 'left'; | ||||||||||||||||||
claudiu-ior marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these two cases do the same thing — combine them?
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these two cases do the same thing — combine them?
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Apply `dir` and `text-align` to an element based on paragraph attributes. | ||||||||||||||||||
| * Used by both `renderLine` (line elements) and `applyParagraphBlockStyles` | ||||||||||||||||||
| * (fragment wrappers) so the logic stays in one place. | ||||||||||||||||||
| */ | ||||||||||||||||||
| export const applyRtlStyles = (element: HTMLElement, attrs: ParagraphAttrs | undefined): boolean => { | ||||||||||||||||||
| const rtl = isRtlParagraph(attrs); | ||||||||||||||||||
| if (rtl) { | ||||||||||||||||||
| element.setAttribute('dir', 'rtl'); | ||||||||||||||||||
| element.style.direction = 'rtl'; | ||||||||||||||||||
| } | ||||||||||||||||||
| element.style.textAlign = resolveTextAlign(attrs?.alignment, rtl); | ||||||||||||||||||
| return rtl; | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Whether the renderer should use absolute-positioned segment layout for a line. | ||||||||||||||||||
| * | ||||||||||||||||||
| * Returns false for RTL paragraphs: the layout engine computes tab X positions | ||||||||||||||||||
| * in LTR order, so for RTL we fall through to inline-flow rendering where the | ||||||||||||||||||
| * browser's native bidi algorithm handles tab positioning via dir="rtl". | ||||||||||||||||||
| */ | ||||||||||||||||||
| export const shouldUseSegmentPositioning = ( | ||||||||||||||||||
| hasExplicitPositioning: boolean, | ||||||||||||||||||
| hasSegments: boolean, | ||||||||||||||||||
| isRtl: boolean, | ||||||||||||||||||
| ): boolean => hasExplicitPositioning && hasSegments && !isRtl; | ||||||||||||||||||
claudiu-ior marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,6 +108,11 @@ import { | |
| stampBetweenBorderDataset, | ||
| type BetweenBorderInfo, | ||
| } from './features/paragraph-borders/index.js'; | ||
| import { | ||
| isRtlParagraph, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| applyRtlStyles, | ||
| shouldUseSegmentPositioning, | ||
| } from './features/rtl-paragraph/index.js'; | ||
|
|
||
| /** | ||
| * Minimal type for WordParagraphLayoutOutput marker data used in rendering. | ||
|
|
@@ -5320,17 +5325,8 @@ export class DomPainter { | |
| if (styleId) { | ||
| el.setAttribute('styleid', styleId); | ||
| } | ||
| applyParagraphDirection(el, paragraphAttrs); | ||
| const alignment = paragraphAttrs.alignment; | ||
|
|
||
| // Apply text-align for center/right immediately. | ||
| // For justify, we keep 'left' and apply spacing via word-spacing. | ||
| if (alignment === 'center' || alignment === 'right') { | ||
| el.style.textAlign = alignment; | ||
| } else { | ||
| // Default to 'left' for 'left', 'justify', 'both', and undefined | ||
| el.style.textAlign = 'left'; | ||
| } | ||
| const pAttrs = block.attrs as ParagraphAttrs | undefined; | ||
| const isRtl = applyRtlStyles(el, pAttrs); | ||
|
|
||
| if (lineRange.pmStart != null) { | ||
| el.dataset.pmStart = String(lineRange.pmStart); | ||
|
|
@@ -5584,10 +5580,11 @@ export class DomPainter { | |
| el.style.wordSpacing = `${spacingPerSpace}px`; | ||
| } | ||
|
|
||
| if (hasExplicitPositioning && line.segments) { | ||
| // Use segment-based rendering with absolute positioning for tab-aligned text | ||
| // When rendering segments, we need to track cumulative X position | ||
| // for segments that don't have explicit X coordinates. | ||
| if (shouldUseSegmentPositioning(hasExplicitPositioning ?? false, Boolean(line.segments), isRtl)) { | ||
| // Use segment-based rendering with absolute positioning for tab-aligned text. | ||
| // shouldUseSegmentPositioning returns false for RTL because the layout engine | ||
| // computes tab positions in LTR order; RTL lines fall through to inline-flow | ||
| // rendering where dir="rtl" lets the browser handle tab positioning. | ||
| // | ||
| // The segment x positions from layout are relative to the content area (left margin = 0). | ||
| // We need to add the paragraph indent to ALL positions (both explicit and calculated). | ||
|
|
@@ -5611,8 +5608,10 @@ export class DomPainter { | |
| : indentLeft; | ||
| const indentOffset = isListParagraph ? listIndentOffset : indentLeft + firstLineOffsetForCumX; | ||
| let cumulativeX = 0; // Start at 0, we'll add indentOffset when positioning | ||
|
|
||
| const segments = line.segments!; | ||
| const segmentsByRun = new Map<number, LineSegment[]>(); | ||
| line.segments.forEach((segment) => { | ||
| segments.forEach((segment) => { | ||
| const list = segmentsByRun.get(segment.runIndex); | ||
| if (list) { | ||
| list.push(segment); | ||
|
|
@@ -5698,7 +5697,6 @@ export class DomPainter { | |
| geoSdtWrapper.style.top = '0px'; | ||
| geoSdtWrapper.style.height = `${line.lineHeight}px`; | ||
| } | ||
| // Adjust element left to be relative to wrapper | ||
| elem.style.left = `${elemLeftPx - geoSdtWrapperLeft}px`; | ||
| geoSdtMaxRight = Math.max(geoSdtMaxRight, elemLeftPx + elemWidthPx); | ||
| this.expandSdtWrapperPmRange(geoSdtWrapper, (runForSdt as TextRun).pmStart, (runForSdt as TextRun).pmEnd); | ||
|
|
@@ -7163,11 +7161,7 @@ const applyParagraphBlockStyles = (element: HTMLElement, attrs?: ParagraphAttrs) | |
| if (attrs.styleId) { | ||
| element.setAttribute('styleid', attrs.styleId); | ||
| } | ||
| applyParagraphDirection(element, attrs); | ||
| if (attrs.alignment) { | ||
| // Avoid native CSS justify: DomPainter applies justify via per-line word-spacing. | ||
| element.style.textAlign = attrs.alignment === 'justify' ? 'left' : attrs.alignment; | ||
| } | ||
| applyRtlStyles(element, attrs); | ||
| if ((attrs as Record<string, unknown>).dropCap) { | ||
| element.classList.add('sd-editor-dropcap'); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolveTextAlignandisRtlParagrapharen't used outside this module — drop them from the export?