[Windows] Ignore case optionally in `AXPlatformNodeTextRangeProviderWin::FindText` #39922

yaakovschectman · 2023-02-27T20:41:28Z

When ignore_case is true, convert needle and haystack strings to lowercase before performing search,

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

yaakovschectman · 2023-02-27T20:42:45Z

third_party/accessibility/ax/platform/ax_platform_node_textrangeprovider_win.cc

-static bool StringSearch(const std::u16string& search_string,
-                         const std::u16string& find_in,
+static bool StringSearch(std::u16string& search_string,
+                         std::u16string& find_in,


Please let me know if you can think of a better way to allow us to optionally copy the needle and haystack strings and then continue with the rest of the method. I attempted using string_view, but the problem with it is that if we create new copies of the strings within the if statement to convert to lowercase and assign to the views, the underlying memory is freed when the if statement completes.

Rather than us trying to implement a case-insensitive compare, I wonder if we shouldn't just delegate to underlying Win32/macOS APIs. The idea of "case" is really finnicky -- e.g. Chinese, Japanese, Korean don't even have a concept of case, and even in French there have historically been differences between France and Canada's handling of accented characters (though IIRC France now matches Canada's handling), Turkish dotted vs non-dotted 'i' etc.

Does Chromium implement case comparison itself?

If we are going to do it ourselves, we should pass the string by value rather than mutate an incoming string.

On Windows, I see lstrcmpiW which calls out to CompareStringEx, and looks like it does what we'd want.

The notes on lstrcmpiW suggest some of the horrors intrinsic in implementing this sort of thing on our own, e.g. should ignore case be interpreted as ignore kana type in Japan? According to that function's implementation, apparently the answer is yes. e.g. か (hiragana) and カ (katakana) are each the phonetic "ka" sound, just expressed using two different phonetic alphabets.

In the case where the user is requesting a case-insensitive compare for some reason, we probably want to be consistent with what the rest of the system is doing, so I'd be tempted to use a system API for this.

These functions compare strings, and StringSearch needs to search for a substring. Chromium appears to have an entire i18n library which provides a StringSearch method (see https://github.com/chromium/chromium/blob/main/base/i18n/string_search.h#L44 ), and furthermore on a 3rd party icu library.

Looking at their implementation, it looks like they're building on top of ICU and doing things like accent handling etc. As I say, I'd be tempted to avoid trying to recreate ICU/Win32 APIs here - between just languages I'm familiar with (French and Japanese) there are already difficulties, but I imagine a huge ton more in languages that I have no idea about.

fwiw their search implementation does look very standalone and we could probably simply adopt it as an option.

What are you looking at as being standalone? It appears that StringSearch creates and calls to FixedPatternStringSearch, which makes use of at least UStringSearch and UCollator from ICU

Correct, I was looking at string_search.h,cc which depend only on ICU (which we already depend on in Flutter) and a few FML-like utilities (which should be easy to replace).

cbracken · 2023-02-28T18:42:44Z

third_party/accessibility/ax/platform/ax_platform_node_textrangeprovider_win_unittest.cc

  EXPECT_UIA_FIND_TEXT(range, L"more text", false, owner);
-  // EXPECT_UIA_FIND_TEXT(range, L"MoRe TeXt", true, owner);
+  EXPECT_UIA_FIND_TEXT(range, L"MoRe TeXt", true, owner);
  EXPECT_UIA_FIND_TEXT(range, L"more", false, owner);


If we adopt Chromium's ICU-based StringSearch implementation, we should add further tests like checking that eleve is found in the string élève and (if they also do kana-type insensitive comparison, which I don't see any reference to, but I'm not an ICU expert) that カタカナ is found in the string かたかな.

These are nice-to-have for the case where you're searching for a particular piece of text in a document. For example, here's me searching the wiki page for katakana with the string カタカナ ("katakana" written using katakana phonetics) but matching both that string and かたかな ("katakana" written using hiragana phonetics) in the same page:

yaakovschectman · 2023-04-04T17:59:56Z

I expect the current test to fail, I just want to check before messing with the BUILD files more.

flutter-dashboard · 2023-04-04T22:02:45Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #39922 at sha ec9de30

flutter-dashboard · 2023-04-05T13:26:42Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #39922 at sha db91100

yaakovschectman · 2023-04-10T15:53:35Z

@cbracken Now that we are using fuller ICU data on desktop, can you give this PR a look?

cbracken

Looks great - this was a painful one! Thanks for fixing!

…eProviderWin::FindText` (flutter/engine#39922)

…in::FindText` (flutter#39922) When `ignore_case` is `true`, convert needle and haystack strings to lowercase before performing search, flutter/flutter#117013 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

yaakovschectman added accessibility affects: desktop platform-windows labels Feb 27, 2023

yaakovschectman requested review from cbracken and loic-sharma February 27, 2023 20:41

yaakovschectman self-assigned this Feb 27, 2023

yaakovschectman commented Feb 27, 2023

View reviewed changes

yaakovschectman marked this pull request as ready for review February 27, 2023 21:39

yaakovschectman force-pushed the TextUnitFormat branch 2 times, most recently from 2a7732a to 67269ba Compare February 28, 2023 17:28

cbracken reviewed Feb 28, 2023

View reviewed changes

yaakovschectman added 7 commits April 4, 2023 12:25

Ignore case optionally

b4f5cf6

Remove TODOs

9639a80

Formatting

e238dcd

Works but not portable

a993131

Fix mistake, set dir

5b12d1f

Use relative path

c9c9fba

Comments

fa5f1dc

yaakovschectman force-pushed the TextUnitFormat branch from dc61700 to fa5f1dc Compare April 4, 2023 17:58

Formatting

ec9de30

flutter-dashboard bot added the will affect goldens label Apr 4, 2023

Merge branch 'main' into TextUnitFormat

db91100

yaakovschectman requested a review from cbracken April 5, 2023 14:31

yaakovschectman requested review from dkwingsmt and chunhtai April 11, 2023 17:04

cbracken approved these changes Apr 11, 2023

View reviewed changes

yaakovschectman merged commit 8d57e6f into flutter:main Apr 11, 2023
36 of 37 checks passed

yaakovschectman deleted the TextUnitFormat branch April 11, 2023 18:55

engine-flutter-autoroll mentioned this pull request Apr 11, 2023

Roll Flutter Engine from 96cb2f5617bb to 2fbc99f3cf84 (6 revisions) flutter/flutter#124621

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 11, 2023

8d57e6fbb [Windows] Ignore case optionally in `AXPlatformNodeTextRang…

7f12e7c

…eProviderWin::FindText` (flutter/engine#39922)

engine-flutter-autoroll mentioned this pull request Apr 11, 2023

Roll Flutter Engine from 96cb2f5617bb to ef089a64cc30 (7 revisions) flutter/flutter#124634

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 11, 2023

8d57e6fbb [Windows] Ignore case optionally in `AXPlatformNodeTextRang…

dce12d2

…eProviderWin::FindText` (flutter/engine#39922)

engine-flutter-autoroll mentioned this pull request Apr 11, 2023

Roll Flutter Engine from 96cb2f5617bb to ad0e2144a05f (8 revisions) flutter/flutter#124639

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 11, 2023

8d57e6fbb [Windows] Ignore case optionally in `AXPlatformNodeTextRang…

b1e8b2f

…eProviderWin::FindText` (flutter/engine#39922)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Ignore case optionally in `AXPlatformNodeTextRangeProviderWin::FindText` #39922

[Windows] Ignore case optionally in `AXPlatformNodeTextRangeProviderWin::FindText` #39922

yaakovschectman commented Feb 27, 2023

yaakovschectman Feb 27, 2023

cbracken Feb 28, 2023

cbracken Feb 28, 2023 •

edited

cbracken Feb 28, 2023 •

edited

yaakovschectman Feb 28, 2023 •

edited

cbracken Feb 28, 2023 •

edited

cbracken Feb 28, 2023

yaakovschectman Feb 28, 2023

cbracken Feb 28, 2023 •

edited

cbracken Feb 28, 2023 •

edited

yaakovschectman commented Apr 4, 2023

flutter-dashboard bot commented Apr 4, 2023

flutter-dashboard bot commented Apr 5, 2023

yaakovschectman commented Apr 10, 2023

cbracken left a comment

[Windows] Ignore case optionally in AXPlatformNodeTextRangeProviderWin::FindText #39922

[Windows] Ignore case optionally in AXPlatformNodeTextRangeProviderWin::FindText #39922

Conversation

yaakovschectman commented Feb 27, 2023

Pre-launch Checklist

yaakovschectman Feb 27, 2023

Choose a reason for hiding this comment

cbracken Feb 28, 2023

Choose a reason for hiding this comment

cbracken Feb 28, 2023 • edited

Choose a reason for hiding this comment

cbracken Feb 28, 2023 • edited

Choose a reason for hiding this comment

yaakovschectman Feb 28, 2023 • edited

Choose a reason for hiding this comment

cbracken Feb 28, 2023 • edited

Choose a reason for hiding this comment

cbracken Feb 28, 2023

Choose a reason for hiding this comment

yaakovschectman Feb 28, 2023

Choose a reason for hiding this comment

cbracken Feb 28, 2023 • edited

Choose a reason for hiding this comment

cbracken Feb 28, 2023 • edited

Choose a reason for hiding this comment

yaakovschectman commented Apr 4, 2023

flutter-dashboard bot commented Apr 4, 2023

flutter-dashboard bot commented Apr 5, 2023

yaakovschectman commented Apr 10, 2023

cbracken left a comment

Choose a reason for hiding this comment

[Windows] Ignore case optionally in `AXPlatformNodeTextRangeProviderWin::FindText` #39922

[Windows] Ignore case optionally in `AXPlatformNodeTextRangeProviderWin::FindText` #39922

cbracken Feb 28, 2023 •

edited

cbracken Feb 28, 2023 •

edited

yaakovschectman Feb 28, 2023 •

edited

cbracken Feb 28, 2023 •

edited

cbracken Feb 28, 2023 •

edited

cbracken Feb 28, 2023 •

edited