-
Notifications
You must be signed in to change notification settings - Fork 956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Ctrl+Backspace handler with text selection logic #629
Conversation
Codecov Report
@@ Coverage Diff @@
## master #629 +/- ##
===================================================
+ Coverage 32.19248% 32.34852% +0.15605%
===================================================
Files 1097 1097
Lines 297585 297757 +172
Branches 38375 38388 +13
===================================================
+ Hits 95800 96320 +520
+ Misses 197591 197227 -364
- Partials 4194 4210 +16
|
@ArrowCase What are the current differences? We should enumerate them |
What about left to right? |
It may also be prudent to ensure that this selection works under Narrator and other supported Accessibility compliance tools |
The full list of differences is unknowable without the SHAutoComplete source, which I doubt will ever be public in my lifetime. One example of a difference is that about half the characters in the Puncutation-Connector category are treated as word characters by SHAutoComplete, but all of them are treated as non-word by my code. The reason for this is that the underscore (pretty much the only non-esoteric character in that category) is one of the ones SHAutoComplete treats as non-word, so I wanted to match that behavior with minimal special casing.
My wording was unclear, I just meant that Ctrl+Backspace (as with regular Backspace) processes characters from the cursor position toward the start of the input (whether the input is displaying in RTL or LTR mode).
Will do. |
Sorry for the delay, I'll be doing accessibility testing this weekend. |
I tested again with Narrator and there are no differences between TextBox controls with and without this change. There seem to be some minor differences between TextBox and ComboBox, but those were not introduced with my change. I also checked with Inspect and couldn't see any meaningful differences. |
SetSelectedTextInternal("", clearUndo: false); | ||
} | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code is identical to the one in ComboBox.cs
.
Can this be extracted into a shared method and reused across multiple call sites?
The changes should also be tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be ideal, but what would we pass to the shared method? TextBox(Base) and ComboBox don't have a common base type, just many similar properties. Only TextBoxBase has the SetSelectedTextInternal method, as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to open an issue to combine this code so that the state doesn't get lost, but for the purposes of this PR, no need to fix this
@ArrowCase I am going to close this due to inactivity. Please feel free to re-open / request re-open when you are able to address the remaining feedback 😃 |
Addressed comment from @danmosemsft. @zsd4yr Can we reopen? And what if any tests will this change require? |
I think this will require some unit testing, exercising these code routes as expected; also a few gifs where you show the expected behavior running on your machine would be great. I expect that we will also need to ensure some backwards compatibility here, make sure we didn't regress any scenarios on older OS's. I think once you've satisfied the above, we can pass off binaries to our testing folks to check for regressions. You will be asked to make a little write-up of what to test in this regard, and we'll pass that off to them 😄 |
Here's a screen capture of the before/after behavior. Previously, single-line text boxes and combo boxes with certain autocomplete options had a Ctrl+Backspace shortcut provided by Windows. Now, all text boxes have a Ctrl+Backspace shortcut provided by WinForms. Also, in all the "after" examples except for the masked text box, the undo buffer is preserved, whereas it is not preserved in the aforementioned "before" cases. I haven't been able to finish adding tests yet because there are new build issues to wrestle with. |
The animation shows a deletion of single words (e.g. CTRL+BKSP). Could you please confirm it is working for deletion of multiple consecutive words (e.g. hold CTRL and press BKSP,BKSP,BKSP). That's something bit us in the back (gitextensions/gitextensions#6049 (comment)). Also our implementation appears to exhibit "additional features" (or 🐛), like deleting words in a readonly controls (gitextensions/gitextensions#6256 (comment)). So worth ensuring this change isn't doing that too. |
@zsd4yr Tried an initial effort for some unit tests. They're the same between TextBox and ComboBox except for the one that tests a read-only TextBox. As for compatibility testing, I would just recommend doing what I did in the above screen capture. Ctrl+Backspace should delete the selection if it exists, or if no selection exists, it should delete the previous "word." It should never insert a box (ASCII To see the old behavior for comparison, run WinForms on .NET Core/Framework without my changes and create a single-line TextBox with @RussKie On the second line I hold Ctrl and press Backspace repeatedly; on the first line I hold down Ctrl+Backspace. |
Awesome 👍 |
LGTM too 😀 |
Add Ctrl+Backspace, "delete previous word," keyboard shortcut to all text edit controls. Previously, this was only available in single-line TextBoxes with autocomplete enabled, due to weird native autocomplete handling.
Rationale in #259. I attempted this previously with messaging to simulate the equivalent Ctrl+Shift+Left and Backspace key presses (#260), but that approach was tenuous at best.
Instead, this change adds logic used by TextBoxBase and ComboBox to determine what the "previous word" is and then delete it. The behavior is not 100% consistent with the native SHAutoComplete behavior, but it's close.
The logic at a high level is:
For determining a word character, I'm using Unicode character classes equivalent to the
\w
regex class but withUnicodeCategory.ConnectorPunctuation
excluded. This is similar to the SHAutoComplete behavior as it does not consider underscores to be word characters. Surrogate pair characters are always included in the current selection rather than trying to wrestle with UTF-16, which from my testing also seems similar to the old native behavior.The performance when processing huge runs of characters, or when repeatedly invoked by holding down the keys, seems as good as or better than the old SHAutoComplete. Also, the usage of
BeginUpdateInternal()
andEndUpdateInternal()
avoids showing a transient selection, which the old native behavior would sometimes do.Fixes #259