Find and replace always replaces the first match when track changes is enabled

[Mati365]

Suggested merge commit message (convention)

Fix (find-and-replace): Do not replace only the first found word when using it with Track Changes plugin.

MINOR BREAKING CHANGE (find-and-replace): The FindCommand#execute now accepts a findCallback that can return an array of results or an object containing an array of results together with the searchText used to invalidate search results in commands like find previous, find next, and replace search.

---

Additional information

Closes: https://github.com/cksource/ckeditor5-commercial/issues/6193 Commercial PR: https://github.com/cksource/ckeditor5-commercial/pull/6275

Debug notes

1. When search is performed, FindCommand#execute is called.
2. It can be called with callback or string as a search parameter.
3. This bug is reproducible if callback is passed as search parameter.
4. Track Changes intercepts find and replace find command execute and forces using callback parameter (it was originally called with string search parameter).
5. FindCommand#execute clears FindAndReplaceState#searchText.
6. If string search parameter is passed, FindAndReplaceState#searchText is being reverted after search (in FindCommand#execute). If callback search parameter is passed, nothing happens and FindAndReplaceState#searchText stays empty.
7. It means that passing callback search parameter resets highlight index here and here, and it's causing an issue. state.searchText is empty (it was cleared in previous steps while using callback search), on the other hand, data.searchText is filled with current input value.

Possible solutions

1. Return searchText along with results in FindCallback. 1.1. It's more flexible if the command is executed with callback which defines the search phrase inside itself [1]. 1.2. It's safer according to tests (none failed). 1.3. It introduces a breaking change in integrations that depends on arguments passed to FindCommand#execute. 1.4. Search UI seems not to be bound to searchText command value. It needs to be refactored, probably. 1.5. It's not super clear for the users why searchText has to be forwarded (despite adding comment).
2. Get rid of state.searchText 2.1. Refactoring whole solution, it's kinda complex and many of the tests will be broken. 2.2. Rename searchText to queryID or something like that. It'll be more obvious for the users. Less dangerous, but under the hood it'll work exactly like 1 solution.
3. Get rid of support callback find. It's an old feature, there are probably a lot of integrations that might depend on this behavior.

[1]

findCommand.execute( () => {
     const callback = typeof callbackOrText === 'string' ? plugin.findByTextCallback( 'Bunny' ) : 'Bunny';
     ...
     return {
        results: ...,
        searchText: 'Bunny'
     };
} );

Reproduction video

https://github.com/ckeditor/ckeditor5/assets/3949262/944f3f63-a1a9-4ecf-adb8-336f93f44b2c

[Mati365]

@DawidKossowski / @oleq / @scofalik Can you take a look?

[f1ames]

I started looking into this PR and the logic in general (well, wanted to understand what's going on before looking into changes itself). Couple of general remarks (not related to the changes itself):

1. The important piece of the puzzle here is what @Mati365 mentioned in:

It means that passing callback search parameter resets highlight index here and here, and it's causing an issue. state.searchText is empty (it was cleared in previous steps while using callback search), on the other hand, data.searchText is filled with current input value.

Since state.searchText it's not there, pressing Replace dialog button, the logic there determines that search term is different than before and reruns find logic (line 79):

https://github.com/ckeditor/ckeditor5/blob/a477c477d5158d0b24059c6e66b504d0b379641b/packages/ckeditor5-find-and-replace/src/findandreplace.ts#L77-L82

This won't happen when using dialog (because Replace button gets disabled when search term changes and user has to perform Find first), but can happen when using API directly.

2. TBH I find the logic of overwriting executeCommand a bit convoluted. In short, the code overwrites original find with own implementation, which calls original find, passing custom find callback, which is then executed multiple times in original find (for each root children). And every time original/custom find is called, we check typeof callbackOrText === 'string' ? ... there. And it's only to filter out deleted parts of the content from find results. I can describe this in more detail if there is time - I know it's not for now but maybe there will be a chance to look into this one day.

3. This is broken for every integration which passes find callback function so not only revision history. I see we don't have other usages like that, and probably haven't received any external requests about this. So maybe it's safe to assume that we can fix/extend how it works to allow future integrations to work correctly (what @Mati365 did here). Not sure if fixing this without touching integrations code would be reasonable/doable.

[Mati365]

@f1ames

I went through the code and I think the direction you @Mati365 proposed is valid 👍

Fixed CR remarks. Can you check again?

1.4. Search UI seems not to be bound to searchText command value. It needs to be refactored, probably.

From what I can see, here: https://github.com/ckeditor/ckeditor5/blob/ck/6193/packages/ckeditor5-find-and-replace/src/ui/findandreplaceformview.ts#L389-L401 we are using plain DOM attributes to accept search text which is later forwarded on submission of form to the state of the search. I'd refactor it to bind the search text value to the state directly and, depending on change events of such text value, clear the history of search results accordingly.

[f1ames]

Found another issue while testing. Not related to the changes from this PR though, reproducible on master too - https://github.com/ckeditor/ckeditor5/issues/16648.

[Mati365]

@f1ames fixed CR remarks

[f1ames]

Fix (find-and-replace): Do not replace only the first found word when using it with Track Changes plugin.

MINOR BREAKING CHANGE (find-and-replace): The FindCommand#execute now accepts a findCallback that can return an array of results or an object containing an array of results together with the searchText used to invalidate search results in commands like find previous, find next, and replace search.

Just a remark here, I don't see it as a breaking change - here, findCallback type was extended, nothing removed, so all previous implementations will work the same way (with the issue as before). So there is a full backwards compatibility in those changes.