UIA: use full buffer comparison in rects and endpoint setter #6447

carlos-zamora · 2020-06-09T23:08:05Z

In UiaTextRange, _getBufferSize returns an optimized version of the
size of the buffer to be the origin and the last character in the
buffer. This is to improve performance on search or checking if you are
currently on the last word/line.

When setting the endpoint and drawing the bounding rectangles, we should
be retrieving the true buffer size. This is because it is still possible
to create UiaTextRanges that are outside of this optimized size. The
main source of this is ExpandToEnclosingUnit() when the unit is
Document. The end should be the last visible character, but it isn't
because that would break our tests.

This is an incomplete solution. #6986 is a follow up to completely test
and implement the solution.

The crash in #6402 was caused by getting the document range (a range of
the full text buffer), then moving the end by one character. When we
get the document range, we get the optimized size of the buffer (the
position of the last character). Moving by one character is valid
because the buffer still has more to explore. We then crash from
checking if the new position is valid on the optimized size, not the
real size.

REFERENCES

#6986 - follow up to properly handle/test this "end of buffer" problem

Closes #6402

DHowett · 2020-06-10T00:00:55Z

nit: I'd like the title to explain when, not just say "sometimes"

DHowett

Please write tests.
What if they're already at the actual real bottom? Where virtual = actual.

src/types/UiaTextRangeBase.cpp

ghost · 2020-06-17T03:00:23Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

codeofdusk · 2020-06-23T12:43:31Z

Will commenting here prevent an automatic close?

ghost · 2020-06-30T15:00:25Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

codeofdusk · 2020-07-04T18:25:59Z

Might a similar approach be needed to fix #5481?

codeofdusk · 2020-07-08T17:45:20Z

@carlos-zamora Any updates on this PR?

carlos-zamora · 2020-07-08T17:47:52Z

@carlos-zamora Any updates on this PR?

Hey, sorry. I think I'm going to have to reevaluate some of my open accessibility PRs today. Both of them have to do with defining the end of the document to be the last character. Which means we'll have to update the tests too. I'm leaving this open for now, but expect a better plan later today or tomorrow :) Sorry for the delay!

DHowett

Please change the name of this PR.
Your comments explain the what, not the why
I still don't understand the why
So, it's possible to navigate character-by-character off the bottom of the viewport? what happens after that?
What happens when you do this at the actual bottom of the buffer at 9001 lines?

DHowett · 2020-07-20T17:26:33Z

What cases does this open up? When we have an endpoint beyond the visible screen, what happens? What if you move it more? What if you read the range aloud, get its text?

Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>

carlos-zamora · 2020-07-20T22:38:44Z

Please change the name of this PR.
✔

Your comments explain the what, not the why

I still don't understand the why
✔

So, it's possible to navigate character-by-character off the bottom of the viewport? what happens after that?
Navigation still uses the optimized buffer size. So it gets clamped to the populated text area, then moves.

What happens when you do this at the actual bottom of the buffer at 9001 lines?
This is handled the same as above.

What cases does this open up? When we have an endpoint beyond the visible screen, what happens? What if you move it more? What if you read the range aloud, get its text?

Scrolling is of no concern because TermCore clamps the scroll offset to be valid.

The text is still extracted properly as you are only extracting spaces when in the non-populated region of the buffer.

The main concern is that this lacks polish. We should provide more tests (as you requested earlier), but I've moved that work into a follow-up item to ensure we are treating the end of the buffer correctly and wholistically.

ghost · 2020-07-20T23:10:30Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

In UiaTextRange, `_getBufferSize` returns an optimized version of the size of the buffer to be the origin and the last character in the buffer. This is to improve performance on search or checking if you are currently on the last word/line. When setting the endpoint and drawing the bounding rectangles, we should be retrieving the true buffer size. This is because it is still possible to create UiaTextRanges that are outside of this optimized size. The main source of this is `ExpandToEnclosingUnit()` when the unit is `Document`. The end _should_ be the last visible character, but it isn't because that would break our tests. This is an incomplete solution. #6986 is a follow up to completely test and implement the solution. The crash in #6402 was caused by getting the document range (a range of the full text buffer), then moving the end by one character. When we get the document range, we get the optimized size of the buffer (the position of the last character). Moving by one character is valid because the buffer still has more to explore. We then crash from checking if the new position is valid on the **optimized size**, not the **real size**. REFERENCES #6986 - follow up to properly handle/test this "end of buffer" problem Closes #6402 (cherry picked from commit c390b61)

codeofdusk · 2020-07-20T23:32:15Z

The end should be the last visible character, but it isn't
because that would break our tests.

Is there any chance that the end could be made the last visible character here? Or is that the aim of #6986? This would resolve #5481 at least from the NVDA perspective.

DHowett · 2020-07-20T23:37:12Z

That's the aim of #6986. 😄

ghost · 2020-07-21T22:34:52Z

🎉Windows Terminal v1.1.2021.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2020-07-22T16:19:23Z

🎉Windows Terminal v1.1.2021.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2020-07-22T16:20:05Z

🎉Windows Terminal Preview v1.2.2022.0 has been released which incorporates this pull request.:tada:

Handy links:

Carlos Zamora (1) * UIA: use full buffer comparison in rects and endpoint setter (GH-6447) Dan Thompson (2) * Tweaks: normalize TextAttribute method names (adjective form) (GH-6951) * Fix 'bcz exclusive' typo (GH-6938) Dustin L. Howett (4) * Fix VT mouse capture issues in Terminal and conhost (GH-7166) * version: bump to 1.3 on master * Update Cascadia Code to 2007.15 (GH-6958) * Move to the TerminalDependencies NuGet feed (GH-6954) James Holderness (3) * Render the SGR "underlined" attribute in the style of the font (CC-7148) * Add support for the "crossed-out" graphic rendition attribute (CC-7143) * Refactor grid line renderers with support for more line types (CC-7107) Leonard Hecker (1) * Added til::spsc, a lock-free, single-producer/-consumer FIFO queue (CC-6751) Michael Niksa (6) * Update TAEF to 10.57.200731005-develop (GH-7164) * Skip DX invalidation if we've already scrolled an entire screen worth of height (GH-6922) * Commit attr runs less frequently by accumulating length of color run (GH-6919) * Set memory order on slow atomics (GH-6920) * Cache the viewport to make invalidation faster (GH-6918) * Correct comment in this SPSC test as a quick follow up to merge. Related work items: MSFT-28208358

carlos-zamora added Area-Accessibility Issues related to accessibility Product-Terminal The new Windows Terminal. labels Jun 9, 2020

ghost added Issue-Bug It either shouldn't be doing this or needs an investigation. Severity-Crash Crashes are real bad news. labels Jun 9, 2020

DHowett requested changes Jun 10, 2020

View reviewed changes

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

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

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 10, 2020

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jun 17, 2020

ghost removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jun 23, 2020

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jun 30, 2020

ghost removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jul 4, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 8, 2020

carlos-zamora marked this pull request as ready for review July 16, 2020 22:05

carlos-zamora assigned DHowett Jul 20, 2020

DHowett requested changes Jul 20, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 20, 2020

carlos-zamora assigned carlos-zamora and unassigned DHowett Jul 20, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 20, 2020

carlos-zamora and others added 3 commits July 20, 2020 17:19

UIA: retrieve full buffer size sometimes

53bd4c6

constructed --> constrained

acbe2fe

Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>

add TODO comment

02465e0

carlos-zamora changed the title ~~UIA: retrieve full buffer size sometimes~~ UIA: allow full buffer comparison in rects and endpoint setter Jul 20, 2020

carlos-zamora force-pushed the dev/cazamor/acc/narrator-crash branch from 82c01b3 to 02465e0 Compare July 20, 2020 22:38

carlos-zamora assigned DHowett Jul 20, 2020

DHowett approved these changes Jul 20, 2020

View reviewed changes

DHowett changed the title ~~UIA: allow full buffer comparison in rects and endpoint setter~~ UIA: use full buffer comparison in rects and endpoint setter Jul 20, 2020

miniksa approved these changes Jul 20, 2020

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 20, 2020

ghost merged commit c390b61 into master Jul 20, 2020

ghost deleted the dev/cazamor/acc/narrator-crash branch July 20, 2020 23:10

ghost mentioned this pull request Jul 21, 2020

Narrator scan mode crashes terminal #6402

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIA: use full buffer comparison in rects and endpoint setter #6447

UIA: use full buffer comparison in rects and endpoint setter #6447

carlos-zamora commented Jun 9, 2020 •

edited by DHowett

Loading

DHowett commented Jun 10, 2020

DHowett left a comment

ghost commented Jun 17, 2020

codeofdusk commented Jun 23, 2020

ghost commented Jun 30, 2020

codeofdusk commented Jul 4, 2020

codeofdusk commented Jul 8, 2020

carlos-zamora commented Jul 8, 2020

DHowett left a comment

DHowett commented Jul 20, 2020

carlos-zamora commented Jul 20, 2020

ghost commented Jul 20, 2020

codeofdusk commented Jul 20, 2020 •

edited

Loading

DHowett commented Jul 20, 2020

ghost commented Jul 21, 2020

ghost commented Jul 22, 2020

ghost commented Jul 22, 2020

UIA: use full buffer comparison in rects and endpoint setter #6447

UIA: use full buffer comparison in rects and endpoint setter #6447

Conversation

carlos-zamora commented Jun 9, 2020 • edited by DHowett Loading

DHowett commented Jun 10, 2020

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Jun 17, 2020

codeofdusk commented Jun 23, 2020

ghost commented Jun 30, 2020

codeofdusk commented Jul 4, 2020

codeofdusk commented Jul 8, 2020

carlos-zamora commented Jul 8, 2020

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Jul 20, 2020

carlos-zamora commented Jul 20, 2020

ghost commented Jul 20, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

codeofdusk commented Jul 20, 2020 • edited Loading

DHowett commented Jul 20, 2020

ghost commented Jul 21, 2020

ghost commented Jul 22, 2020

ghost commented Jul 22, 2020

carlos-zamora commented Jun 9, 2020 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

codeofdusk commented Jul 20, 2020 •

edited

Loading