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.

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

No longer jump to the first item after replacing phrase #17292

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}
};
}
27 changes: 21 additions & 6 deletions packages/ckeditor5-find-and-replace/src/findandreplacestate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ export default class FindAndReplaceState extends /* #__PURE__ */ ObservableMixin
} );

this.on<ObservableChangeEvent<ResultType | null>>( 'change:highlightedResult', ( ) => {
this.refreshHighlightOffset();
this.refreshHighlightOffset( model );
} );
}

Expand Down Expand Up @@ -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<ResultType> ): Array<ResultType> {
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.
*/
Expand Down
26 changes: 26 additions & 0 deletions packages/ckeditor5-find-and-replace/src/replacecommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import type { ResultType } from './findandreplace.js';
import { sortSearchResultsByMarkerPositions } from './findandreplacestate.js';
import { ReplaceCommandBase } from './replacecommandbase.js';

/**
Expand All @@ -22,6 +23,31 @@ 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 ];
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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' );
Expand Down Expand Up @@ -1357,6 +1357,47 @@ describe( 'FindAndReplaceFormView', () => {
editor.execute( 'undo' );
expect( matchCounterElement.textContent ).to.equal( '4 of 4' );
} );

it( 'should keep highlighted offset after replacement', () => {
editor.setData(
`<p>
Chocolate <span style="color:#fff700;">cake</span> bar ice cream topping marzipan.
Powder gingerbread bear claw tootsie roll lollipop marzipan icing bonbon.
</p>
<p>
Chupa chups jelly beans halvah ice cream gingerbread bears candy halvah gummi bears.
cAke dragée dessert chocolate.
</p>
<p>
Sime text with text highlight: <mark class="marker-green">Chocolate</mark> bonbon
<mark class="marker-yellow">Chocolate</mark> ice cream <mark class="marker-blue">Chocolate</mark>
gummies <mark class="pen-green">Chocolate</mark> tootsie roll
</p>`
);

toggleDialog();

// Let's skip the first found item.
findInput.fieldView.value = 'Choco';
findButton.fire( 'execute' );
findNextButton.fire( 'execute' );

// Replace second and third one.
view._replaceInputView.fieldView.value = '###';

replaceButton.fire( 'execute' );
replaceButton.fire( 'execute' );

// And there check if the highlight is still in the right place.
replaceButton.fire( 'execute' );

expect( editor.getData() ).to.be.equal(
'<p>Chocolate cake bar ice cream topping marzipan. Powder gingerbread bear claw tootsie roll' +
' lollipop marzipan icing bonbon.</p><p>Chupa chups jelly beans halvah ice cream gingerbread ' +
'bears candy halvah gummi bears. cAke dragée dessert ###late.</p><p>Sime text with text highlight: ' +
'###late bonbon ###late ice cream Chocolate gummies Chocolate tootsie roll</p>'
);
} );
} );

describe( 'dirty state of the form', () => {
Expand Down