diff --git a/packages/ckeditor5-find-and-replace/src/findandreplaceediting.ts b/packages/ckeditor5-find-and-replace/src/findandreplaceediting.ts index dbc061be2a5..c16dff96538 100644 --- a/packages/ckeditor5-find-and-replace/src/findandreplaceediting.ts +++ b/packages/ckeditor5-find-and-replace/src/findandreplaceediting.ts @@ -292,7 +292,7 @@ export default class FindAndReplaceEditing extends Plugin { this.state!.highlightedResult = changedSearchResults[ 0 ]; } else { // If there is already highlight item then refresh highlight offset after appending new items. - this.state!.refreshHighlightOffset(); + this.state!.refreshHighlightOffset( model ); } }; } diff --git a/packages/ckeditor5-find-and-replace/src/findandreplacestate.ts b/packages/ckeditor5-find-and-replace/src/findandreplacestate.ts index e2f1adbeb5b..00f5005f035 100644 --- a/packages/ckeditor5-find-and-replace/src/findandreplacestate.ts +++ b/packages/ckeditor5-find-and-replace/src/findandreplacestate.ts @@ -118,7 +118,7 @@ export default class FindAndReplaceState extends /* #__PURE__ */ ObservableMixin } ); this.on>( 'change:highlightedResult', ( ) => { - this.refreshHighlightOffset(); + this.refreshHighlightOffset( model ); } ); } @@ -149,20 +149,35 @@ export default class FindAndReplaceState extends /* #__PURE__ */ ObservableMixin /** * Refreshes the highlight result offset based on it's index within the result list. */ - public refreshHighlightOffset(): void { + public refreshHighlightOffset( model: Model ): void { const { highlightedResult, results } = this; - const sortMapping = { before: -1, same: 0, after: 1, different: 1 }; if ( highlightedResult ) { - this.highlightedOffset = Array.from( results ) - .sort( ( a, b ) => sortMapping[ a.marker!.getStart().compareWith( b.marker!.getStart() ) ] ) - .indexOf( highlightedResult ) + 1; + this.highlightedOffset = sortSearchResultsByMarkerPositions( model, [ ...results ] ).indexOf( highlightedResult ) + 1; } else { this.highlightedOffset = 0; } } } +/** + * Sorts search results by marker positions. Make sure that the results are sorted in the same order as they appear in the document + * to avoid issues with the `find next` command. Apparently, the order of the results in the state might be different than the order + * of the markers in the model. + */ +export function sortSearchResultsByMarkerPositions( model: Model, results: Array ): Array { + const sortMapping = { before: -1, same: 0, after: 1, different: 1 }; + + // `compareWith` doesn't play well with multi-root documents, so we need to sort results by root name first + // and then sort them within each root. It prevents "random" order of results when the document has multiple roots. + // See more: https://github.com/ckeditor/ckeditor5/pull/17292#issuecomment-2442084549 + return model.document.getRootNames().flatMap( rootName => + results + .filter( result => result.marker!.getStart().root.rootName === rootName ) + .sort( ( a, b ) => sortMapping[ a.marker!.getStart().compareWith( b.marker!.getStart() ) ] ) + ); +} + /** * The callback function used to find matches in the document. */ diff --git a/packages/ckeditor5-find-and-replace/src/replacecommand.ts b/packages/ckeditor5-find-and-replace/src/replacecommand.ts index 33a04d06e8b..bcba98ce0f5 100644 --- a/packages/ckeditor5-find-and-replace/src/replacecommand.ts +++ b/packages/ckeditor5-find-and-replace/src/replacecommand.ts @@ -8,6 +8,7 @@ */ import type { ResultType } from './findandreplace.js'; +import { sortSearchResultsByMarkerPositions } from './findandreplacestate.js'; import { ReplaceCommandBase } from './replacecommandbase.js'; /** @@ -22,6 +23,34 @@ export default class ReplaceCommand extends ReplaceCommandBase { * @fires execute */ public override execute( replacementText: string, result: ResultType ): void { + // We save highlight offset here, as the information about the highlighted result will be lost after the changes. + // + // It happens because result list is partially regenerated if the result is removed from the paragraph. + // Partially means that all sibling result items that are placed in the same paragraph are removed and added again, + // which causes the highlighted result to be malformed (usually it's set to first but it's not guaranteed). + // + // While this saving can be done in editing state, it's better to keep it here, as it's a part of the command logic + // and might be super tricky to implement in multi-root documents. + // + // Keep in mind that the highlighted offset is indexed from 1, as it's displayed to the user. It's why we subtract 1 here. + // + // More info: https://github.com/ckeditor/ckeditor5/issues/16648 + const oldHighlightOffset = Math.max( + this._state!.highlightedOffset - 1, + 0 + ); + this._replace( replacementText, result ); + + // Let's revert the highlight offset to the previous value. + if ( this._state!.results.length ) { + // Highlight offset operates on sorted array, so we need to sort the results first. + // It's not guaranteed that items in state results are sorted, usually they are, but it's not guaranteed when + // the result is removed from the paragraph with other highlighted results. + const sortedResults = sortSearchResultsByMarkerPositions( this.editor.model, [ ...this._state!.results ] ); + + // Just make sure that we don't overflow the results array, so use modulo. + this._state!.highlightedResult = sortedResults[ oldHighlightOffset % sortedResults.length ]; + } } } diff --git a/packages/ckeditor5-find-and-replace/tests/ui/findandreplaceformview.js b/packages/ckeditor5-find-and-replace/tests/ui/findandreplaceformview.js index 750c62ca041..5e3b86b15e9 100644 --- a/packages/ckeditor5-find-and-replace/tests/ui/findandreplaceformview.js +++ b/packages/ckeditor5-find-and-replace/tests/ui/findandreplaceformview.js @@ -1286,7 +1286,7 @@ describe( 'FindAndReplaceFormView', () => { expect( matchCounterElement.textContent ).to.equal( '2 of 3' ); replaceButton.fire( 'execute' ); - expect( matchCounterElement.textContent ).to.equal( '1 of 2' ); + expect( matchCounterElement.textContent ).to.equal( '2 of 2' ); replaceButton.fire( 'execute' ); expect( matchCounterElement.textContent ).to.equal( '1 of 1' );