Bug 251054

Hi Team,

While going through Blink's commit, I came across following code removal, which seems to lead to erroneous behavior.

Blink Commit - https://chromium.googlesource.com/chromium/blink/+/857f2e42c139b277ed29cd8ae112cb4f1d6b5a90

WebKit Source - https://searchfox.org/wubkat/source/Source/WebCore/editing/Editor.cpp#3615

Blink Commit Commit Log:

Currently, Editor::RangeOfString() sets the start and end positions for the searchRange depending on the findOptions.findNext option's value. If 'StartInSelection' has been set (for findNext) then the start of the searchRange would point to the start of the current active (matched) selection.
Thus the search would begin from this point and the current active selection will be returned as the result/matched range in case the search keyword is same as for the previous find operation.

But, right after this resultRange is obtained, we have code (in rangeOfString()) that compares the new resultRange with the current active range and if found to be the same (which would be the case when the search keyword remains the same), it resets the start and end positions of the searchRange, this time from after the current active selection, irrespective of whatever the findOptions.findNext value is set to, and then performs another find operation to get a new resultRange. This new resultRange (if found) would thus lie beyond the previous active (matched) selection.

This results in erroneous behavior for scenarios wherein, even though the user/application doesn't move to the next match, simply performing another find operation using the same keyword would unnecessarily increment the activeMatch range as well as count.

I found this code to have been added in the patch: http://trac.webkit.org/changeset/14782. However, the changelog just states: "(WebCore::Frame::findString): Moved from FrameMac. Compare a selection created using the found range with the current selection in case the current selection is the found range minus some collapsed whitespace on the edges."

This change should not cause any of the existing tests (layout as well as unit tests) to fail.

____

Just wanted to raise this for input and if we haven't fixed it over the time, can we remove this as well?

Thanks!