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

UIA Providers misrepresent box selections #4509

Closed
5 tasks done
carlos-zamora opened this issue Feb 8, 2020 · 3 comments · Fixed by #4991
Closed
5 tasks done

UIA Providers misrepresent box selections #4509

carlos-zamora opened this issue Feb 8, 2020 · 3 comments · Fixed by #4991
Labels
Area-Accessibility Issues related to accessibility Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.0

Comments

@carlos-zamora
Copy link
Member

carlos-zamora commented Feb 8, 2020 •
edited
Loading

Summary

Our TermControlUiaProvider (aka ScreenInfoUiaProvider) assumes that a retrieved selection is always a line selection.

Technical Details

ScreenInfoUiaProviderBase::GetSelection() returns a UiaTextRange for the endpoints of the selection (after a brief inclusive/exclusive conversion). We need two modifications here:

  1. ScreenInfoUiaProviderBase should check if the selection is a box selection or a line selection. And should create a different branch for each of those options.
  2. UiaTextRangeBase needs...
  • a constructor for a box selection UTR
  • a flag to know if it is in this style
  • a new bounding rect for a UTR of this style
  • a modified way to retrieve text for this
  • (?) modify Select to make a box selection? not needed. How selections are made is already abstracted to the selection code, not UIA providers
@carlos-zamora carlos-zamora added Product-Conhost For issues in the Console codebase Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Accessibility Issues related to accessibility Product-Terminal The new Windows Terminal. labels Feb 8, 2020
@carlos-zamora carlos-zamora added this to the Terminal v0.10 milestone Feb 8, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 8, 2020
This was referenced Feb 8, 2020
Merged
Closed
@ghost ghost added In-PR This issue has a related PR and removed In-PR This issue has a related PR labels Feb 8, 2020
@carlos-zamora
Copy link
Member Author

Closed the PR attempting to fix this. Ran into the following problem:
Closing this PR. This is the incorrect approach because of the following scenario:

TextBuffer:

     axxxx
   😀xxx😀
     xxxxx
   😀xxx😀
     xxxxb

Selecting from a to b in box selection should result in all of the x being selected, but the selection expanding to encompass the wide glyphs. The implementation in this PR does not account for the wide glyphs.

Proposed Implementation

Create a SelectionTextRange class that implements the UiaTextRange class.
Make this heavily rely on GetSelectionRects to properly represent the selection.
GetSelection from ScreenInfoUiaProviderBase should just create this type of text range and abstract all of the difficult bits away. The SelectionTextRange constructor would just take the output from GetSelectionRects.

@DHowett-MSFT DHowett-MSFT added Issue-Task It's a feature request, but it doesn't really need a major design. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Issue-Bug It either shouldn't be doing this or needs an investigation. labels Feb 10, 2020
@DHowett-MSFT
Copy link
Contributor

This has always been an issue and is task-level work; recategorized as a task.

@carlos-zamora carlos-zamora mentioned this issue Feb 11, 2020
Closed
22 tasks
This was referenced Feb 13, 2020
Merged
Merged
DHowett-MSFT pushed a commit that referenced this issue Feb 28, 2020
@carlos-zamora
Move rect expansion to textbuffer; refactor selection code (#4560)
0e672fa 
- When performing chunk selection, the expansion now occurs at the time
  of the selection, not the rendering of the selection
- `GetSelectionRects()` was moved to the `TextBuffer` and is now shared
  between ConHost and Windows Terminal
- Some of the selection variables were renamed for clarity
- Selection COORDs are now in the Text Buffer coordinate space
- Fixes an issue with Shift+Click after performing a Multi-Click
  Selection

## References
This also contributes to...
- #4509: UIA Box Selection
- #2447: UIA Signaling for Selection
- #1354: UIA support for Wide Glyphs

Now that the expansion occurs at before render-time, the selection
anchors are an accurate representation of what is selected. We just need
to move `GetText` to the `TextBuffer`. Then we can have those three
issues just rely on code from the text buffer. This also means ConHost
gets some of this stuff for free 😀

### TextBuffer
- `GetTextRects` is the abstracted form of `GetSelectionRects`
- `_ExpandTextRow` is still needed to handle wide glyphs properly

### Terminal
- Rename...
    - `_boxSelection` --> `_blockSelection` for consistency with ConHost
    - `_selectionAnchor` --> `_selectionStart` for consistency with UIA
    - `_endSelectionPosition` --> `_selectionEnd` for consistency with
      UIA
- Selection anchors are in Text Buffer coordinates now
- Really rely on `SetSelectionEnd` to accomplish appropriate chunk
  selection and shift+click actions

## Validation Steps Performed
- Shift+Click
- Multi-Click --> Shift+Click
- Chunk Selection at...
    - top of buffer
    - bottom of buffer
    - random region in scrollback

Closes #4465
Closes #4547
carlos-zamora added a commit that referenced this issue Mar 9, 2020
@carlos-zamora
Abstract GetTextForClipboard() for UIA (#4578)
e79a421 
## Summary of the Pull Request
`GetTextForClipboard` already exists in the TextBuffer. It makes sense to use that for UIA as well. This changes the behavior or `GetText()` such that it does not remove leading/trailing whitespace anymore. That is more of an expected behavior.

## References
This also contributes to...
- #4509: UIA Box Selection
- #2447: UIA Signaling for Selection
- #1354: UIA support for Wide Glyphs
Now that the expansion occurs at before render-time, the selection anchors are an accurate representation of what is selected. We just need to move GetText to the TextBuffer. Then we can have those three issues just rely on code from the text buffer. This also means ConHost gets some of this stuff for free 😀

## Detailed Description of the Pull Request / Additional comments
- `TextBuffer::GetTextForClipboard()` --> `GetText()`
- `TextBuffer::GetText()` no longer requires GetForegroundColor/GetBackgroundColor. If either of these are not defined, we return a `TextAndColor` with only the `text` field populated.
- renamed a few parameters for copying text to the clipboard for clarity
- Updated `UiaTextRange::GetText()` to use `TextBuffer::GetText()`

## Validation Steps Performed
Manual tests for UIA using accessibility insights and Windows Terminal's copy action (w/ and w/out shift)

Added tests as well.
@carlos-zamora carlos-zamora mentioned this issue Mar 18, 2020
Merged
1 task
@ghost ghost added the In-PR This issue has a related PR label Mar 18, 2020
@ghost ghost closed this as completed in #4991 Mar 18, 2020
@ghost ghost removed the In-PR This issue has a related PR label Mar 18, 2020
ghost pushed a commit that referenced this issue Mar 18, 2020
@carlos-zamora
Properly represent block selections in UIA (#4991)
8627932 
## Summary of the Pull Request
Block selections were always read and displayed as line selections in UIA. This fixes that.

## PR Checklist
* [x] Closes #4509 

## Detailed Description of the Pull Request / Additional comments
1. Expose `IsBlockSelection()` via IUiaData
2. Update the constructor to be able to take in a block selection parameter
3. Make ScreenInfoUiaProviders pass step 1 output into step 2 constructor
4. Update all instances of `UiaTextRange::GetTextRects()` to include this new flag

## Validation Steps Performed
Manually tested.
Additional tests would be redundant as GetTextRects() is tested in the text buffer.
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Mar 18, 2020
DHowett-MSFT pushed a commit that referenced this issue Apr 14, 2020
@carlos-zamora @DHowett
Properly represent block selections in UIA (#4991)
ad80cff 
## Summary of the Pull Request
Block selections were always read and displayed as line selections in UIA. This fixes that.

## PR Checklist
* [x] Closes #4509 

## Detailed Description of the Pull Request / Additional comments
1. Expose `IsBlockSelection()` via IUiaData
2. Update the constructor to be able to take in a block selection parameter
3. Make ScreenInfoUiaProviders pass step 1 output into step 2 constructor
4. Update all instances of `UiaTextRange::GetTextRects()` to include this new flag

## Validation Steps Performed
Manually tested.
Additional tests would be redundant as GetTextRects() is tested in the text buffer.
@ghost
Copy link

ghost commented Apr 22, 2020

🎉This issue was addressed in #4991, which has now been successfully released as Windows Terminal Preview v0.11.1121.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Accessibility Issues related to accessibility Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.0
Development

Successfully merging a pull request may close this issue.

Properly represent block selections in UIA microsoft/terminal
3 participants
@carlos-zamora @DHowett-MSFT @cinnamon-msft