Conversation
JiuqingSong
requested review from
BryanValverdeU,
Copilot,
flyingbee2012,
ianeli1,
juliaroldi and
vinguyen12
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #3280 by ensuring list-level metadata is applied even when an existing <ul> is reused during model-to-DOM rendering, so the content model’s list style stays in sync.
Changes:
- Apply
applyMetadata()during the “reuse existing list level” path forULinhandleList. - Add a unit test ensuring list-level metadata is applied when reusing an existing
ULand that format application isn’t redundantly invoked.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/roosterjs-content-model-dom/lib/modelToDom/handlers/handleList.ts | Applies list-level metadata on the list-reuse path for UL to keep model listStyleType updated. |
| packages/roosterjs-content-model-dom/test/modelToDom/handlers/handleListTest.ts | Adds coverage for metadata application when reusing an existing UL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Apply metadata to list level to make sure list style is correct after rendering | ||
| applyMetadata(itemLevel, context.metadataAppliers.listLevel, itemLevel.format, context); |
There was a problem hiding this comment.
Calling applyMetadata inside the reuse loop uses the current context.listFormat.nodeStack length to determine list depth (see listLevelMetadataApplier computes depth = nodeStack.length - 2). During the reuse loop, nodeStack still includes the full stack depth, so applying metadata for layer 0/1/... will use the deepest depth for every call, producing incorrect listStyleType for nested lists. Consider temporarily adjusting nodeStack to the current layer depth when applying metadata (or otherwise passing the correct depth), and consider applying this for OL as well if metadata can affect ordered list styles.
| // Apply metadata to list level to make sure list style is correct after rendering | |
| applyMetadata(itemLevel, context.metadataAppliers.listLevel, itemLevel.format, context); | |
| // Apply metadata to list level to make sure list style is correct after rendering. | |
| // Temporarily adjust nodeStack length so depth-dependent metadata is computed correctly | |
| const originalLength = nodeStack.length; | |
| try { | |
| // nodeStack[0] is the parent, nodeStack[layer + 1] is the current list level | |
| nodeStack.length = layer + 2; | |
| applyMetadata( | |
| itemLevel, | |
| context.metadataAppliers.listLevel, | |
| itemLevel.format, | |
| context | |
| ); | |
| } finally { | |
| nodeStack.length = originalLength; | |
| } |
| it('List style type should be changed by metadata when there is existing UL to reuse', () => { | ||
| const listItem: ContentModelListItem = { | ||
| blockType: 'BlockGroup', | ||
| blockGroupType: 'ListItem', | ||
| blocks: [ | ||
| { | ||
| blockType: 'Paragraph', | ||
| segments: [ | ||
| { | ||
| segmentType: 'Br', | ||
| format: {}, | ||
| }, | ||
| ], | ||
| format: {}, | ||
| }, | ||
| ], | ||
| levels: [ | ||
| { | ||
| listType: 'UL', | ||
| format: {}, | ||
| dataset: { | ||
| editingInfo: '{"applyListStyleFromLevel":true}', | ||
| }, | ||
| }, | ||
| ], | ||
| formatHolder: { | ||
| segmentType: 'SelectionMarker', | ||
| isSelected: false, | ||
| format: {}, | ||
| }, | ||
| format: {}, | ||
| }; | ||
|
|
||
| context = createModelToDomContext(undefined, { | ||
| metadataAppliers: { | ||
| listLevel: listLevelMetadataApplier, | ||
| }, | ||
| }); | ||
|
|
||
| const existingUL = document.createElement('ul'); | ||
| context.listFormat.nodeStack = [ | ||
| { | ||
| node: parent, | ||
| refNode: null, | ||
| }, | ||
| { | ||
| node: existingUL, | ||
| listType: 'UL', | ||
| dataset: { | ||
| editingInfo: '{"applyListStyleFromLevel":true}', | ||
| }, | ||
| format: {}, | ||
| refNode: null, | ||
| }, | ||
| ]; | ||
|
|
||
| const applyFormatSpy = spyOn(applyFormat, 'applyFormat').and.callThrough(); | ||
| const applyMetadataSpy = spyOn(applyMetadata, 'applyMetadata').and.callThrough(); | ||
|
|
||
| handleList(document, parent, listItem, context, null); | ||
|
|
||
| expect(applyMetadataSpy).toHaveBeenCalledTimes(1); | ||
| expect(applyMetadataSpy).toHaveBeenCalledWith( | ||
| listItem.levels[0], | ||
| context.metadataAppliers.listLevel as any, | ||
| listItem.levels[0].format, | ||
| context | ||
| ); | ||
| expect(applyFormatSpy).not.toHaveBeenCalled(); | ||
|
|
||
| expectHtml(parent.outerHTML, ['<div></div>']); | ||
| expect(context.listFormat).toEqual({ | ||
| threadItemCounts: [], | ||
| nodeStack: [ | ||
| { | ||
| node: parent, | ||
| refNode: null, | ||
| }, | ||
| { | ||
| node: existingUL, | ||
| listType: 'UL', | ||
| dataset: { editingInfo: '{"applyListStyleFromLevel":true}' }, | ||
| format: {}, | ||
| refNode: null, | ||
| }, | ||
| ], | ||
| }); | ||
| expect(listItem.levels[0].format.listStyleType).toBe('disc'); | ||
| }); |
There was a problem hiding this comment.
The new reuse-path test only covers a single-level UL. Since listLevelMetadataApplier derives list style from list depth, adding a nested-list reuse test (multiple levels in nodeStack/levels) would help catch depth-related regressions in the new applyMetadata-on-reuse logic.
2 participants
When render list item, we are applying metadata to change its list style type if need. However, if there is existing UL to reuse, the metadata applying process will be skipped which causes list style type is not updated in model. And #3280 is a result of this issue.
This change always apply metadata when UL is reused, so model will be up to date when render.