From 5b4e9aef20b74e5b0a34de887713259769a4343e Mon Sep 17 00:00:00 2001 From: Julia Roldi Date: Fri, 11 Sep 2026 17:01:47 -0300 Subject: [PATCH 1/2] apply table format --- .../publicApi/table/applyTableBorderFormat.ts | 134 +++++++++--- .../table/applyTableBorderFormatTest.ts | 201 ++++++++++++++++++ 2 files changed, 302 insertions(+), 33 deletions(-) diff --git a/packages/roosterjs-content-model-api/lib/publicApi/table/applyTableBorderFormat.ts b/packages/roosterjs-content-model-api/lib/publicApi/table/applyTableBorderFormat.ts index 0bf8f6353295..e65561d000fc 100644 --- a/packages/roosterjs-content-model-api/lib/publicApi/table/applyTableBorderFormat.ts +++ b/packages/roosterjs-content-model-api/lib/publicApi/table/applyTableBorderFormat.ts @@ -23,6 +23,15 @@ import type { */ type BorderPositions = 'borderTop' | 'borderBottom' | 'borderLeft' | 'borderRight'; +/** + * @internal + * Border positions to update on a cell + */ +type BorderUpdate = { + cell: ReadonlyContentModelTableCell; + positions: BorderPositions[]; +}; + /** * @internal * Perimeter of the table selection @@ -37,6 +46,7 @@ type Perimeter = { /** * Operations to apply border + * Remove the targeted borders instead if they all already match the requested format. * @param editor The editor instance * @param border The border to apply * @param operation The operation to apply @@ -94,6 +104,13 @@ export function applyTableBorderFormat( const isRtl = tableModel.format.direction == 'rtl'; if (sel) { + const borderUpdates: BorderUpdate[] = []; + const collectBorderFormat = ( + cell: ReadonlyContentModelTableCell, + positions: BorderPositions[] + ) => { + borderUpdates.push({ cell, positions }); + }; const operations: BorderOperations[] = [operation]; while (operations.length) { switch (operations.pop()) { @@ -121,7 +138,7 @@ export function applyTableBorderFormat( ) { const cell = tableModel.rows[rowIndex].cells[colIndex]; // Format cells - All borders - applyBorderFormat(cell, borderFormat, allBorders); + collectBorderFormat(cell, allBorders); } } @@ -143,7 +160,7 @@ export function applyTableBorderFormat( isRtl ? sel.lastColumn : sel.firstColumn ]; // Format cells - Left border - applyBorderFormat(cell, borderFormat, leftBorder); + collectBorderFormat(cell, leftBorder); } // Format perimeter @@ -161,7 +178,7 @@ export function applyTableBorderFormat( isRtl ? sel.firstColumn : sel.lastColumn ]; // Format cells - Right border - applyBorderFormat(cell, borderFormat, rightBorder); + collectBorderFormat(cell, rightBorder); } // Format perimeter @@ -176,7 +193,7 @@ export function applyTableBorderFormat( ) { const cell = tableModel.rows[sel.firstRow].cells[colIndex]; // Format cells - Top border - applyBorderFormat(cell, borderFormat, topBorder); + collectBorderFormat(cell, topBorder); } // Format perimeter @@ -191,7 +208,7 @@ export function applyTableBorderFormat( ) { const cell = tableModel.rows[sel.lastRow].cells[colIndex]; // Format cells - Bottom border - applyBorderFormat(cell, borderFormat, bottomBorder); + collectBorderFormat(cell, bottomBorder); } // Format perimeter @@ -207,9 +224,8 @@ export function applyTableBorderFormat( } // Single column selection if (singleCol) { - applyBorderFormat( + collectBorderFormat( tableModel.rows[sel.firstRow].cells[sel.firstColumn], - borderFormat, ['borderBottom'] ); for ( @@ -219,25 +235,20 @@ export function applyTableBorderFormat( ) { const cell = tableModel.rows[rowIndex].cells[sel.firstColumn]; - applyBorderFormat(cell, borderFormat, [ - 'borderTop', - 'borderBottom', - ]); + collectBorderFormat(cell, ['borderTop', 'borderBottom']); } - applyBorderFormat( + collectBorderFormat( tableModel.rows[sel.lastRow].cells[sel.firstColumn], - borderFormat, ['borderTop'] ); break; } // Single row selection if (singleRow) { - applyBorderFormat( + collectBorderFormat( tableModel.rows[sel.firstRow].cells[ isRtl ? sel.lastColumn : sel.firstColumn ], - borderFormat, ['borderRight'] ); for ( @@ -246,16 +257,12 @@ export function applyTableBorderFormat( colIndex++ ) { const cell = tableModel.rows[sel.firstRow].cells[colIndex]; - applyBorderFormat(cell, borderFormat, [ - 'borderLeft', - 'borderRight', - ]); + collectBorderFormat(cell, ['borderLeft', 'borderRight']); } - applyBorderFormat( + collectBorderFormat( tableModel.rows[sel.firstRow].cells[ isRtl ? sel.firstColumn : sel.lastColumn ], - borderFormat, ['borderLeft'] ); break; @@ -263,35 +270,31 @@ export function applyTableBorderFormat( // For multiple rows and columns selections // Top left cell - applyBorderFormat( + collectBorderFormat( tableModel.rows[sel.firstRow].cells[ isRtl ? sel.lastColumn : sel.firstColumn ], - borderFormat, ['borderBottom', 'borderRight'] ); // Top right cell - applyBorderFormat( + collectBorderFormat( tableModel.rows[sel.firstRow].cells[ isRtl ? sel.firstColumn : sel.lastColumn ], - borderFormat, ['borderBottom', 'borderLeft'] ); // Bottom left cell - applyBorderFormat( + collectBorderFormat( tableModel.rows[sel.lastRow].cells[ isRtl ? sel.lastColumn : sel.firstColumn ], - borderFormat, ['borderTop', 'borderRight'] ); // Bottom right cell - applyBorderFormat( + collectBorderFormat( tableModel.rows[sel.lastRow].cells[ isRtl ? sel.firstColumn : sel.lastColumn ], - borderFormat, ['borderTop', 'borderLeft'] ); // First row @@ -301,7 +304,7 @@ export function applyTableBorderFormat( colIndex++ ) { const cell = tableModel.rows[sel.firstRow].cells[colIndex]; - applyBorderFormat(cell, borderFormat, [ + collectBorderFormat(cell, [ 'borderBottom', 'borderLeft', 'borderRight', @@ -314,7 +317,7 @@ export function applyTableBorderFormat( colIndex++ ) { const cell = tableModel.rows[sel.lastRow].cells[colIndex]; - applyBorderFormat(cell, borderFormat, [ + collectBorderFormat(cell, [ 'borderTop', 'borderLeft', 'borderRight', @@ -327,7 +330,7 @@ export function applyTableBorderFormat( rowIndex++ ) { const cell = tableModel.rows[rowIndex].cells[sel.firstColumn]; - applyBorderFormat(cell, borderFormat, [ + collectBorderFormat(cell, [ 'borderTop', 'borderBottom', isRtl ? 'borderLeft' : 'borderRight', @@ -340,7 +343,7 @@ export function applyTableBorderFormat( rowIndex++ ) { const cell = tableModel.rows[rowIndex].cells[sel.lastColumn]; - applyBorderFormat(cell, borderFormat, [ + collectBorderFormat(cell, [ 'borderTop', 'borderBottom', isRtl ? 'borderRight' : 'borderLeft', @@ -365,6 +368,20 @@ export function applyTableBorderFormat( } } + // Compare before changing any cells, and only consider borders targeted by + // this operation. Normalize CSS so DOM colors (rgb) also match hex input. + if ( + operation != 'noBorders' && + borderUpdates.length > 0 && + hasMatchingBorders(borderUpdates, borderFormat, editor) + ) { + borderFormat = ''; + } + + for (const { cell, positions } of borderUpdates) { + applyBorderFormat(cell, borderFormat, positions); + } + //Format perimeter if necessary or possible modifyPerimeter(tableModel, sel, borderFormat, perimeter, isRtl); } @@ -386,6 +403,57 @@ export function applyTableBorderFormat( ); } +/** + * Check targeted borders without repeatedly parsing identical CSS values. + * @param borderUpdates The borders to compare + * @param borderFormat The requested border format + * @param editor The editor providing the document for CSS normalization + */ +function hasMatchingBorders( + borderUpdates: BorderUpdate[], + borderFormat: string, + editor: IEditor +): boolean { + const matchingBorders = new Set(); + let comparisonStyle: CSSStyleDeclaration | undefined; + let normalizedBorder = borderFormat; + + for (const { cell, positions } of borderUpdates) { + for (const pos of positions) { + const value = cell.format[pos] || ''; + + // Exact matches need no CSS parsing. Equivalent values are parsed only once. + if (value == borderFormat || matchingBorders.has(value)) { + continue; + } + + if (!comparisonStyle) { + comparisonStyle = editor.getDocument().createElement('div').style; + normalizedBorder = normalizeBorder(borderFormat, comparisonStyle); + } + + if (normalizeBorder(value, comparisonStyle) != normalizedBorder) { + return false; + } + + matchingBorders.add(value); + } + } + + return true; +} + +/** + * Normalize a border value using the browser's CSS serialization. + * @param value The border value to normalize + * @param comparisonStyle A reusable style declaration for border comparison + */ +function normalizeBorder(value: string, comparisonStyle: CSSStyleDeclaration): string { + comparisonStyle.border = ''; + comparisonStyle.border = value; + return comparisonStyle.border || value; +} + /** * @internal * Apply border format to a cell diff --git a/packages/roosterjs-content-model-api/test/publicApi/table/applyTableBorderFormatTest.ts b/packages/roosterjs-content-model-api/test/publicApi/table/applyTableBorderFormatTest.ts index 20b9aba56038..b424705770c0 100644 --- a/packages/roosterjs-content-model-api/test/publicApi/table/applyTableBorderFormatTest.ts +++ b/packages/roosterjs-content-model-api/test/publicApi/table/applyTableBorderFormatTest.ts @@ -44,6 +44,7 @@ describe('applyTableBorderFormat', () => { spyOn(normalizeTable, 'normalizeTable'); editor = ({} as any) as IEditor; + editor.getDocument = () => document; }); function runTest( @@ -78,6 +79,206 @@ describe('applyTableBorderFormat', () => { blocks: [expectedTable], }); } + describe('toggle borders', () => { + const operations: BorderOperations[] = [ + 'allBorders', + 'outsideBorders', + 'insideBorders', + 'topBorders', + 'bottomBorders', + 'leftBorders', + 'rightBorders', + ]; + const positions = ['borderTop', 'borderBottom', 'borderLeft', 'borderRight'] as const; + const originalBorder = '1px solid red'; + + function applyToTable( + table: ContentModelTable, + operation: BorderOperations, + border: Border = testBorder + ) { + const model = createContentModelDocument(); + model.blocks.push(table); + editor.formatContentModel = jasmine + .createSpy('formatContentModel') + .and.callFake((callback: ContentModelFormatter) => + callback(model, { newEntities: [], deletedEntities: [], newImages: [] }) + ); + applyTableBorderFormat(editor, border, operation); + } + + function copyTable(table: ContentModelTable): ContentModelTable { + return JSON.parse(JSON.stringify(table)); + } + + function spyOnBorderNormalization() { + const style = document.createElement('div').style; + const setBorder = jasmine.createSpy('setBorder').and.callFake((value: string) => { + style.border = value; + }); + // Wrap the native declaration because CSS properties are not ordinary accessors. + const comparisonStyle = { + get border() { + return style.border; + }, + set border(value: string) { + setBorder(value); + }, + }; + const createElement = spyOn(document, 'createElement').and.returnValue(({ + style: comparisonStyle, + } as any) as HTMLElement); + return { setBorder, createElement }; + } + + operations.forEach(operation => { + [false, true].forEach(isRtl => { + // Single cell, row, column, and grids with and without inner cells. + [ + [3, 3], + [3, 5], + [5, 3], + [4, 4], + [4, 5], + [5, 5], + ].forEach(([rows, columns]) => { + it(`${operation}, ${rows - 2}x${ + columns - 2 + }, RTL=${isRtl}: toggle off and on`, () => { + const table = createTestTable(rows, columns, { + borderTop: originalBorder, + borderBottom: originalBorder, + borderLeft: originalBorder, + borderRight: originalBorder, + }); + table.format.direction = isRtl ? 'rtl' : 'ltr'; + applyToTable(table, operation); + + const appliedTable = copyTable(table); + const clearedTable = copyTable(table); + + clearedTable.rows.forEach(row => + row.cells.forEach(cell => + positions.forEach(pos => { + if (cell.format[pos] == testBorderString) { + cell.format[pos] = ''; + } + }) + ) + ); + + // Untargeted borders and metadata must stay unchanged, while shared + // borders on cells outside the selection must also be cleared. + runTest(table, clearedTable, testBorder, operation); + runTest(table, appliedTable, testBorder, operation); + }); + }); + }); + + it(`${operation}: apply to the whole selection when one targeted border differs`, () => { + const table = createTestTable(5, 5); + applyToTable(table, operation); + const expectedTable = copyTable(table); + const cell = + table.rows[operation == 'bottomBorders' ? 3 : 1].cells[ + operation == 'rightBorders' ? 3 : 1 + ]; + const position = positions.filter(pos => cell.format[pos] == testBorderString)[0]; + + expect(position).toBeDefined(); + cell.format[position] = originalBorder; + runTest(table, expectedTable, testBorder, operation); + }); + }); + + it('ignores adjacent unselected cells when deciding whether to remove borders', () => { + const table = createTestTable(3, 3, { borderTop: testBorderString }); + const expectedTable = copyTable(table); + expectedTable.rows[1].cells[1].format.borderTop = ''; + expectedTable.rows[0].cells[1].format.borderBottom = ''; + expectedTable.rows[1].cells[1].dataset.editingInfo = '{"borderOverride":true}'; + expectedTable.rows[0].cells[1].dataset.editingInfo = '{"borderOverride":true}'; + + runTest(table, expectedTable, testBorder, 'topBorders'); + }); + + it('matches equivalent CSS borders after a DOM round trip', () => { + const table = createTestTable(3, 3); + applyToTable(table, 'outsideBorders'); + const expectedTable = copyTable(table); + + table.rows.forEach((row, rowIndex) => + row.cells.forEach((cell, colIndex) => + positions.forEach(pos => { + if (cell.format[pos] == testBorderString) { + cell.format[pos] = '3px double rgb(170, 187, 204)'; + expectedTable.rows[rowIndex].cells[colIndex].format[pos] = ''; + } + }) + ) + ); + + runTest(table, expectedTable, testBorder, 'outsideBorders'); + }); + + it('does not access the document when all targeted borders match exactly', () => { + const table = createTestTable(5, 5); + applyToTable(table, 'allBorders'); + const getDocument = spyOn(editor, 'getDocument').and.callThrough(); + + applyToTable(table, 'allBorders'); + + expect(getDocument).not.toHaveBeenCalled(); + expect(table.rows[1].cells[1].format.borderTop).toBe(''); + }); + + it('normalizes repeated equivalent borders only once per operation', () => { + const rgbBorder = '3px double rgb(170, 187, 204)'; + const table = createTestTable(5, 5, { + borderTop: rgbBorder, + borderBottom: rgbBorder, + borderLeft: rgbBorder, + borderRight: rgbBorder, + }); + const { setBorder, createElement } = spyOnBorderNormalization(); + + applyToTable(table, 'allBorders'); + + expect(createElement).toHaveBeenCalledTimes(1); + // Clear and assign once for the requested border and once for the RGB value. + expect(setBorder).toHaveBeenCalledTimes(4); + expect(table.rows[1].cells[1].format.borderTop).toBe(''); + expect(table.rows[3].cells[3].format.borderBottom).toBe(''); + }); + + it('stops comparing at the first mismatch', () => { + const table = createTestTable(5, 5, { borderTop: originalBorder }); + const { setBorder } = spyOnBorderNormalization(); + + applyToTable(table, 'allBorders'); + + expect(setBorder).toHaveBeenCalledTimes(4); + expect(table.rows[3].cells[3].format.borderBottom).toBe(testBorderString); + }); + + it('uses table border defaults when toggling', () => { + const table = createTestTable(3, 3); + table.format.borderTop = testBorderString; + applyToTable(table, 'topBorders', {}); + const expectedTable = copyTable(table); + expectedTable.rows[1].cells[1].format.borderTop = ''; + expectedTable.rows[0].cells[1].format.borderBottom = ''; + + runTest(table, expectedTable, {}, 'topBorders'); + }); + + it('noBorders keeps borders removed when applied repeatedly', () => { + const table = createTestTable(4, 4); + applyToTable(table, 'noBorders'); + runTest(table, copyTable(table), testBorder, 'noBorders'); + }); + }); + it('All Borders', () => { runTest( createTestTable(4, 4), From dd0b9c3a2eff9ea58ed512cc0242ea66506887eb Mon Sep 17 00:00:00 2001 From: Julia Roldi Date: Mon, 14 Sep 2026 16:08:56 -0300 Subject: [PATCH 2/2] border comparation --- .../publicApi/table/applyTableBorderFormat.ts | 41 +++++++++---------- .../table/applyTableBorderFormatTest.ts | 30 +------------- 2 files changed, 21 insertions(+), 50 deletions(-) diff --git a/packages/roosterjs-content-model-api/lib/publicApi/table/applyTableBorderFormat.ts b/packages/roosterjs-content-model-api/lib/publicApi/table/applyTableBorderFormat.ts index e65561d000fc..0c481c415eb5 100644 --- a/packages/roosterjs-content-model-api/lib/publicApi/table/applyTableBorderFormat.ts +++ b/packages/roosterjs-content-model-api/lib/publicApi/table/applyTableBorderFormat.ts @@ -5,6 +5,7 @@ import { mutateBlock, getTableMetadata, parseValueWithUnit, + parseColor, setFirstColumnFormatBorders, updateTableCellMetadata, } from 'roosterjs-content-model-dom'; @@ -373,7 +374,7 @@ export function applyTableBorderFormat( if ( operation != 'noBorders' && borderUpdates.length > 0 && - hasMatchingBorders(borderUpdates, borderFormat, editor) + hasMatchingBorders(borderUpdates, borderFormat) ) { borderFormat = ''; } @@ -407,16 +408,9 @@ export function applyTableBorderFormat( * Check targeted borders without repeatedly parsing identical CSS values. * @param borderUpdates The borders to compare * @param borderFormat The requested border format - * @param editor The editor providing the document for CSS normalization */ -function hasMatchingBorders( - borderUpdates: BorderUpdate[], - borderFormat: string, - editor: IEditor -): boolean { +function hasMatchingBorders(borderUpdates: BorderUpdate[], borderFormat: string): boolean { const matchingBorders = new Set(); - let comparisonStyle: CSSStyleDeclaration | undefined; - let normalizedBorder = borderFormat; for (const { cell, positions } of borderUpdates) { for (const pos of positions) { @@ -427,12 +421,7 @@ function hasMatchingBorders( continue; } - if (!comparisonStyle) { - comparisonStyle = editor.getDocument().createElement('div').style; - normalizedBorder = normalizeBorder(borderFormat, comparisonStyle); - } - - if (normalizeBorder(value, comparisonStyle) != normalizedBorder) { + if (!areSameBorders(value, borderFormat)) { return false; } @@ -444,14 +433,22 @@ function hasMatchingBorders( } /** - * Normalize a border value using the browser's CSS serialization. - * @param value The border value to normalize - * @param comparisonStyle A reusable style declaration for border comparison + * Compare border components, parsing colors to account for equivalent hex and RGB values. */ -function normalizeBorder(value: string, comparisonStyle: CSSStyleDeclaration): string { - comparisonStyle.border = ''; - comparisonStyle.border = value; - return comparisonStyle.border || value; +function areSameBorders(border1: string, border2: string): boolean { + const values1 = extractBorderValues(border1); + const values2 = extractBorderValues(border2); + const color1 = values1.color || ''; + const color2 = values2.color || ''; + const rgb1 = parseColor(color1); + const rgb2 = parseColor(color2); + + return ( + values1.width == values2.width && + values1.style == values2.style && + (color1 == color2 || + (!!rgb1 && !!rgb2 && rgb1[0] == rgb2[0] && rgb1[1] == rgb2[1] && rgb1[2] == rgb2[2])) + ); } /** diff --git a/packages/roosterjs-content-model-api/test/publicApi/table/applyTableBorderFormatTest.ts b/packages/roosterjs-content-model-api/test/publicApi/table/applyTableBorderFormatTest.ts index b424705770c0..a9b20ad68db3 100644 --- a/packages/roosterjs-content-model-api/test/publicApi/table/applyTableBorderFormatTest.ts +++ b/packages/roosterjs-content-model-api/test/publicApi/table/applyTableBorderFormatTest.ts @@ -111,26 +111,6 @@ describe('applyTableBorderFormat', () => { return JSON.parse(JSON.stringify(table)); } - function spyOnBorderNormalization() { - const style = document.createElement('div').style; - const setBorder = jasmine.createSpy('setBorder').and.callFake((value: string) => { - style.border = value; - }); - // Wrap the native declaration because CSS properties are not ordinary accessors. - const comparisonStyle = { - get border() { - return style.border; - }, - set border(value: string) { - setBorder(value); - }, - }; - const createElement = spyOn(document, 'createElement').and.returnValue(({ - style: comparisonStyle, - } as any) as HTMLElement); - return { setBorder, createElement }; - } - operations.forEach(operation => { [false, true].forEach(isRtl => { // Single cell, row, column, and grids with and without inner cells. @@ -202,7 +182,7 @@ describe('applyTableBorderFormat', () => { runTest(table, expectedTable, testBorder, 'topBorders'); }); - it('matches equivalent CSS borders after a DOM round trip', () => { + it('matches borders with equivalent hex and RGB colors', () => { const table = createTestTable(3, 3); applyToTable(table, 'outsideBorders'); const expectedTable = copyTable(table); @@ -232,7 +212,7 @@ describe('applyTableBorderFormat', () => { expect(table.rows[1].cells[1].format.borderTop).toBe(''); }); - it('normalizes repeated equivalent borders only once per operation', () => { + it('matches repeated equivalent borders', () => { const rgbBorder = '3px double rgb(170, 187, 204)'; const table = createTestTable(5, 5, { borderTop: rgbBorder, @@ -240,24 +220,18 @@ describe('applyTableBorderFormat', () => { borderLeft: rgbBorder, borderRight: rgbBorder, }); - const { setBorder, createElement } = spyOnBorderNormalization(); applyToTable(table, 'allBorders'); - expect(createElement).toHaveBeenCalledTimes(1); - // Clear and assign once for the requested border and once for the RGB value. - expect(setBorder).toHaveBeenCalledTimes(4); expect(table.rows[1].cells[1].format.borderTop).toBe(''); expect(table.rows[3].cells[3].format.borderBottom).toBe(''); }); it('stops comparing at the first mismatch', () => { const table = createTestTable(5, 5, { borderTop: originalBorder }); - const { setBorder } = spyOnBorderNormalization(); applyToTable(table, 'allBorders'); - expect(setBorder).toHaveBeenCalledTimes(4); expect(table.rows[3].cells[3].format.borderBottom).toBe(testBorderString); });