Expose Text Attributes to UI Automation by carlos-zamora · Pull Request #10336 · microsoft/terminal

carlos-zamora · 2021-06-04T18:48:54Z

Summary of the Pull Request

This implements GetAttributeValue and FindAttribute for UiaTextRangeBase (the shared ITextRangeProvider for Conhost and Windows Terminal). This also updates UiaTracing to collect more useful information on these function calls.

References

#7000 - Epic
Text Attribute Identifiers
ITextRangeProvider::GetAttributeValue
ITextRangeProvider::FindAttribute

PR Checklist

Closes Feature Idea: Find/Extract Text Attributes for Accessibility #2161
Tests added/passed

Detailed Description of the Pull Request / Additional comments

TextBuffer:
- Exposes a new TextBufferCellIterator that takes in an end position. This simplifies the logic drastically as we can now use this iterator to navigate through the text buffer. The iterator can also expose the position in the buffer.
UiaTextRangeBase:
- Shared logic & helper functions:
  - Most of the text attributes are stored as TextAttributes in the text buffer. To extract them, we generate an attribute verification function via _getAttrVerificationFn(), then use that to verify if a given cell has the desired attribute.
  - A few attributes are special (i.e. font name, font size, and "is read only"), in that they are (1) acquired differently and (2) consistent across the entire text buffer. These are handled separate from the attribute verification function.
- GetAttributeValue: Retrieve the attribute verification of the first cell in the range. Then, verify that the entire range has that attribute by iterating through the text range. If a cell does not have that attribute, return the "reserved mixed attribute value".
- FindAttribute: Iterate through the text range and leverage the attribute verification function to find the first contiguous range with that attribute. Then, make the end exclusive and output a UiaTextRangeBase. This function must be able to perform a search backwards, so we abstract the "start" and "end" into resultFirstAnchor and resultSecondAnchor, then perform post processing to output a valid UiaTextRangeBase.
UiaTracing:
- GetAttributeValue: Log uia text range, desired attribute, resulting attribute metadata, and the type of the result.
- FindAttribute: Log uia text range, desired attribute and attribute metadata, if we were searching backwards, the type of the result, and the resulting text range.
- AttributeType is a nice way to understand/record if the result was either of the reserved UIA values, a normal result, or an error.
UiaTextRangeTests:
- GetAttributeValue:
  - verify that we know which attributes we support
  - test each of the known text attributes (expecting 100% code coverage for _getAttrVerificationFn())
- FindAttribute:
  - test each of the known special text attributes
  - test IsItalic. NOTE: I'm explicitly only testing one of the standard text attributes because the logic is largely the same between all of them and they leverage _getAttrVerificationFn().

Validation Steps Performed

@codeofdusk has been testing this Conhost build
Tests added for Conhost and shared implementation
Windows Terminal changes were manually verified using accessibility insights and NVDA

carlos-zamora · 2021-06-04T18:51:53Z

+        // TODO CARLOS: how do I get the font size in points?
+        if (queryFontSize == 0) //_pData->GetFontInfo().GetUnscaledSize())
+        {
+            Clone(ppRetVal);
+        }


@DHowett or @miniksa Any ideas on how to get the font size in points from FontInfo?

You mean instead of pixels? I believe in the dx renderer is a conversion. You could move it to a helper

I'm gonna punt on this one and file it as a follow-up because...

we don't expect the font size to change within the buffer, so exposing it isn't urgent

I'll have to do a different approach between ConHost and Windows Terminal here. And that just sounds like extra work for an already bulky PR.

carlos-zamora · 2021-06-04T18:59:05Z

Known Issues

font size always returns 0. This is because I need to figure out how to convert the inner representation of the font size to "points".
foreground color is "0" (black). I don't know why.

Things that I should do

Compare this behavior to another app like Microsoft Word

codeofdusk · 2021-06-04T19:47:39Z

Please also consider regression testing against legacy console (i.e. disable the UIA feature flag and check text formatting).

DHowett · 2021-06-16T22:44:13Z

+// - limit - boundaries for the iterator to operate within
+// - until - X,Y position in buffer for last position for the iterator to read (inclusive)


what is the difference between a limit and an until? Why can they be different?

Updated the comment to be more clear. Also renamed...

limit --> bounds

until --> limit

Theoretically, you could have...

bounds = bufferSize & limit = (3,3): iterate from at through limit (explores (4,3) --> (5,3) --> (6,3))

bounds = (5,5) & limit = (3,3): iterate from at through limit across bounds (explores (4,3) --> (5,3) --> (0,4))

DHowett · 2021-06-16T22:45:57Z

+        THROW_IF_FAILED(_uiaProvider->GetAttributeValue(textAttributeId, &result));
+
+        // Convert the resulting VARIANT into a format that is consumable by XAML.
+        switch (result.vt)


I'm surprised that nobody has made a C++ visitor specialization for a variant

DHowett · 2021-06-16T22:54:32Z

+    case UIA_IsReadOnlyAttributeId:
    {
        pRetVal->vt = VT_BOOL;
        pRetVal->boolVal = VARIANT_FALSE;


well this rather depends: the entire buffer is technically "read-only"... it just happens to change when the connected application receives input.

A non-read-only buffer suggests that you could, e.g., select a region and delete it... or change its formatting or something.

The Text Attribute ID docs:

Identifies the IsReadOnly text attribute, which indicates whether the text is read-only (TRUE) or can be modified (FALSE).

@codeofdusk made an interesting suggestion as to the interpretation of that text: append "by the user" at the end. In console, a user can interact with the buffer via key strokes, and those key strokes may modify any section of the buffer. Surprisingly, something like "text on a webpage" is also not read only! IsReadOnly seems to be limited to text that is disabled and cannot be interacted with in any way.

lhecker

I'm convinced that you should the COORD limit logic into the 2 places that need it instead of TextBufferCellIterator.
TextBufferCellIterator at this point is already something of a magic hambeast that does everything and I actually think that concerns are better separated, by not having this logic in the basic iterator. Creating an iterator-adapter for instance would be rather clean, but just doing the loop-early-exit logic in those 2 places is also clean.

DHowett

14/17

DHowett · 2021-06-29T17:59:20Z

    size_t _hyperlinkPatternId;

    std::wstring _workingDirectory;
+    std::optional<FontInfo> _fontInfo;


you may just want to drop the optional and initialize this _fontInfo directly with the fake fontinfo value in TerminalRenderData.cpp:R48.

Then stomp it when the font changes.

qq: why does this go through a value stored on the TerminalCore (which is a cache that could get out of sync) and not through some other component that knows what the font is?

like: TSFInputControl had the same problem -- had to get the font -- and I am concerned that now there are two ways to get the font (new one and old one)

So this is the part that sucks. IUiaData is down in TerminalCore. But all of the font info (_desiredFont and _actualFont) is actually up in ControlCore. Because of this, we need to have ControlCore tell TerminalCore "hey, the font change to X" here.

TSFInputControl looks like it works directly with the TerminalControl (now ControlCore since the split) to get what it needs.

lhecker · 2021-06-29T18:27:21Z

@carlos-zamora @DHowett Since it might be interesting for others as well I'm going to post it here:
If y is a trivial operation, x | y often generates significantly faster assembly than x || y.

C++ requires short-circuit evaluation and thus compilers will absolutely refuse to eagerly evaluate or inline the left-hand side of a boolean operator. For instance if you write boolean = boolean || TrivialFunction() then TrivialFunction()'s code will only ever run if the boolean variable is false, even if it's as trivial as reading a single variable.

You can see the drastic difference here: https://godbolt.org/z/sd9qY8Meh
(And that's despite maximum inline expansion with /Ob3!)

However _bounds.IncrementInBounds(newPos) sounds like a mutating operation. I don't think it should run if _exceeded is true. 😅

DHowett · 2021-06-29T18:33:03Z

However _bounds.IncrementInBounds(newPos) sounds like a mutating operation. I don't think it should run if _exceeded is true. 😅

it appears that you have talked yourself around into wanting short-circuit evaluation after all ;)

miniksa · 2021-07-06T20:00:05Z

@miniksa This code is written in a style that I'd use for interpreted or JITed languages, because in those function call overhead is fairly high, while closure overhead is fairly low.
In C/C++ on the other hand I would suggest leveraging how CPU branch predictors work: constant function calls and constant branches are extremely cheap, while closures and their heap allocations are extremely expensive.

I wrote a suggestion before:

I convinced it would be more ergonomic to call it directly in the loop and have it switch through the switch (attributeId) every time. Since attributeId is constant a modern CPU would have the easiest time of its life to predict the correct switch case.

Derp. Sorry, I missed that.

@miniksa Oh yeah using function pointers would also be possible for sure and would be even more performant...
If there's any state worth preserving for a specific _getAttrVerificationFn we can simply put it into a member of the class and access it through the this pointer.

You mean function pointer like C-style? Or is there a nicer way to encapsulate it?

…lambda for attr verification

carlos-zamora · 2021-07-06T23:28:01Z

Checked in with Leonard offline. I made it so that we don't return the lambda. Instead, we basically "run the lambda" that we would've returned.

Co-authored-by: Michael Niksa <miniksa@microsoft.com>

carlos-zamora · 2021-07-07T20:01:04Z

@DHowett putting this on your radar

@zadjii-msft idk if you wanted to take another look, but I'll leave you as assigned anyways haha.

DHowett

I trust both other signers here, and I did a medium-close read of the code myself. Best way to know is to try it.

ghost · 2021-07-09T22:56:49Z

Hello @carlos-zamora!

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.

ghost · 2021-07-14T17:08:20Z

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

Handy links:

Fix-up of #10964. Summary of the issue: The link to Expose Text Attributes to UI Automation microsoft/terminal#10336 was broken by t2t preproc directives. Additional clarification on "NVDA API levels" might be appreciated by some users.

ghost added Area-Accessibility Issues related to accessibility Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Jun 4, 2021

carlos-zamora requested a review from miniksa June 4, 2021 18:49

carlos-zamora commented Jun 4, 2021

View reviewed changes

carlos-zamora requested a review from DHowett June 4, 2021 18:55

carlos-zamora force-pushed the dev/cazamor/a11y-7000/text-attributes branch from 7807ba6 to 2ae0ab8 Compare June 4, 2021 18:56

codeofdusk reviewed Jun 4, 2021

View reviewed changes

Comment thread src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp Outdated

DHowett reviewed Jun 16, 2021

View reviewed changes

carlos-zamora force-pushed the dev/cazamor/a11y-7000/text-attributes branch from 2ae0ab8 to 2ee0cd8 Compare June 22, 2021 23:29

carlos-zamora marked this pull request as ready for review June 22, 2021 23:30

carlos-zamora force-pushed the dev/cazamor/a11y-7000/text-attributes branch from 2ee0cd8 to a99fa3c Compare June 23, 2021 21:29

carlos-zamora commented Jun 25, 2021

View reviewed changes

Comment thread src/types/UiaTextRangeBase.cpp Outdated

Comment thread src/types/UiaTextRangeBase.cpp Outdated

lhecker reviewed Jun 28, 2021

View reviewed changes

Comment thread src/buffer/out/textBufferCellIterator.cpp Outdated

Comment thread src/types/UiaTextRangeBase.cpp Outdated

Comment thread src/cascadia/TerminalControl/XamlUiaTextRange.cpp Outdated

Comment thread src/cascadia/TerminalControl/XamlUiaTextRange.cpp Outdated

carlos-zamora self-assigned this Jun 28, 2021

miniksa reviewed Jun 28, 2021

View reviewed changes

DHowett reviewed Jun 29, 2021

View reviewed changes

Comment thread src/buffer/out/textBufferCellIterator.cpp Outdated

DHowett reviewed Jun 29, 2021

View reviewed changes

Comment thread src/types/UiaTracing.cpp Outdated

DHowett requested changes Jun 29, 2021

View reviewed changes

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 Jun 29, 2021

carlos-zamora added 4 commits June 29, 2021 16:09

Expose Text Attributes to UI Automation

84bca57

fix/add tests; remove invalid attributes

136a548

address PR feedback

a7bbf9d

remove 'limit' from new iterator

2be632b

fix 1 char wide case; better degenerate range handling; don't return …

3fa8c95

…lambda for attr verification

Merge branch 'main' into dev/cazamor/a11y-7000/text-attributes

7c88010

carlos-zamora assigned DHowett, miniksa and zadjii-msft and unassigned carlos-zamora Jul 6, 2021

miniksa reviewed Jul 7, 2021

View reviewed changes

Comment thread src/types/UiaTextRangeBase.cpp

miniksa approved these changes Jul 7, 2021

View reviewed changes

carlos-zamora and others added 2 commits July 7, 2021 12:48

fix failing tests and audit mode

74a645f

fix typo

2e7895c

Co-authored-by: Michael Niksa <miniksa@microsoft.com>

carlos-zamora unassigned miniksa Jul 7, 2021

DHowett approved these changes Jul 9, 2021

View reviewed changes

Comment thread src/types/UiaTextRangeBase.cpp

Comment thread src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp

carlos-zamora unassigned DHowett and zadjii-msft Jul 9, 2021

apply Dustin's suggestions

96a5551

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 9, 2021

ghost merged commit a0e5085 into main Jul 9, 2021

ghost deleted the dev/cazamor/a11y-7000/text-attributes branch July 9, 2021 23:21

ghost mentioned this pull request Jul 14, 2021

Feature Idea: Find/Extract Text Attributes for Accessibility #2161

Closed

carlos-zamora restored the dev/cazamor/a11y-7000/text-attributes branch July 14, 2021 18:40

carlos-zamora deleted the dev/cazamor/a11y-7000/text-attributes branch July 14, 2021 18:58

codeofdusk mentioned this pull request Oct 22, 2021

UI Automation in Windows Console: make default in FORMATTED consoles (Windows 11 Sun Valley 2) nvaccess/nvda#10964

Merged

codeofdusk mentioned this pull request Jun 21, 2022

UI Automation in Windows Console: fix-up changes for UIA by default nvaccess/nvda#13828

Merged

5 tasks

codeofdusk mentioned this pull request Aug 17, 2022

UIA Formatted Text Navigation #3567

Open

This pull request was closed.

		// - limit - boundaries for the iterator to operate within
		// - until - X,Y position in buffer for last position for the iterator to read (inclusive)

Conversation

carlos-zamora commented Jun 4, 2021

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-zamora commented Jun 4, 2021

Known Issues

Things that I should do

Uh oh!

codeofdusk commented Jun 4, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lhecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker commented Jun 29, 2021 •

edited

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.