From 8475f611805adf6e5498ea58d17442708b317b01 Mon Sep 17 00:00:00 2001 From: jiuqingsong Date: Thu, 29 Jun 2023 00:07:37 -0700 Subject: [PATCH 1/2] Fix 213215: set direction for list item --- .../lib/modelApi/block/setModelDirection.ts | 53 +++++++++++++++++++ .../lib/publicApi/block/setDirection.ts | 24 ++------- .../lib/format/ContentModelListItemFormat.ts | 2 + 3 files changed, 58 insertions(+), 21 deletions(-) create mode 100644 packages-content-model/roosterjs-content-model-editor/lib/modelApi/block/setModelDirection.ts diff --git a/packages-content-model/roosterjs-content-model-editor/lib/modelApi/block/setModelDirection.ts b/packages-content-model/roosterjs-content-model-editor/lib/modelApi/block/setModelDirection.ts new file mode 100644 index 00000000000..bb011d22eb0 --- /dev/null +++ b/packages-content-model/roosterjs-content-model-editor/lib/modelApi/block/setModelDirection.ts @@ -0,0 +1,53 @@ +import { findListItemsInSameThread } from '../list/findListItemsInSameThread'; +import { getOperationalBlocks } from '../selection/collectSelections'; +import { isBlockGroupOfType } from '../common/isBlockGroupOfType'; +import { + ContentModelBlock, + ContentModelDocument, + ContentModelListItem, +} from 'roosterjs-content-model-types'; + +/** + * @internal + */ +export function setModelDirection(model: ContentModelDocument, direction: 'ltr' | 'rtl') { + const paragraphOrListItemOrTable = getOperationalBlocks( + model, + ['ListItem'], + ['TableCell'] + ); + + paragraphOrListItemOrTable.forEach(({ block }) => { + if (isBlockGroupOfType(block, 'ListItem')) { + const items = findListItemsInSameThread(model, block); + + items.forEach(item => { + item.levels.forEach(level => { + level.direction = direction; + }); + }); + + block.blocks.forEach(block => internalSetDirection(block, direction)); + } else if (block) { + internalSetDirection(block, direction); + } + }); + + return paragraphOrListItemOrTable.length > 0; +} + +function internalSetDirection(block: ContentModelBlock, direction: 'ltr' | 'rtl') { + block.format.direction = direction; + + // Adjust margin when change direction + // TODO: make margin and padding direction-aware, like what we did for textAlign. So no need to adjust them here + // TODO: Do we also need to handle border here? + const marginLeft = block.format.marginLeft; + const paddingLeft = block.format.paddingLeft; + + block.format.marginLeft = block.format.marginRight; + block.format.marginRight = marginLeft; + + block.format.paddingLeft = block.format.paddingRight; + block.format.paddingRight = paddingLeft; +} diff --git a/packages-content-model/roosterjs-content-model-editor/lib/publicApi/block/setDirection.ts b/packages-content-model/roosterjs-content-model-editor/lib/publicApi/block/setDirection.ts index 3a11c205bf1..45e51120b90 100644 --- a/packages-content-model/roosterjs-content-model-editor/lib/publicApi/block/setDirection.ts +++ b/packages-content-model/roosterjs-content-model-editor/lib/publicApi/block/setDirection.ts @@ -1,5 +1,6 @@ -import { formatParagraphWithContentModel } from '../utils/formatParagraphWithContentModel'; +import { formatWithContentModel } from '../utils/formatWithContentModel'; import { IContentModelEditor } from '../../publicTypes/IContentModelEditor'; +import { setModelDirection } from '../../modelApi/block/setModelDirection'; /** * Set text direction of selected paragraphs (Left to right or Right to left) @@ -7,24 +8,5 @@ import { IContentModelEditor } from '../../publicTypes/IContentModelEditor'; * @param direction Direction value: ltr (Left to right) or rtl (Right to left) */ export default function setDirection(editor: IContentModelEditor, direction: 'ltr' | 'rtl') { - formatParagraphWithContentModel(editor, 'setDirection', para => { - const isOldValueRtl = para.format.direction == 'rtl'; - const isNewValueRtl = direction == 'rtl'; - - if (isOldValueRtl != isNewValueRtl) { - para.format.direction = direction; - - // Adjust margin when change direction - // TODO: make margin and padding direction-aware, like what we did for textAlign. So no need to adjust them here - // TODO: Do we also need to handle border here? - const marginLeft = para.format.marginLeft; - const paddingLeft = para.format.paddingLeft; - - para.format.marginLeft = para.format.marginRight; - para.format.marginRight = marginLeft; - - para.format.paddingLeft = para.format.paddingRight; - para.format.paddingRight = paddingLeft; - } - }); + formatWithContentModel(editor, 'setDirection', model => setModelDirection(model, direction)); } diff --git a/packages-content-model/roosterjs-content-model-types/lib/format/ContentModelListItemFormat.ts b/packages-content-model/roosterjs-content-model-types/lib/format/ContentModelListItemFormat.ts index 6fa3b21eff5..1b948ccc3c5 100644 --- a/packages-content-model/roosterjs-content-model-types/lib/format/ContentModelListItemFormat.ts +++ b/packages-content-model/roosterjs-content-model-types/lib/format/ContentModelListItemFormat.ts @@ -1,6 +1,7 @@ import { DirectionFormat } from './formatParts/DirectionFormat'; import { LineHeightFormat } from './formatParts/LineHeightFormat'; import { MarginFormat } from './formatParts/MarginFormat'; +import { PaddingFormat } from './formatParts/PaddingFormat'; import { TextAlignFormat } from './formatParts/TextAlignFormat'; /** @@ -9,4 +10,5 @@ import { TextAlignFormat } from './formatParts/TextAlignFormat'; export type ContentModelListItemFormat = DirectionFormat & LineHeightFormat & MarginFormat & + PaddingFormat & TextAlignFormat; From a839a8b327bf0bc5e224fc62b270da0edc1e146a Mon Sep 17 00:00:00 2001 From: Jiuqing Song Date: Thu, 29 Jun 2023 10:21:15 -0700 Subject: [PATCH 2/2] fix test --- .../lib/modelApi/block/setModelDirection.ts | 48 ++-- .../modelApi/block/setModelDirectionTest.ts | 222 ++++++++++++++++++ .../test/publicApi/block/setDirectionTest.ts | 20 -- 3 files changed, 255 insertions(+), 35 deletions(-) create mode 100644 packages-content-model/roosterjs-content-model-editor/test/modelApi/block/setModelDirectionTest.ts diff --git a/packages-content-model/roosterjs-content-model-editor/lib/modelApi/block/setModelDirection.ts b/packages-content-model/roosterjs-content-model-editor/lib/modelApi/block/setModelDirection.ts index bb011d22eb0..037424f8f7f 100644 --- a/packages-content-model/roosterjs-content-model-editor/lib/modelApi/block/setModelDirection.ts +++ b/packages-content-model/roosterjs-content-model-editor/lib/modelApi/block/setModelDirection.ts @@ -2,9 +2,11 @@ import { findListItemsInSameThread } from '../list/findListItemsInSameThread'; import { getOperationalBlocks } from '../selection/collectSelections'; import { isBlockGroupOfType } from '../common/isBlockGroupOfType'; import { - ContentModelBlock, + ContentModelBlockFormat, ContentModelDocument, ContentModelListItem, + MarginFormat, + PaddingFormat, } from 'roosterjs-content-model-types'; /** @@ -25,29 +27,45 @@ export function setModelDirection(model: ContentModelDocument, direction: 'ltr' item.levels.forEach(level => { level.direction = direction; }); - }); - block.blocks.forEach(block => internalSetDirection(block, direction)); + item.blocks.forEach(block => internalSetDirection(block.format, direction)); + }); } else if (block) { - internalSetDirection(block, direction); + internalSetDirection(block.format, direction); } }); return paragraphOrListItemOrTable.length > 0; } -function internalSetDirection(block: ContentModelBlock, direction: 'ltr' | 'rtl') { - block.format.direction = direction; +function internalSetDirection(format: ContentModelBlockFormat, direction: 'ltr' | 'rtl') { + const wasRtl = format.direction == 'rtl'; + const isRtl = direction == 'rtl'; - // Adjust margin when change direction - // TODO: make margin and padding direction-aware, like what we did for textAlign. So no need to adjust them here - // TODO: Do we also need to handle border here? - const marginLeft = block.format.marginLeft; - const paddingLeft = block.format.paddingLeft; + if (wasRtl != isRtl) { + format.direction = direction; - block.format.marginLeft = block.format.marginRight; - block.format.marginRight = marginLeft; + // Adjust margin when change direction + // TODO: make margin and padding direction-aware, like what we did for textAlign. So no need to adjust them here + // TODO: Do we also need to handle border here? + const marginLeft = format.marginLeft; + const paddingLeft = format.paddingLeft; + + setProperty(format, 'marginLeft', format.marginRight); + setProperty(format, 'marginRight', marginLeft); + setProperty(format, 'paddingLeft', format.paddingRight); + setProperty(format, 'paddingRight', paddingLeft); + } +} - block.format.paddingLeft = block.format.paddingRight; - block.format.paddingRight = paddingLeft; +function setProperty( + format: MarginFormat & PaddingFormat, + key: keyof (MarginFormat & PaddingFormat), + value: string | undefined +) { + if (value) { + format[key] = value; + } else { + delete format[key]; + } } diff --git a/packages-content-model/roosterjs-content-model-editor/test/modelApi/block/setModelDirectionTest.ts b/packages-content-model/roosterjs-content-model-editor/test/modelApi/block/setModelDirectionTest.ts new file mode 100644 index 00000000000..b01623098a1 --- /dev/null +++ b/packages-content-model/roosterjs-content-model-editor/test/modelApi/block/setModelDirectionTest.ts @@ -0,0 +1,222 @@ +import { ContentModelDocument } from 'roosterjs-content-model-types'; +import { setModelDirection } from '../../../lib/modelApi/block/setModelDirection'; + +describe('setModelDirection', () => { + function runTest( + model: ContentModelDocument, + direction: 'ltr' | 'rtl', + expectedModel: ContentModelDocument, + expectedResult: boolean + ) { + const result = setModelDirection(model, direction); + + expect(result).toBe(expectedResult); + expect(model).toEqual(expectedModel); + } + + it('set direction for paragraph', () => { + runTest( + { + blockGroupType: 'Document', + blocks: [ + { + blockType: 'Paragraph', + format: { + marginLeft: '10px', + paddingRight: '20px', + }, + segments: [ + { + segmentType: 'SelectionMarker', + format: {}, + isSelected: true, + }, + ], + }, + ], + }, + 'rtl', + { + blockGroupType: 'Document', + blocks: [ + { + blockType: 'Paragraph', + format: { + direction: 'rtl', + marginRight: '10px', + paddingLeft: '20px', + }, + segments: [ + { + segmentType: 'SelectionMarker', + format: {}, + isSelected: true, + }, + ], + }, + ], + }, + true + ); + }); + + it('set direction for divider', () => { + runTest( + { + blockGroupType: 'Document', + blocks: [ + { + blockType: 'Divider', + format: {}, + isSelected: true, + tagName: 'hr', + }, + ], + }, + 'rtl', + { + blockGroupType: 'Document', + blocks: [ + { + blockType: 'Divider', + format: { + direction: 'rtl', + }, + isSelected: true, + tagName: 'hr', + }, + ], + }, + true + ); + }); + + it('set direction for list item', () => { + runTest( + { + blockGroupType: 'Document', + blocks: [ + { + blockType: 'BlockGroup', + blockGroupType: 'ListItem', + format: {}, + formatHolder: { + segmentType: 'SelectionMarker', + format: {}, + isSelected: true, + }, + levels: [ + { + listType: 'OL', + }, + ], + blocks: [ + { + blockType: 'Paragraph', + segments: [], + format: {}, + }, + { + blockType: 'Paragraph', + segments: [], + format: {}, + }, + ], + }, + { + blockType: 'BlockGroup', + blockGroupType: 'ListItem', + format: {}, + formatHolder: { + segmentType: 'SelectionMarker', + format: {}, + isSelected: true, + }, + levels: [ + { + listType: 'OL', + }, + ], + blocks: [ + { + blockType: 'Paragraph', + segments: [ + { + segmentType: 'SelectionMarker', + format: {}, + isSelected: true, + }, + ], + format: {}, + }, + ], + }, + ], + }, + 'rtl', + { + blockGroupType: 'Document', + blocks: [ + { + blockType: 'BlockGroup', + blockGroupType: 'ListItem', + format: {}, + formatHolder: { + segmentType: 'SelectionMarker', + format: {}, + isSelected: true, + }, + levels: [ + { + listType: 'OL', + direction: 'rtl', + }, + ], + blocks: [ + { + blockType: 'Paragraph', + segments: [], + format: { direction: 'rtl' }, + }, + { + blockType: 'Paragraph', + segments: [], + format: { direction: 'rtl' }, + }, + ], + }, + { + blockType: 'BlockGroup', + blockGroupType: 'ListItem', + format: {}, + formatHolder: { + segmentType: 'SelectionMarker', + format: {}, + isSelected: true, + }, + levels: [ + { + listType: 'OL', + direction: 'rtl', + }, + ], + blocks: [ + { + blockType: 'Paragraph', + segments: [ + { + segmentType: 'SelectionMarker', + format: {}, + isSelected: true, + }, + ], + format: { direction: 'rtl' }, + }, + ], + }, + ], + }, + true + ); + }); +}); diff --git a/packages-content-model/roosterjs-content-model-editor/test/publicApi/block/setDirectionTest.ts b/packages-content-model/roosterjs-content-model-editor/test/publicApi/block/setDirectionTest.ts index 58036972eb7..118c7591d55 100644 --- a/packages-content-model/roosterjs-content-model-editor/test/publicApi/block/setDirectionTest.ts +++ b/packages-content-model/roosterjs-content-model-editor/test/publicApi/block/setDirectionTest.ts @@ -99,10 +99,6 @@ describe('setDirection', () => { blockType: 'Paragraph', format: { direction: 'rtl', - marginLeft: undefined, - marginRight: undefined, - paddingLeft: undefined, - paddingRight: undefined, }, segments: [ { @@ -149,10 +145,6 @@ describe('setDirection', () => { blockType: 'Paragraph', format: { direction: 'rtl', - marginLeft: undefined, - marginRight: undefined, - paddingLeft: undefined, - paddingRight: undefined, }, segments: [ { @@ -262,10 +254,6 @@ describe('setDirection', () => { blockType: 'Paragraph', format: { direction: 'rtl', - marginLeft: undefined, - marginRight: undefined, - paddingLeft: undefined, - paddingRight: undefined, }, segments: [ { @@ -280,10 +268,6 @@ describe('setDirection', () => { blockType: 'Paragraph', format: { direction: 'rtl', - marginLeft: undefined, - marginRight: undefined, - paddingLeft: undefined, - paddingRight: undefined, }, segments: [ { @@ -385,10 +369,6 @@ describe('setDirection', () => { blockType: 'Paragraph', format: { direction: 'rtl', - marginLeft: undefined, - marginRight: undefined, - paddingLeft: undefined, - paddingRight: undefined, }, segments: [ {