Improve wide glyph support in UIA by carlos-zamora · Pull Request #4946 · microsoft/terminal

carlos-zamora · 2020-03-16T21:09:51Z

Summary of the Pull Request

Added better wide glyph support for UIA. We used to move one cell at a time, so wide glyphs would be read twice.
Converted a few things to use til::point since I'm already here.
fixed telemetry for UIA

PR Checklist

Closes Accessibility: Wide Glyph Detection #1354

Detailed Description of the Pull Request / Additional comments

The text buffer has a concept of word boundaries, so it makes sense to have a concept of glyph boundaries too.

_start and _end in UiaTextRange are now til::point

Validation Steps Performed

Verified using Narrator

carlos-zamora · 2020-03-16T21:15:54Z

@miniksa Found a bug where the following won't work:

COORD inclusiveEnd{ _end };

where _end is a til::point.

After discussing with @DHowett-MSFT, the best and only fix for this is to replace these instances with this:

COORD inclusiveEnd = _end;

What was happening in the first notation is the implicit conversion of til::point to bool to SHORT. The constructor for COORD only sees one SHORT so it thinks the value should be the definition for X, and Y should stay as 0. So we end up getting 1, 0.

@DHowett-MSFT suggested we make the bool operator on these new til types explicit. That'll come in in a new PR in a few minutes. More details then.

miniksa · 2020-03-16T21:30:14Z

@miniksa Michael Niksa FTE Found a bug where the following won't work:
COORD inclusiveEnd{ _end };
where _end is a til::point.

After discussing with @DHowett-MSFT Dustin Howett FTE, the best and only fix for this is to replace these instances with this:
COORD inclusiveEnd = _end;
What was happening in the first notation is the implicit conversion of til::point to bool to SHORT. The constructor for COORD only sees one SHORT so it thinks the value should be the definition for X, and Y should stay as 0. So we end up getting 1, 0.

@DHowett-MSFT Dustin Howett FTE suggested we make the bool operator on these new til types explicit. That'll come in in a new PR in a few minutes. More details then.

Glad you asked Dustin. I forgot about explicit.

DHowett-MSFT · 2020-03-16T22:47:01Z

I'm averse to making these changes during a bug fix.

DHowett-MSFT · 2020-03-16T22:47:14Z

Unless switching to til::point actually helped fix the bug, right?

carlos-zamora · 2020-03-16T22:56:07Z

Unless switching to til::point actually helped fix the bug, right?

:( not really. Can I at least have the new functions in TextBuffer return til::point instead of COORD?

DHowett-MSFT · 2020-03-16T22:58:14Z

Yes, they can take and return til::point. The implicit conversions will make that easy and fun.

src/buffer/out/textBuffer.cpp

src/types/UiaTextRangeBase.cpp

zadjii-msft

This seems like something that's very unittest-able, but I'm not going to block over that

src/buffer/out/textBuffer.cpp

DHowett-MSFT · 2020-03-23T23:43:07Z

@msftbot merge this in 7 minutes

ghost · 2020-03-23T23:43:11Z

Hello @DHowett-MSFT!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Mon, 23 Mar 2020 23:50:09 GMT, which is in 7 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

## Summary of the Pull Request - Added better wide glyph support for UIA. We used to move one _cell_ at a time, so wide glyphs would be read twice. - Converted a few things to use til::point since I'm already here. - fixed telemetry for UIA ## PR Checklist * [x] Closes #1354 ## Detailed Description of the Pull Request / Additional comments The text buffer has a concept of word boundaries, so it makes sense to have a concept of glyph boundaries too. _start and _end in UiaTextRange are now til::point ## Validation Steps Performed Verified using Narrator

Improve wide glyph support in UIA (GH-4946) Add enhanced key support for ConPty (GH-5021) Set DxRenderer non-text alias mode (GH-5149) Reduce CursorChanged Events for Accessibility (GH-5196) Add more object ID tracing for Accessibility (GH-5215) Add SS3 cursor key encoding to ConPty (GH-5383) UIA: Prevent crash from invalid UTR endpoint comparison (GH-5399) Make CodepointWidthDetector::GetWidth faster (CC-3727) add til::math, use it for float conversions to point, size (GH-5150) Add support for renderer backoff, don't FAIL_FAST on 3x failures, add UI (GH-5353) Fix a deadlock and a bounding rects issue in UIA (GH-5385) Don't duplicate spaces from potentially-wrapped EOL-deferred lines (GH-5398) Reimplement the VT tab stop functionality (CC-5173) Clamp parameter values to a maximum of 32767. (CC-5200) Prevent the cursor type being reset when changing the visibility (CC-5251) Make RIS switch back to the main buffer (CC-5248) Add support for the DSR-OS operating status report (CC-5300) Update the virtual bottom location if the cursor moves below it (CC-5317) ci: run spell check in CI, fix remaining issues (CC-4799) (CC-5352) Set Cascadia Code as default font (GH-5121) Show a double width cursor for double width characters (GH-5319) Delegate all character input to the character event handler (CC-4192) Update til::bitmap to use dynamic_bitset<> + libpopcnt (GH-5092) Merged PR 4465022: [Git2Git] Merged PR 4464559: Console: Ingest OSS changes up to e055079 Correct scrolling invalidation region for tmux in pty w/ bitmap (GH-5122) Render row-by-row instead of invalidating entire screen (GH-5185) Make conechokey use ReadConsoleInputW by default (GH-5148) Manually pass mouse wheel messages to TermControls (GH-5131) This fixes C-M-space for WSL but not for Win32, but I'm not sure there's a problem in Win32 quite yet. (GH-5208) Fix copying wrapped lines by implementing better scrolling (GH-5181) Emit lines wrapped due to spaces at the end correctly (GH-5294) Remove unneeded whitespace (CC-5162)

ghost · 2020-04-22T17:17:15Z

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

equetzal · 2020-04-26T05:40:16Z

Is this the feature that makes that this text "<=" seems like this "≤"? 'Cause it would be very useful to deactivate the feature. I use nano in the Windows Terminal instead of using a normal text editor just to see the original characters in the files and this makes that a little hard. So maybe this could be configurable.

DHowett-MSFT · 2020-04-26T05:41:25Z

No, that would be the font ligatures that are in the font we chose by default. You can disable them by setting your font to Consolas. We have resolved this for the next version.

carlos-zamora added Product-Conhost For issues in the Console codebase Area-Accessibility Issues related to accessibility Product-Terminal The new Windows Terminal. labels Mar 16, 2020

DHowett-MSFT reviewed Mar 16, 2020

View reviewed changes

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

DHowett-MSFT reviewed Mar 16, 2020

View reviewed changes

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

src/types/UiaTextRangeBase.cpp Outdated Show resolved Hide resolved

src/types/UiaTextRangeBase.cpp Outdated Show resolved Hide resolved

zadjii-msft approved these changes Mar 20, 2020

View reviewed changes

src/buffer/out/textBuffer.cpp Show resolved Hide resolved

carlos-zamora added 7 commits March 23, 2020 09:43

wide glyph support for UIA

566f47c

also add til::point to the tests

8f2abac

This is why unit tests are necessary

8c922b4

undo making _start/_end points. But leave the bits to make that easier.

e5d1f9b

bring back the autos

98f77de

n-th times the charm

14d1bc7

add tests

83c9fb3

carlos-zamora force-pushed the dev/cazamor/acc/wide-glyph branch from beef1ba to 83c9fb3 Compare March 23, 2020 19:30

DHowett-MSFT suggested changes Mar 23, 2020

View reviewed changes

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 23, 2020

better logic to think good

139ccbd

DHowett-MSFT approved these changes Mar 23, 2020

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 23, 2020

DHowett-MSFT changed the title ~~Improved wide glyph support for UIA~~ Improve wide glyph support in UIA Mar 23, 2020

ghost merged commit a338227 into master Mar 23, 2020

ghost deleted the dev/cazamor/acc/wide-glyph branch March 23, 2020 23:50

ghost mentioned this pull request Apr 22, 2020

Accessibility: Wide Glyph Detection #1354

Closed

1 task

This pull request was closed.

Conversation

carlos-zamora commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

carlos-zamora commented Mar 16, 2020

Uh oh!

miniksa commented Mar 16, 2020

Uh oh!

DHowett-MSFT commented Mar 16, 2020

Uh oh!

DHowett-MSFT commented Mar 16, 2020

Uh oh!

carlos-zamora commented Mar 16, 2020

Uh oh!

DHowett-MSFT commented Mar 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DHowett-MSFT commented Mar 23, 2020

Uh oh!

ghost commented Mar 23, 2020

Uh oh!

ghost commented Apr 22, 2020

Uh oh!

equetzal commented Apr 26, 2020

Uh oh!

DHowett-MSFT commented Apr 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

carlos-zamora commented Mar 16, 2020 •

edited

Loading