Skip to content
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.

Sign up for GitHub

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

Merge Link & Image Format when using MergeModel #2681

Merged
merged 9 commits into from
Jun 6, 2024
Merged

Merge Link & Image Format when using MergeModel #2681

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2552c3e
mergeLinkFormat
BryanValverdeU Jun 4, 2024
96ea0c0
Merge branch 'master' into u/bvalverde/mergeLinkFormat
BryanValverdeU Jun 5, 2024
0fbb114
Also fix for images
BryanValverdeU Jun 5, 2024
92dc7d7
Merge branch 'u/bvalverde/mergeLinkFormat' of https://github.com/micr…
BryanValverdeU Jun 5, 2024
5b1c7bd
remove unneeded changes
BryanValverdeU Jun 5, 2024
703d103
Remove more unneeded changes
BryanValverdeU Jun 5, 2024
c3a30e5
Address comment
BryanValverdeU Jun 5, 2024
23b674a
nit
BryanValverdeU Jun 5, 2024
83f4442
Merge branch 'master' into u/bvalverde/mergeLinkFormat
JiuqingSong Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions packages/roosterjs-content-model-dom/lib/modelApi/editing/mergeModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {
ContentModelBlock,
ContentModelBlockFormat,
ContentModelDocument,
ContentModelHyperLinkFormat,
ContentModelListItem,
ContentModelParagraph,
ContentModelSegmentFormat,
Expand Down Expand Up @@ -359,6 +360,14 @@ function applyDefaultFormat(
...paragraphFormat,
...segment.format,
});

if (segment.link) {
segment.link.format = mergeSegmentFormat(
applyDefaultFormatOption,
getSegmentFormatInLinkFormat(format),
segment.link.format
);
}
});

if (applyDefaultFormatOption === 'keepSourceEmphasisFormat') {
Expand All @@ -375,6 +384,27 @@ function mergeBlockFormat(applyDefaultFormatOption: string, block: ReadonlyConte
}
}

/**
* Hyperlink format type definition only contains textColor, backgroundColor and underline.
* So create a minimum object with the styles supported in Hyperlink to be used in merge.
*/
function getSegmentFormatInLinkFormat(
targetFormat: ContentModelSegmentFormat
): ContentModelSegmentFormat {
const result: ContentModelHyperLinkFormat = {};
if (targetFormat.textColor) {
result.textColor = targetFormat.textColor;
}
if (targetFormat.backgroundColor) {
result.backgroundColor = targetFormat.backgroundColor;
}
if (targetFormat.underline) {
result.underline = targetFormat.underline;
}

return result;
}

function mergeSegmentFormat(
applyDefaultFormatOption: 'mergeAll' | 'keepSourceEmphasisFormat',
targetFormat: ContentModelSegmentFormat,
Expand All @@ -383,6 +413,7 @@ function mergeSegmentFormat(
return applyDefaultFormatOption == 'mergeAll'
? { ...targetFormat, ...sourceFormat }
: {
...getFormatWithoutSegmentFormat(sourceFormat),
...targetFormat,
...getSemanticFormat(sourceFormat),
};
Expand All @@ -405,3 +436,29 @@ function getSemanticFormat(segmentFormat: ContentModelSegmentFormat): ContentMod

return result;
}

/**
* Segment format can also contain other type of metadata, for example in Images/Hyperlink,
* we want to preserve these properties when merging format
*/
function getFormatWithoutSegmentFormat(
sourceFormat: ContentModelSegmentFormat
): ContentModelSegmentFormat {
const resultFormat = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just simply copy the format that we want to copy, but not copy all then delete others?

I feel the format we want to keep is less than the ones we want to remove

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, if we need to copy everything other than segment format, you can do like this:

  1. add a object to contains all properties of segment:
// An object to provide keys of required properties of segment format, the do NOT use any of its values
const requiredEmptySegmentFormat: Required<ContentModelSegmentFormat> = {
  fontFamily: null!,
  ...
}
  1. When copy format, delete format with keys of this object
const format = { ... }
getObjectKeys(requiredEmptySegmentFormat).forEach(key => delete format[key]);

So that if we add some new property to segment format in the future, we can still take care of them here since it will break the build.

Copy link
Contributor Author

@BryanValverdeU BryanValverdeU Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, if we need to copy everything other than segment format, you can do like this:

  1. add a object to contains all properties of segment:
// An object to provide keys of required properties of segment format, the do NOT use any of its values
const requiredEmptySegmentFormat: Required<ContentModelSegmentFormat> = {
  fontFamily: null!,
  ...
}
  1. When copy format, delete format with keys of this object
const format = { ... }
getObjectKeys(requiredEmptySegmentFormat).forEach(key => delete format[key]);

So that if we add some new property to segment format in the future, we can still take care of them here since it will break the build.

This looks better, I agree. Changing this.

The problem I saw with Copying only what we need is that ContentModelHyperlinkFormat and ContentModelImageFormat contain a lot more properties, so I thought removing the unneeded properties would be better.
And also in case there is a customized process/applier so we dont remove other styles.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense

...sourceFormat,
};

delete resultFormat.backgroundColor;
delete resultFormat.fontFamily;
delete resultFormat.fontSize;
delete resultFormat.fontWeight;
delete resultFormat.italic;
delete resultFormat.letterSpacing;
delete resultFormat.lineHeight;
delete resultFormat.strikethrough;
delete resultFormat.superOrSubScriptSequence;
delete resultFormat.textColor;
delete resultFormat.underline;

return resultFormat;
}
302 changes: 302 additions & 0 deletions packages/roosterjs-content-model-dom/test/modelApi/editing/mergeModelTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3533,4 +3533,306 @@ describe('mergeModel', () => {
tableContext: undefined,
});
});

it('Merge Link Format with mergeAll option', () => {
const newTarget: ContentModelDocument = {
blockGroupType: 'Document',
blocks: [
{
segments: [
{
isSelected: true,
segmentType: 'SelectionMarker',
format: {
fontFamily: 'Calibri',
fontSize: '11pt',
textColor: '#000000',
},
},
],
segmentFormat: {
fontFamily: 'Calibri',
fontSize: '11pt',
textColor: '#000000',
},
blockType: 'Paragraph',
format: {},
},
],
format: {
fontFamily: 'Calibri',
fontSize: '11pt',
textColor: '#000000',
},
};
const mergeLinkSourceModel: ContentModelDocument = {
blockGroupType: 'Document',
blocks: [
{
blockType: 'Paragraph',
segments: [
{
segmentType: 'Text',
text: 'Work Item 222824',
format: {
fontFamily:
'"Segoe UI VSS (Regular)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", Helvetica, Ubuntu, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol"',
fontSize: '14px',
textColor: 'var(--communication-foreground,rgba(0, 90, 158, 1))',
underline: true,
italic: false,
backgroundColor: 'rgb(32, 31, 30)',
},
link: {
format: {
underline: true,
href: 'https://www.bing.com',
anchorClass: 'bolt-link',
textColor:
'var(--communication-foreground,rgba(0, 90, 158, 1))',
backgroundColor: 'rgb(32, 31, 30)',
borderRadius: '2px',
textAlign: 'start',
},
dataset: {},
},
},
{
segmentType: 'Text',
text: 'Test',
format: {
fontFamily:
'"Segoe UI VSS (Regular)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", Helvetica, Ubuntu, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol"',
fontSize: '14px',
textColor: 'rgb(255, 255, 255)',
italic: false,
backgroundColor: 'rgb(32, 31, 30)',
},
},
],
format: {},
isImplicit: true,
segmentFormat: {
fontFamily:
'"Segoe UI VSS (Regular)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", Helvetica, Ubuntu, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol"',
fontSize: '14px',
},
},
],
};
mergeModel(newTarget, mergeLinkSourceModel, undefined, {
mergeFormat: 'mergeAll',
});

const para = newTarget.blocks[0] as ContentModelParagraph;
expect(para.segments[0].link).toEqual({
format: {
href: 'https://www.bing.com',
anchorClass: 'bolt-link',
borderRadius: '2px',
textAlign: 'start',
textColor: 'var(--communication-foreground,rgba(0, 90, 158, 1))',
backgroundColor: 'rgb(32, 31, 30)',
underline: true,
},
dataset: {},
});
});

it('Merge Link Format with keepSourceEmphasisFormat option', () => {
const targetModel: ContentModelDocument = {
blockGroupType: 'Document',
blocks: [
{
segments: [
{
isSelected: true,
segmentType: 'SelectionMarker',
format: {
fontFamily: 'Calibri',
fontSize: '11pt',
textColor: '#000000',
},
},
],
segmentFormat: {
fontFamily: 'Calibri',
fontSize: '11pt',
textColor: '#000000',
},
blockType: 'Paragraph',
format: {},
},
],
format: {
fontFamily: 'Calibri',
fontSize: '11pt',
textColor: '#000000',
},
};
const sourceModel: ContentModelDocument = {
blockGroupType: 'Document',
blocks: [
{
blockType: 'Paragraph',
segments: [
{
segmentType: 'Text',
text: 'Work Item 222824',
format: {
fontFamily:
'"Segoe UI VSS (Regular)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", Helvetica, Ubuntu, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol"',
fontSize: '14px',
textColor: 'var(--communication-foreground,rgba(0, 90, 158, 1))',
underline: true,
italic: false,
backgroundColor: 'rgb(32, 31, 30)',
},
link: {
format: {
underline: true,
href: 'https://www.bing.com',
anchorClass: 'bolt-link',
textColor:
'var(--communication-foreground,rgba(0, 90, 158, 1))',
backgroundColor: 'rgb(32, 31, 30)',
borderRadius: '2px',
textAlign: 'start',
},
dataset: {},
},
},
{
segmentType: 'Text',
text: 'Test',
format: {
fontFamily:
'"Segoe UI VSS (Regular)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", Helvetica, Ubuntu, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol"',
fontSize: '14px',
textColor: 'rgb(255, 255, 255)',
italic: false,
backgroundColor: 'rgb(32, 31, 30)',
},
},
],
format: {},
isImplicit: true,
segmentFormat: {
fontFamily:
'"Segoe UI VSS (Regular)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", Helvetica, Ubuntu, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol"',
fontSize: '14px',
},
},
],
};

mergeModel(targetModel, sourceModel, undefined, {
mergeFormat: 'keepSourceEmphasisFormat',
});

const para = targetModel.blocks[0] as ContentModelParagraph;
expect(para.segments[0].link).toEqual({
format: {
href: 'https://www.bing.com',
anchorClass: 'bolt-link',
borderRadius: '2px',
textAlign: 'start',
textColor: '#000000',
underline: true,
},
dataset: {},
});
});

it('Keep image width when merging with keepSourceEmphasisFormat', () => {
const targetModel = createContentModelDocument();
const para = createParagraph();
const marker = createSelectionMarker();
para.segments.push(marker);
targetModel.blocks.push(para);

marker.format = {
fontFamily: 'Calibri',
fontSize: '11pt',
textColor: '#000000',
};

const sourceModel = createContentModelDocument();
sourceModel.blocks.push({
blockType: 'Paragraph',
format: {},
segments: [
{
segmentType: 'Image',
format: {
fontFamily: 'Remove this',
fontSize: 'Remove this',
textColor: 'Remove this',
backgroundColor: 'imageColor',
width: 'imageWidth',
maxWidth: 'imageMaxWidth',
height: 'imageHeight',
maxHeight: 'imageMaxHeight',
id: 'imageId',
marginBottom: '0px',
marginLeft: '0px',
marginRight: '0px',
marginTop: '0px',
borderBottom: 'border',
borderBottomLeftRadius: 'border',
borderBottomRightRadius: 'border',
borderLeft: 'border',
borderRadius: 'border',
borderTop: 'border',
borderRight: 'border',
borderTopLeftRadius: 'border',
borderTopRightRadius: 'border',
boxShadow: 'border',
display: 'display',
float: 'float',
minHeight: 'minHeight',
minWidth: 'minWidth',
verticalAlign: 'top',
},
dataset: {},
src: 'https://www.bing.com',
},
],
});

mergeModel(targetModel, sourceModel, undefined, {
mergeFormat: 'keepSourceEmphasisFormat',
});

const block = targetModel.blocks[0] as ContentModelParagraph;
expect(block.segments[0].format).toEqual({
fontFamily: 'Calibri',
fontSize: '11pt',
textColor: '#000000',
width: 'imageWidth',
maxWidth: 'imageMaxWidth',
height: 'imageHeight',
maxHeight: 'imageMaxHeight',
id: 'imageId',
marginBottom: '0px',
marginLeft: '0px',
marginRight: '0px',
marginTop: '0px',
borderBottom: 'border',
borderBottomLeftRadius: 'border',
borderBottomRightRadius: 'border',
borderLeft: 'border',
borderRadius: 'border',
borderTop: 'border',
borderRight: 'border',
borderTopLeftRadius: 'border',
borderTopRightRadius: 'border',
boxShadow: 'border',
display: 'display',
float: 'float',
minHeight: 'minHeight',
minWidth: 'minWidth',
verticalAlign: 'top',
});
});
});
Loading