From d10f567a1afcd25603acd50beabb41efd6559b96 Mon Sep 17 00:00:00 2001 From: Bryan Valverde Date: Wed, 11 Mar 2026 17:03:10 -0600 Subject: [PATCH] Fix marginTop/marginBottom returning undefined when selection spans unmeaningful paragraphs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a selection spans multiple paragraphs and the cursor lands on an unmeaningful paragraph (one that contains only a trailing/leading SelectionMarker), the previous call to getSelectedParagraphs with removeUnmeaningful=true would skip those paragraphs. This caused getFormatState to see a mix of paragraphs — some with marginTop/marginBottom set and some without — which triggered the conflict-resolution logic and returned undefined for those margin values, breaking the "space before/after" toolbar buttons. Fix: pass removeUnmeaningful=false in formatParagraphWithContentModel so that all selected paragraphs (including unmeaningful ones at the edges) receive the same format, keeping margins consistent across the selection. Also fix the isChecked logic in spaceBeforeAfterButtons so the button highlights correctly when a positive margin is present, and add overload + implementation guard for the new removeUnmeaningful parameter in getSelectedParagraphs. Add unit tests for the new removeUnmeaningful=false and mutate=true paths in collectSelections, and for formatParagraphWithContentModel covering multiple paragraphs, list items, table cells, no-selection return value, and apiName threading. Co-Authored-By: Claude Sonnet 4.6 --- .../demoButtons/spaceBeforeAfterButtons.ts | 2 +- .../utils/formatParagraphWithContentModel.ts | 6 +- .../formatParagraphWithContentModelTest.ts | 107 ++++++++++++++++++ .../modelApi/selection/collectSelections.ts | 19 +++- .../selection/collectSelectionsTest.ts | 56 +++++++++ 5 files changed, 186 insertions(+), 4 deletions(-) diff --git a/demo/scripts/controlsV2/demoButtons/spaceBeforeAfterButtons.ts b/demo/scripts/controlsV2/demoButtons/spaceBeforeAfterButtons.ts index 7c4fec4d0271..d1384690b164 100644 --- a/demo/scripts/controlsV2/demoButtons/spaceBeforeAfterButtons.ts +++ b/demo/scripts/controlsV2/demoButtons/spaceBeforeAfterButtons.ts @@ -12,7 +12,7 @@ export const spaceAfterButton: RibbonButton = { key: spaceAfterButtonKey, unlocalizedText: 'Remove space after', iconName: 'CaretDown8', - isChecked: formatState => !formatState.marginBottom || parseInt(formatState.marginBottom) <= 0, + isChecked: formatState => !!formatState.marginBottom && parseInt(formatState.marginBottom) > 0, onClick: editor => { const marginBottom = getFormatState(editor).marginBottom; setParagraphMargin( diff --git a/packages/roosterjs-content-model-api/lib/publicApi/utils/formatParagraphWithContentModel.ts b/packages/roosterjs-content-model-api/lib/publicApi/utils/formatParagraphWithContentModel.ts index 1c4102a2e3ce..d18b030719cb 100644 --- a/packages/roosterjs-content-model-api/lib/publicApi/utils/formatParagraphWithContentModel.ts +++ b/packages/roosterjs-content-model-api/lib/publicApi/utils/formatParagraphWithContentModel.ts @@ -17,7 +17,11 @@ export function formatParagraphWithContentModel( (model, context) => { splitSelectedParagraphByBr(model); - const paragraphs = getSelectedParagraphs(model, true /*mutate*/); + const paragraphs = getSelectedParagraphs( + model, + true /*mutate*/, + false /*removeUnmeaningful*/ + ); paragraphs.forEach(setStyleCallback); context.newPendingFormat = 'preserve'; diff --git a/packages/roosterjs-content-model-api/test/publicApi/utils/formatParagraphWithContentModelTest.ts b/packages/roosterjs-content-model-api/test/publicApi/utils/formatParagraphWithContentModelTest.ts index 2d5fd971ec07..60f73a539a38 100644 --- a/packages/roosterjs-content-model-api/test/publicApi/utils/formatParagraphWithContentModelTest.ts +++ b/packages/roosterjs-content-model-api/test/publicApi/utils/formatParagraphWithContentModelTest.ts @@ -11,7 +11,11 @@ import { } from 'roosterjs-content-model-types'; import { createContentModelDocument, + createListItem, + createListLevel, createParagraph, + createTable, + createTableCell, createText, } from 'roosterjs-content-model-dom'; @@ -132,4 +136,107 @@ describe('formatParagraphWithContentModel', () => { expect(splitSelectedParagraphByBrSpy).toHaveBeenCalledTimes(1); expect(splitSelectedParagraphByBrSpy).toHaveBeenCalledWith(model); }); + + it('multiple paragraphs selected', () => { + model = createContentModelDocument(); + const para1 = createParagraph(); + const para2 = createParagraph(); + const text1 = createText('test1'); + const text2 = createText('test2'); + + text1.isSelected = true; + text2.isSelected = true; + + para1.segments.push(text1); + para2.segments.push(text2); + model.blocks.push(para1, para2); + + formatParagraphWithContentModel( + editor, + apiName, + paragraph => (paragraph.format.backgroundColor = 'red') + ); + + expect(para1.format.backgroundColor).toBe('red'); + expect(para2.format.backgroundColor).toBe('red'); + }); + + it('paragraph in list item', () => { + model = createContentModelDocument(); + const listItem = createListItem([createListLevel('UL')]); + const para = createParagraph(); + const text = createText('item'); + + text.isSelected = true; + para.segments.push(text); + listItem.blocks.push(para); + model.blocks.push(listItem); + + formatParagraphWithContentModel( + editor, + apiName, + paragraph => (paragraph.format.backgroundColor = 'blue') + ); + + expect(para.format.backgroundColor).toBe('blue'); + expect(splitSelectedParagraphByBrSpy).toHaveBeenCalledTimes(1); + }); + + it('paragraph in table cell', () => { + model = createContentModelDocument(); + const table = createTable(1); + const cell = createTableCell(); + const para = createParagraph(); + const text = createText('cell content'); + + text.isSelected = true; + para.segments.push(text); + cell.blocks.push(para); + table.rows[0].cells.push(cell); + model.blocks.push(table); + + formatParagraphWithContentModel( + editor, + apiName, + paragraph => (paragraph.format.backgroundColor = 'green') + ); + + expect(para.format.backgroundColor).toBe('green'); + expect(splitSelectedParagraphByBrSpy).toHaveBeenCalledTimes(1); + }); + + it('returns false when no paragraphs are selected', () => { + model = createContentModelDocument(); + const para = createParagraph(); + para.segments.push(createText('unselected')); + model.blocks.push(para); + + let callbackReturn: boolean | undefined; + (editor.formatContentModel as jasmine.Spy).and.callFake( + (callback: ContentModelFormatter, options: FormatContentModelOptions) => { + context = { + newEntities: [], + newImages: [], + deletedEntities: [], + rawEvent: options.rawEvent, + }; + callbackReturn = callback(model, context); + } + ); + + formatParagraphWithContentModel(editor, apiName, () => {}); + + expect(callbackReturn).toBe(false); + }); + + it('passes apiName to formatContentModel options', () => { + model = createContentModelDocument(); + + formatParagraphWithContentModel(editor, apiName, () => {}); + + expect(editor.formatContentModel).toHaveBeenCalledWith( + jasmine.any(Function), + jasmine.objectContaining({ apiName }) + ); + }); }); diff --git a/packages/roosterjs-content-model-dom/lib/modelApi/selection/collectSelections.ts b/packages/roosterjs-content-model-dom/lib/modelApi/selection/collectSelections.ts index 454cdb1433f8..99d295f7ca71 100644 --- a/packages/roosterjs-content-model-dom/lib/modelApi/selection/collectSelections.ts +++ b/packages/roosterjs-content-model-dom/lib/modelApi/selection/collectSelections.ts @@ -203,6 +203,18 @@ export function getSelectedParagraphs( mutate: true ): ShallowMutableContentModelParagraph[]; +/** + * Get any array of selected paragraphs from a content model, return mutable paragraphs + * @param model The Content Model to get selection from + * @param mutate Set to true to indicate we will mutate the selected paragraphs + * @param removeUnmeaningful True to remove unmeaningful selection like only selection marker is selected, or head/tail paragraph is selected with selection marker at the wrong place + */ +export function getSelectedParagraphs( + model: ReadonlyContentModelDocument, + mutate: true, + removeUnmeaningful: boolean +): ShallowMutableContentModelParagraph[]; + /** * Get any array of selected paragraphs from a content model (Readonly) * @param model The Content Model to get selection from @@ -213,12 +225,15 @@ export function getSelectedParagraphs( export function getSelectedParagraphs( model: ReadonlyContentModelDocument, - mutate?: boolean + mutate?: boolean, + removeUnmeaningful: boolean = true ): ReadonlyContentModelParagraph[] { const selections = collectSelections(model, { includeListFormatHolder: 'never' }); const result: ReadonlyContentModelParagraph[] = []; - removeUnmeaningfulSelections(selections); + if (removeUnmeaningful) { + removeUnmeaningfulSelections(selections); + } selections.forEach(({ block }) => { if (block?.blockType == 'Paragraph') { diff --git a/packages/roosterjs-content-model-dom/test/modelApi/selection/collectSelectionsTest.ts b/packages/roosterjs-content-model-dom/test/modelApi/selection/collectSelectionsTest.ts index 768eb2ccdc4b..208489889d7f 100644 --- a/packages/roosterjs-content-model-dom/test/modelApi/selection/collectSelectionsTest.ts +++ b/packages/roosterjs-content-model-dom/test/modelApi/selection/collectSelectionsTest.ts @@ -424,6 +424,62 @@ describe('getSelectedParagraphs', () => { [p1] ); }); + + it('removeUnmeaningful=false keeps trailing selection marker paragraph', () => { + const s1 = createText('test1'); + const m2 = createSelectionMarker({ fontSize: '20px' }); + const p1 = createParagraph(false, { lineHeight: '10px' }); + const p2 = createParagraph(false, { lineHeight: '20px' }); + + p1.segments.push(s1); + p2.segments.push(m2); + + spyOn(iterateSelections, 'iterateSelections').and.callFake((_, callback) => { + callback([], undefined, p1, [s1]); + callback([], undefined, p2, [m2]); + return false; + }); + + const result = getSelectedParagraphs(null!, true /*mutate*/, false /*removeUnmeaningful*/); + + expect(result).toEqual([p1, p2]); + }); + + it('removeUnmeaningful=false keeps leading selection marker paragraph', () => { + const s2 = createText('test2'); + const m1 = createSelectionMarker({ fontSize: '10px' }); + const p1 = createParagraph(false, { lineHeight: '10px' }); + const p2 = createParagraph(false, { lineHeight: '20px' }); + + p1.segments.push(m1); + p2.segments.push(s2); + + spyOn(iterateSelections, 'iterateSelections').and.callFake((_, callback) => { + callback([], undefined, p1, [m1]); + callback([], undefined, p2, [s2]); + return false; + }); + + const result = getSelectedParagraphs(null!, true /*mutate*/, false /*removeUnmeaningful*/); + + expect(result).toEqual([p1, p2]); + }); + + it('mutate=true returns paragraphs as mutable', () => { + const p1 = createParagraph(false, { lineHeight: '10px' }); + const text = createText('hello'); + p1.segments.push(text); + + spyOn(iterateSelections, 'iterateSelections').and.callFake((_, callback) => { + callback([], undefined, p1, [text]); + return false; + }); + + const result = getSelectedParagraphs(null!, true /*mutate*/); + + expect(result.length).toBe(1); + expect(result[0]).toBe(p1); + }); }); describe('getFirstSelectedTable', () => {