Skip to content

Commit 1aadc5d

Browse files
niegowskiniegowski
authored
Merge pull request #18806 from ckeditor/cc/8005
Internal (list): List items should not be rewrapped for unrelated attribute changes.
2 parents 685287f + 3f7f1d6 commit 1aadc5d

File tree

4 files changed

+187
-8
lines changed

4 files changed

+187
-8
lines changed

‎packages/ckeditor5-list/src/list/converters.ts‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,10 @@ export function reconvertItemsOnDataChange(
198198
function collectListItemsToRefresh( listHead: ListElement, changedItems: Set<ModelNode> ) {
199199
const itemsToRefresh = [];
200200
const visited = new Set();
201-
const stack: Array<ListItemAttributesMap> = [];
201+
const stack: Array<{
202+
modelAttributes: ListItemAttributesMap;
203+
modelElement: ListElement;
204+
}> = [];
202205

203206
for ( const { node, previous } of new SiblingListBlocksIterator( listHead ) ) {
204207
if ( visited.has( node ) ) {
@@ -213,10 +216,13 @@ export function reconvertItemsOnDataChange(
213216
}
214217

215218
// Update the stack for the current indent level.
216-
stack[ itemIndent ] = Object.fromEntries(
217-
Array.from( node.getAttributes() )
218-
.filter( ( [ key ] ) => attributeNames.includes( key ) )
219-
);
219+
stack[ itemIndent ] = {
220+
modelAttributes: Object.fromEntries(
221+
Array.from( node.getAttributes() )
222+
.filter( ( [ key ] ) => attributeNames.includes( key ) )
223+
),
224+
modelElement: node
225+
};
220226

221227
// Find all blocks of the current node.
222228
const blocks = getListItemBlocks( node, { direction: 'forward' } );
@@ -271,7 +277,10 @@ export function reconvertItemsOnDataChange(
271277

272278
function doesItemWrappingRequiresRefresh(
273279
item: ModelElement,
274-
stack: Array<ListItemAttributesMap>,
280+
stack: Array<{
281+
modelAttributes: ListItemAttributesMap;
282+
modelElement: ListElement;
283+
}>,
275284
changedItems: Set<ModelNode>
276285
) {
277286
// Items directly affected by some "change" don't need a refresh, they will be converted by their own changes.
@@ -298,7 +307,8 @@ export function reconvertItemsOnDataChange(
298307
const eventName = `checkAttributes:${ isListItemElement ? 'item' : 'list' }` as const;
299308
const needsRefresh = listEditing.fire<ListEditingCheckAttributesEvent>( eventName, {
300309
viewElement: element as ViewElement,
301-
modelAttributes: stack[ indent ]
310+
modelAttributes: stack[ indent ].modelAttributes,
311+
modelReferenceElement: stack[ indent ].modelElement
302312
} );
303313

304314
if ( needsRefresh ) {

‎packages/ckeditor5-list/src/list/listediting.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1002,6 +1002,7 @@ export type ListEditingCheckAttributesEvent = {
10021002
args: [ {
10031003
viewElement: ViewElement & { id?: string };
10041004
modelAttributes: ListItemAttributesMap;
1005+
modelReferenceElement: ListElement;
10051006
} ];
10061007
return: boolean;
10071008
};

‎packages/ckeditor5-list/src/listproperties/listpropertiesediting.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,12 @@ export class ListPropertiesEditing extends Plugin {
127127
// Verify if the list view element (ul or ol) requires refreshing.
128128
listEditing.on<ListEditingCheckAttributesEvent>(
129129
'checkAttributes:list',
130-
( evt, { viewElement, modelAttributes } ) => {
130+
( evt, { viewElement, modelAttributes, modelReferenceElement } ) => {
131131
for ( const strategy of strategies ) {
132+
if ( !strategy.appliesToListItem( modelReferenceElement ) ) {
133+
continue;
134+
}
135+
132136
if ( strategy.getAttributeOnUpcast( viewElement ) != modelAttributes[ strategy.attributeName ] ) {
133137
evt.return = true;
134138
evt.stop();

‎packages/ckeditor5-list/tests/listproperties/listpropertiesediting.js‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { VirtualTestEditor } from '@ckeditor/ckeditor5-core/tests/_utils/virtual
77
import { testUtils } from '@ckeditor/ckeditor5-core/tests/_utils/utils.js';
88
import { Paragraph } from '@ckeditor/ckeditor5-paragraph/src/paragraph.js';
99
import { UndoEditing } from '@ckeditor/ckeditor5-undo/src/undoediting.js';
10+
import { BoldEditing } from '@ckeditor/ckeditor5-basic-styles';
1011
import { _setModelData, _getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model.js';
1112
import { ListPropertiesEditing } from '../../src/listproperties/listpropertiesediting.js';
1213
import { modelList } from '../list/_utils/utils.js';
@@ -1089,4 +1090,167 @@ describe( 'ListPropertiesEditing', () => {
10891090
} );
10901091
} );
10911092
} );
1093+
1094+
describe( 'all styles enabled', () => {
1095+
beforeEach( async () => {
1096+
editor = await VirtualTestEditor.create( {
1097+
plugins: [ Paragraph, ListPropertiesEditing, BoldEditing ],
1098+
list: {
1099+
properties: { styles: true, startIndex: true, reversed: true }
1100+
}
1101+
} );
1102+
1103+
model = editor.model;
1104+
1105+
stubUid();
1106+
} );
1107+
1108+
afterEach( () => {
1109+
return editor.destroy();
1110+
} );
1111+
1112+
it( 'should not reconvert the whole list on single item marker bold set', () => {
1113+
editor.setData(
1114+
'<ul>' +
1115+
'<li>foo 1.</li>' +
1116+
'<li>foo 2.' +
1117+
'<ul>' +
1118+
'<li>foo 2.1.</li>' +
1119+
'<li>foo 2.2.</li>' +
1120+
'</ul>' +
1121+
'</li>' +
1122+
'</ul>'
1123+
);
1124+
1125+
editor.model.change( writer => writer.setSelection( editor.model.document.getRoot().getChild( 2 ), 'in' ) );
1126+
1127+
expect( _getModelData( model ) ).to.equalMarkup(
1128+
'<paragraph listIndent="0" listItemId="a00" listStyle="default" listType="bulleted">' +
1129+
'foo 1.' +
1130+
'</paragraph>' +
1131+
'<paragraph listIndent="0" listItemId="a03" listStyle="default" listType="bulleted">' +
1132+
'foo 2.' +
1133+
'</paragraph>' +
1134+
'<paragraph listIndent="1" listItemId="a01" listStyle="default" listType="bulleted">' +
1135+
'[foo 2.1.]' +
1136+
'</paragraph>' +
1137+
'<paragraph listIndent="1" listItemId="a02" listStyle="default" listType="bulleted">' +
1138+
'foo 2.2.' +
1139+
'</paragraph>'
1140+
);
1141+
1142+
const changeDataListener = sinon.spy( () => {
1143+
const changes = editor.model.document.differ.getChanges();
1144+
1145+
expect( changes.length ).to.equal( 2 );
1146+
1147+
expect( changes[ 0 ].type ).to.equal( 'attribute' );
1148+
expect( changes[ 0 ].attributeKey ).to.equal( 'listItemBold' );
1149+
expect( changes[ 0 ].attributeOldValue ).to.be.null;
1150+
expect( changes[ 0 ].attributeNewValue ).to.be.true;
1151+
expect( changes[ 0 ].range.start.path ).to.deep.equal( [ 2 ] );
1152+
expect( changes[ 0 ].range.end.path ).to.deep.equal( [ 2, 0 ] );
1153+
1154+
expect( changes[ 1 ].type ).to.equal( 'attribute' );
1155+
expect( changes[ 1 ].attributeKey ).to.equal( 'bold' );
1156+
expect( changes[ 1 ].attributeOldValue ).to.be.null;
1157+
expect( changes[ 1 ].attributeNewValue ).to.be.true;
1158+
expect( changes[ 1 ].range.start.path ).to.deep.equal( [ 2, 0 ] );
1159+
expect( changes[ 1 ].range.end.path ).to.deep.equal( [ 2, 8 ] );
1160+
} );
1161+
1162+
editor.model.document.on( 'change:data', changeDataListener );
1163+
1164+
editor.execute( 'bold' );
1165+
1166+
expect( changeDataListener.calledOnce ).to.be.true;
1167+
1168+
expect( _getModelData( model ) ).to.equalMarkup(
1169+
'<paragraph listIndent="0" listItemId="a00" listStyle="default" listType="bulleted">' +
1170+
'foo 1.' +
1171+
'</paragraph>' +
1172+
'<paragraph listIndent="0" listItemId="a03" listStyle="default" listType="bulleted">' +
1173+
'foo 2.' +
1174+
'</paragraph>' +
1175+
'<paragraph listIndent="1" listItemBold="true" listItemId="a01" listStyle="default" listType="bulleted">' +
1176+
'[<$text bold="true">foo 2.1.</$text>]' +
1177+
'</paragraph>' +
1178+
'<paragraph listIndent="1" listItemId="a02" listStyle="default" listType="bulleted">' +
1179+
'foo 2.2.' +
1180+
'</paragraph>'
1181+
);
1182+
} );
1183+
1184+
it( 'should not reconvert the whole list on single item marker bold reset', () => {
1185+
editor.setData(
1186+
'<ul>' +
1187+
'<li>foo 1.</li>' +
1188+
'<li>foo 2.' +
1189+
'<ul>' +
1190+
'<li><b>foo 2.1.</b></li>' +
1191+
'<li>foo 2.2.</li>' +
1192+
'</ul>' +
1193+
'</li>' +
1194+
'</ul>'
1195+
);
1196+
1197+
editor.model.change( writer => writer.setSelection( editor.model.document.getRoot().getChild( 2 ), 'in' ) );
1198+
1199+
expect( _getModelData( model ) ).to.equalMarkup(
1200+
'<paragraph listIndent="0" listItemId="a00" listStyle="default" listType="bulleted">' +
1201+
'foo 1.' +
1202+
'</paragraph>' +
1203+
'<paragraph listIndent="0" listItemId="a03" listStyle="default" listType="bulleted">' +
1204+
'foo 2.' +
1205+
'</paragraph>' +
1206+
'<paragraph listIndent="1" listItemBold="true" listItemId="a01" listStyle="default" listType="bulleted">' +
1207+
'[<$text bold="true">foo 2.1.</$text>]' +
1208+
'</paragraph>' +
1209+
'<paragraph listIndent="1" listItemId="a02" listStyle="default" listType="bulleted">' +
1210+
'foo 2.2.' +
1211+
'</paragraph>'
1212+
);
1213+
1214+
const changeDataListener = sinon.spy( () => {
1215+
const changes = editor.model.document.differ.getChanges();
1216+
1217+
expect( changes.length ).to.equal( 2 );
1218+
1219+
expect( changes[ 0 ].type ).to.equal( 'attribute' );
1220+
expect( changes[ 0 ].attributeKey ).to.equal( 'listItemBold' );
1221+
expect( changes[ 0 ].attributeOldValue ).to.be.true;
1222+
expect( changes[ 0 ].attributeNewValue ).to.be.null;
1223+
expect( changes[ 0 ].range.start.path ).to.deep.equal( [ 2 ] );
1224+
expect( changes[ 0 ].range.end.path ).to.deep.equal( [ 2, 0 ] );
1225+
1226+
expect( changes[ 1 ].type ).to.equal( 'attribute' );
1227+
expect( changes[ 1 ].attributeKey ).to.equal( 'bold' );
1228+
expect( changes[ 1 ].attributeOldValue ).to.be.true;
1229+
expect( changes[ 1 ].attributeNewValue ).to.be.null;
1230+
expect( changes[ 1 ].range.start.path ).to.deep.equal( [ 2, 0 ] );
1231+
expect( changes[ 1 ].range.end.path ).to.deep.equal( [ 2, 8 ] );
1232+
} );
1233+
1234+
editor.model.document.on( 'change:data', changeDataListener );
1235+
1236+
editor.execute( 'bold' );
1237+
1238+
expect( changeDataListener.calledOnce ).to.be.true;
1239+
1240+
expect( _getModelData( model ) ).to.equalMarkup(
1241+
'<paragraph listIndent="0" listItemId="a00" listStyle="default" listType="bulleted">' +
1242+
'foo 1.' +
1243+
'</paragraph>' +
1244+
'<paragraph listIndent="0" listItemId="a03" listStyle="default" listType="bulleted">' +
1245+
'foo 2.' +
1246+
'</paragraph>' +
1247+
'<paragraph listIndent="1" listItemId="a01" listStyle="default" listType="bulleted">' +
1248+
'[foo 2.1.]' +
1249+
'</paragraph>' +
1250+
'<paragraph listIndent="1" listItemId="a02" listStyle="default" listType="bulleted">' +
1251+
'foo 2.2.' +
1252+
'</paragraph>'
1253+
);
1254+
} );
1255+
} );
10921256
} );

0 commit comments

Comments
 (0)