Use Intl.Segmenter for the entire-word search matching#21521
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #21521 +/- ##
=======================================
Coverage 89.41% 89.41%
=======================================
Files 262 262
Lines 66738 66704 -34
=======================================
- Hits 59675 59646 -29
+ Misses 7063 7058 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thank you for this @calixteman -- I've tested this locally (in both Firefox and Chromium) and the adjacent dash case appears to be fixed. Here's what I'm seeing for the other two cases:
|
|
Yep I saw these two issues. |
|
IMO, for the whole-words search function it would be better to allow some false positives than false negatives (if those are the two choices) because I think it's always easier (and better) for the user to review and discard false positives than it is for them to find the false negatives; I spent the better part of an hour binary-searching the example PDF while toggling the Whole Words option in order to locate the missing occurrences. Of course, I only did that because I knew they existed; most users would be none the wiser and would simply not find what they were looking for (at unknown cost), unless they disabled Whole Words and dug through the larger set of matches. However, that larger set may be much larger indeed. The example query "property" is not particularly illustrative here, since almost all words containing it as a substring have something to do with "property" the concept, but consider a word like "art"; there are many words containing that substring that have nothing at all to do with "art" the concept, evidenced by the 421 occurrences of that substring in the Free Culture text when Whole Words is disabled, but only 12 occurrences otherwise. For that reason, I think it makes sense to consider Also I may not be clear on what you mean by "it isn't an hyphenation", but I am pretty sure that "intellectual-property" and "special-interest" are correctly hyphenated in the Free Culture text, since they are being used as adjectives rather than nouns. |
timvandermeij
left a comment
There was a problem hiding this comment.
Looks good, with the comment addressed. Good idea to use the built-in Intl.Segmenter for this now that it is broadly available because it makes the logic quite a bit simpler!
`getCharacterType` was a port of the old `WordBreaker::GetClass`, which has been removed and replaced with ICU4X word segmentation. Use `Intl.Segmenter` instead, testing each match boundary on the two adjacent grapheme clusters in isolation like Firefox's find. It fixes mozilla#21520.
getCharacterTypewas a port of the oldWordBreaker::GetClass, which has been removed and replaced with ICU4X word segmentation. UseIntl.Segmenterinstead, testing each match boundary on the two adjacent grapheme clusters in isolation like Firefox's find.It fixes #21520.