Skip to content

Commit

Permalink
Merge pull request #2150 from microsoft/u/juliaroldi/margin-block-on-…
Browse files Browse the repository at this point in the history 
…list

Remove margin block on list
  • Loading branch information
@juliaroldi
juliaroldi authored Nov 6, 2023
2 parents 37e8fb5 + be8f6c9 commit 631900e
Show file tree
Hide file tree
Showing 11 changed files with 249 additions and 1 deletion.
20 changes: 20 additions & 0 deletions ...content-model/roosterjs-content-model-dom/lib/formatHandlers/block/marginFormatHandler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@ export const marginFormatHandler: FormatHandler<MarginFormat> = {
}
}
});

const marginBlockStart = element.style.marginBlockStart || defaultStyle.marginBlockStart;
const marginTop = element.style.marginTop || defaultStyle.marginTop;
if (marginBlockStart && !marginTop) {
format.marginBlockStart = parseValueWithUnit(marginBlockStart) + 'px';
}

const marginBlockEnd = element.style.marginBlockEnd || defaultStyle.marginBlockEnd;
const marginBottom = element.style.marginBottom || defaultStyle.marginBottom;
if (marginBlockEnd && !marginBottom) {
format.marginBlockEnd = parseValueWithUnit(marginBlockEnd) + 'px';
}
},
apply: (format, element, context) => {
MarginKeys.forEach(key => {
Expand All @@ -44,5 +56,13 @@ export const marginFormatHandler: FormatHandler<MarginFormat> = {
element.style[key] = value || '0';
}
});

if (format.marginBlockStart && !format.marginTop) {
element.style.marginBlockStart = format.marginBlockStart;
}

if (format.marginBlockEnd && !format.marginBottom) {
element.style.marginBlockEnd = format.marginBlockEnd;
}
},
};
70 changes: 70 additions & 0 deletions ...nt-model/roosterjs-content-model-dom/test/formatHandlers/block/marginFormatHandlerTest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,49 @@ describe('marginFormatHandler.parse', () => {
marginLeft: '50px',
});
});

it('Has margin block in CSS', () => {
div.style.marginBlockEnd = '1px';
div.style.marginBlockStart = '1px';
marginFormatHandler.parse(format, div, context, {});
expect(format).toEqual({
marginBlockEnd: '1px',
marginBlockStart: '1px',
});
});

it('Has margin block in default style', () => {
marginFormatHandler.parse(format, div, context, {
marginBlockEnd: '1em',
marginBlockStart: '1em',
});
expect(format).toEqual({
marginBlockEnd: '0px',
marginBlockStart: '0px',
});
});

it('Merge margin values', () => {
div.style.marginBlockStart = '15pt';
format.marginBlockStart = '30px';
marginFormatHandler.parse(format, div, context, {});
expect(format).toEqual({
marginBlockStart: '20px',
});
});

it('Do not overlay margin values with margin block values', () => {
div.style.margin = '1px 2px 3px 4px';
div.style.marginBlockEnd = '5px';
div.style.marginBlockStart = '6px';
marginFormatHandler.parse(format, div, context, {});
expect(format).toEqual({
marginTop: '1px',
marginRight: '2px',
marginBottom: '3px',
marginLeft: '4px',
});
});
});

describe('marginFormatHandler.apply', () => {
Expand Down Expand Up @@ -110,4 +153,31 @@ describe('marginFormatHandler.apply', () => {
marginFormatHandler.apply(format, div, context);
expect(div.outerHTML).toBe('<div></div>');
});

it('No margin block', () => {
marginFormatHandler.apply(format, div, context);
expect(div.outerHTML).toBe('<div></div>');
});

it('Has margin block', () => {
format.marginBlockEnd = '1px';
format.marginBlockStart = '2px';

marginFormatHandler.apply(format, div, context);

expect(div.outerHTML).toBe('<div style="margin-block: 2px 1px;"></div>');
});

it('Do not overlay margin values with margin block values', () => {
format.marginTop = '1px';
format.marginRight = '2px';
format.marginBottom = '3px';
format.marginLeft = '4px';
format.marginBlockEnd = '5px';
format.marginBlockStart = '6px';

marginFormatHandler.apply(format, div, context);

expect(div.outerHTML).toBe('<div style="margin: 1px 2px 3px 4px;"></div>');
});
});
2 changes: 2 additions & 0 deletions packages-content-model/roosterjs-content-model-editor/lib/modelApi/list/setListType.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ export function setListType(model: ContentModelDocument, listType: 'OL' | 'UL')
direction: block.format.direction,
textAlign: block.format.textAlign,
marginTop: hasIgnoredParagraphBefore ? '0' : undefined,
marginBlockEnd: '0px',
marginBlockStart: '0px',
}),
],
// For list bullet, we only want to carry over these formats from segments:
Expand Down
18 changes: 18 additions & 0 deletions packages-content-model/roosterjs-content-model-editor/test/modelApi/list/setListTypeTest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ describe('indent', () => {
direction: undefined,
textAlign: undefined,
marginTop: undefined,
marginBlockEnd: '0px',
marginBlockStart: '0px',
},
dataset: {},
},
Expand Down Expand Up @@ -299,6 +301,8 @@ describe('indent', () => {
direction: undefined,
textAlign: undefined,
marginTop: undefined,
marginBlockEnd: '0px',
marginBlockStart: '0px',
},
dataset: {},
},
Expand Down Expand Up @@ -362,6 +366,8 @@ describe('indent', () => {
direction: 'rtl',
textAlign: 'start',
marginTop: undefined,
marginBlockEnd: '0px',
marginBlockStart: '0px',
},
},
],
Expand Down Expand Up @@ -441,6 +447,8 @@ describe('indent', () => {
textAlign: undefined,
marginTop: undefined,
marginBottom: '0',
marginBlockEnd: '0px',
marginBlockStart: '0px',
},
},
],
Expand Down Expand Up @@ -469,6 +477,8 @@ describe('indent', () => {
textAlign: undefined,
startNumberOverride: undefined,
marginTop: '0',
marginBlockEnd: '0px',
marginBlockStart: '0px',
},
},
],
Expand Down Expand Up @@ -523,6 +533,8 @@ describe('indent', () => {
direction: undefined,
textAlign: undefined,
marginTop: undefined,
marginBlockEnd: '0px',
marginBlockStart: '0px',
},
},
],
Expand Down Expand Up @@ -579,6 +591,8 @@ describe('indent', () => {
direction: undefined,
textAlign: undefined,
marginTop: undefined,
marginBlockEnd: '0px',
marginBlockStart: '0px',
},
},
],
Expand Down Expand Up @@ -606,6 +620,8 @@ describe('indent', () => {
direction: undefined,
textAlign: undefined,
marginTop: undefined,
marginBlockEnd: '0px',
marginBlockStart: '0px',
},
},
],
Expand Down Expand Up @@ -633,6 +649,8 @@ describe('indent', () => {
direction: undefined,
textAlign: undefined,
startNumberOverride: undefined,
marginBlockEnd: '0px',
marginBlockStart: '0px',
},
},
],
Expand Down
10 changes: 10 additions & 0 deletions packages-content-model/roosterjs-content-model-types/lib/format/formatParts/MarginFormat.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,14 @@ export type MarginFormat = {
* Margin left value
*/
marginLeft?: string;

/**
* Margin-block start value
*/
marginBlockStart?: string;

/**
* Margin-block end value
*/
marginBlockEnd?: string;
};
13 changes: 13 additions & 0 deletions packages/roosterjs-editor-api/lib/utils/toggleListType.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import blockFormat from '../utils/blockFormat';
import { createVListFromRegion, getBlockElementAtNode } from 'roosterjs-editor-dom';
import { ExperimentalFeatures } from 'roosterjs-editor-types';
import type { VList } from 'roosterjs-editor-dom';
import type { BulletListType, IEditor, ListType, NumberingListType } from 'roosterjs-editor-types';
import type {
CompatibleBulletListType,
Expand Down Expand Up @@ -49,6 +50,7 @@ export default function toggleListType(
if (!block) {
return;
}

const vList =
chain && end && start?.equalTo(end)
? chain.createVListAtBlock(block, startNumber)
Expand All @@ -60,6 +62,9 @@ export default function toggleListType(
if (vList && start && end) {
vList.changeListType(start, end, listType);
vList.setListStyleType(orderedStyle, unorderedStyle);
if (isNewList(vList)) {
vList.removeMargins();
}
vList.writeBack(
editor.isFeatureEnabled(ExperimentalFeatures.ReuseAllAncestorListElements),
editor.isFeatureEnabled(ExperimentalFeatures.DisableListChain)
Expand All @@ -70,3 +75,11 @@ export default function toggleListType(
apiNameOverride || 'toggleListType'
);
}

function isNewList(vList: VList | null) {
const list = vList?.rootList;
if (list) {
return list.childElementCount === 0;
}
return false;
}
26 changes: 26 additions & 0 deletions packages/roosterjs-editor-api/test/format/setIndentationTest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,19 @@ describe('setIndentation()', () => {
);
});

it('Indent the first list item in a list with margin-block', () => {
runTest(
'<div><ol style="margin-block:0px;"><li><span id="test">Text</span></li></ol></div>',
() => {
const range = new Range();
range.setStart(editor.getDocument().getElementById('test'), 0);
editor.select(range);
},
Indentation.Increase,
'<div><blockquote style="margin-top:0;margin-bottom:0"><ol style="margin-block:0px;"><li><span id="test">Text</span></li></ol></blockquote></div>'
);
});

it('Outdent the first list item in a list', () => {
runTest(
'<div><blockquote style="margin-top:0;margin-bottom:0"><ol style="list-style-position: inside;"><li><span id="test">Text</span></li></ol></blockquote></div>',
Expand All @@ -58,6 +71,19 @@ describe('setIndentation()', () => {
);
});

it('Outdent the first list item in a list with margin-block', () => {
runTest(
'<div><blockquote style="margin-top:0;margin-bottom:0"><ol style="list-style-position: inside;margin-block:0px;"><li><span id="test">Text</span></li></ol></blockquote></div>',
() => {
const range = new Range();
range.setStart(editor.getDocument().getElementById('test'), 0);
editor.select(range);
},
Indentation.Decrease,
'<div><ol style="list-style-position: inside;margin-block:0px;"><li><span id="test">Text</span></li></ol></div>'
);
});

it('Outdent whole table selected, when no Blockquote wraping table', () => {
runTest(
'<table id="test"><tbody><tr><td><br></td><td><br></td></tr><tr><td><br></td><td><br></td></tr></tbody></table>',
Expand Down
20 changes: 20 additions & 0 deletions packages/roosterjs-editor-api/test/utils/toggleListTypeTest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,24 @@ describe('toggleListTypeTest()', () => {
'<div style="font-family: Arial; font-size: 16pt; color: rgb(0, 111, 201);">default format</div><div style="font-family: Arial; font-size: 16pt; color: rgb(0, 111, 201);"><br></div><div style="font-family: Arial; font-size: 16pt; color: rgb(0, 111, 201);"><ul data-editing-info="{&quot;orderedStyleType&quot;:1,&quot;unorderedStyleType&quot;:1}"><li style="list-style-type: disc;">test</li></ul><div><br></div><ul data-editing-info="{&quot;orderedStyleType&quot;:1,&quot;unorderedStyleType&quot;:1}"><li style="list-style-type: disc;">test</li></ul></div>'
);
});

it('Do not set margin-block when in the middle', () => {
// Arrange
const originalContent =
'<div style="font-family: Arial; font-size: 16pt; color: rgb(0, 111, 201);">default format</div>' +
'<div style="font-family: Arial; font-size: 16pt; color: rgb(0, 111, 201);"><br></div>' +
'<div style="font-family: Arial; font-size: 16pt; color: rgb(0, 111, 201);">' +
'<ul id="list"><li>test</li><li id="focusNode"><br></li><li>test</li></ul>' +
'</div>';
editor.setContent(originalContent);
editor.focus();
editor.select(document.getElementById('focusNode'), PositionType.Begin);

// Act
toggleListType(editor, ListType.Unordered);

// Assert
const ul = editor.getDocument().getElementById('list');
expect(ul?.style.marginBlock).toBe('');
});
});
11 changes: 10 additions & 1 deletion packages/roosterjs-editor-dom/lib/list/VList.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,6 @@ export default class VList {
* @param end End position to operate to
* @param alignment Align items left, center or right
*/

setAlignment(
start: NodePosition,
end: NodePosition,
Expand All @@ -328,6 +327,16 @@ export default class VList {
});
}

/**
* Remove margins of a new list
*/
removeMargins() {
if (!this.rootList.style.marginTop && !this.rootList.style.marginBottom) {
this.rootList.style.marginBlockStart = '0px';
this.rootList.style.marginBlockEnd = '0px';
}
}

/**
* Change list type of the given range of this list.
* If some of the items are not real list item yet, this will make them to be list item with given type
Expand Down
8 changes: 8 additions & 0 deletions packages/roosterjs-editor-dom/lib/list/VListItem.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,14 @@ function createListElement(
result = doc.createElement(listType == ListType.Ordered ? 'ol' : 'ul');
}

if (
originalRoot?.style.marginBlockStart == '0px' &&
originalRoot?.style.marginBlockEnd == '0px'
) {
result.style.marginBlockStart = '0px';
result.style.marginBlockEnd = '0px';
}

// Always maintain the metadata saved in the list
if (originalRoot && nextLevel == 1 && listType != getListTypeFromNode(originalRoot)) {
const style = getMetadata<ListStyleMetadata>(originalRoot, ListStyleDefinitionMetadata);
Expand Down
Loading

0 comments on commit 631900e

Please sign in to comment.