Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fire UIA Events for Output and Cursor Movement #4826

Merged
6 commits merged into from
Mar 18, 2020
Merged

Fire UIA Events for Output and Cursor Movement #4826

6 commits merged into from
Mar 18, 2020

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Mar 6, 2020 •
edited
Loading

Summary of the Pull Request

This notifies automation clients (i.e.: NVDA, narrator, etc...) of new output being rendered to the screen.

References

Close #2447 - Signaling for new output and cursor
Close #3791 - fixed by signaling cursor changes

Detailed Description of the Pull Request / Additional comments

  • Added tracing for UiaRenderer. This makes it easier to debug issues with notifying an automation client.
  • Fire TextChanged automation events when new content is output to the screen.

Validation Steps Performed

Verified with NVDA [1]

Narrator

Narrator works better, but is unable to detect new output consistently. There is no harm for narrator when this change goes in.

[1] #2447 (comment)

@carlos-zamora
Copy link
Member Author

I meant to publish this PR as a draft but I guess there's no undo button...

@codeofdusk codeofdusk mentioned this pull request Mar 6, 2020
Merged
@codeofdusk
Copy link
Contributor

This PR allows NVDA to fully support Terminal. Corresponding NVDA-side changes are in nvaccess/nvda#10784.

@zadjii-msft zadjii-msft added Area-Accessibility Issues related to accessibility Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Mar 9, 2020
zadjii-msft
zadjii-msft reviewed Mar 9, 2020
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose I'm holding my check to see what else narrator needs, but so far looks good

src/types/UiaTracing.cpp Outdated Show resolved Hide resolved
@carlos-zamora carlos-zamora mentioned this pull request Mar 9, 2020
Closed
5 tasks
@DHowett-MSFT
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carlos-zamora carlos-zamora changed the title Fire UIAutomation Events for Output Fire UIA Events for Output and Cursor Movement Mar 13, 2020
@carlos-zamora
Copy link
Member Author

carlos-zamora commented Mar 13, 2020 •
edited
Loading

Given the latest commit, looks like NVDA 2020.1 works fine.
But Narrator gets worse and crashes on any input...

EDIT: this also crashes accevent. So this leads me to believe that something bad is going on with dispatching our CursorChanged event.

@miniksa
Copy link
Member

miniksa commented Mar 17, 2020

This is me noting that I saw this PR and am not reviewing at this time because @carlos-zamora says there's still some nasty crash.

@codeofdusk
Copy link
Contributor

Any chance this PR could be merged without 522b99a and the rest of the event support (for Narrator) Could be added in subsequent PRs? This would allow NVDA to support Terminal.

leonMSFT
leonMSFT approved these changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty straightforward to me 🚀

src/cascadia/TerminalControl/TermControlAutomationPeer.cpp Show resolved Hide resolved
zadjii-msft
zadjii-msft approved these changes Mar 18, 2020
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really love a comment where Leon called out, but sure :shipit:

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 18, 2020
@ghost
Copy link

ghost commented Mar 18, 2020

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 ghost merged commit eca0c6b into master Mar 18, 2020
@ghost ghost deleted the dev/cazamor/acc/sig/output branch March 18, 2020 18:01
DHowett-MSFT pushed a commit that referenced this pull request Apr 14, 2020
@carlos-zamora @DHowett
Fire UIA Events for Output and Cursor Movement (#4826)
1bcc85a 
## Summary of the Pull Request
This notifies automation clients (i.e.: NVDA, narrator, etc...) of new output being rendered to the screen.

## References
Close #2447 - Signaling for new output and cursor
Close #3791 - fixed by signaling cursor changes

## Detailed Description of the Pull Request / Additional comments
- Added tracing for UiaRenderer. This makes it easier to debug issues with notifying an automation client.
- Fire TextChanged automation events when new content is output to the screen.

## Validation Steps Performed
Verified with NVDA [1]

## Narrator
Narrator works _better_, but is unable to detect new output consistently. There is no harm for narrator when this change goes in.

[1] #2447 (comment)
@codeofdusk
Copy link
Contributor

Any idea when this PR will make it to Windows Store users?

@DHowett-MSFT
Copy link
Contributor

Aiming for tomorrow.

@codeofdusk
Copy link
Contributor

Wow, that was good timing!

@ghost
Copy link

ghost commented Apr 22, 2020

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

Handy links:

@ghost ghost mentioned this pull request Apr 22, 2020
Closed
@chinwobble chinwobble mentioned this pull request Sep 20, 2020
Open
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@leonMSFT leonMSFT leonMSFT approved these changes

Assignees
No one assigned
Labels
Area-Accessibility Issues related to accessibility Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Text Cursor Indicator does not move with cursor Accessibility: Signaling Model
6 participants
@carlos-zamora @codeofdusk @DHowett-MSFT @miniksa @zadjii-msft @leonMSFT