Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Jul 14, 2021

This PR changes the current strategy that delays all character events to WM_CHAR, so that non-surrogate events are dispatched at WM_KEYDOWN, and only surrogate events are still dispatched at WM_CHAR. This fixes the issue mentioned at flutter/flutter#78005 (comment), which is caused by #27064.

flutter/flutter#85587 noticed that Ctrl-Digit (such as Ctrl-1) key presses yield non-zero characters but will not be followed by WM_CHAR, causing its following WM_KEYUP to be lost. It tried to fix it by considering all key presses with Control modifier as non-character. However, this fix is flawed for conflicting with AltGr-Letter (Such as AltGr-Q) key presses, which intrinsically come with a CtrlLeft modifier.

This PR basically reverts #27064 and uses a different method. It is noticed that delaying WM_KEYDOWN to WM_CHAR is only for combining surrogates, and the characters of Ctrl-Digit events are BMP characters. Therefore all BMP characters do not need to be delayed, and it will no longer matter if a WM_CHAR follows a WM_KEYDOWN if the character is a BMP.

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.

@dkwingsmt
Copy link
Contributor Author

@stuartmorgan Since you wrote the surrogate combining logic, can you take a look if this approach makes sense?

@stuartmorgan-g
Copy link
Contributor

I had to learn how that worked mostly by trial and error when I was writing it, and I'm afraid what I did learn has been paged out. I have no idea if this will regress something else.

I would say that at this point this code is a case study in the cost of putting off unit tests; we've thrashed quite a bit in this method. Lets start with adding a really robust set of unit tests by looking back at every issue that's been fixed in this text input code and ensuring each item that's been discussed has a unit test, and ensure coverage of any basic behaviors that are still not covered (ascii, arrow keys, delete/backspace, control keys, surrogate pairs, etc.). Then the answer to "does this change make sense" for this code can be driven by unit tests rather than attempting to reason about it, since we've demonstrated pretty thoroughly at this point that none of us understand all the edge cases of this complex input system well enough for the latter to work.

@dkwingsmt
Copy link
Contributor Author

I agree. I'll try to figure out myself. :) Let me know if you can think of any edge cases that might be of interest.

@dkwingsmt
Copy link
Contributor Author

Close in favor for #27921.

@dkwingsmt dkwingsmt closed this Aug 12, 2021
@dkwingsmt dkwingsmt deleted the early-bmp branch January 6, 2022 01:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants