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

Selection Expansion should not be occurring during Rendering #4465

Closed
carlos-zamora opened this issue Feb 4, 2020 · 4 comments · Fixed by #4560
Closed

Selection Expansion should not be occurring during Rendering #4465

carlos-zamora opened this issue Feb 4, 2020 · 4 comments · Fixed by #4560
Assignees
@carlos-zamora
Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) 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

This is a major bug, not because of what it's doing now, but how it's affecting other things.

How our selection model operates

Whenever we set a selection anchor (i.e.: pointer movements, search, etc), we are recording the COORD of the pointer or space that we want to select. If we encounter a wide glyph (i.e.: 😐) or are doing chunk selection for words/lines, the selection anchors are wrong and we expand to encompass them during _GetSelectionRects().

Issues this is causing

  1. Accessibility: Signaling Model #2447 Signaling for selection: the selection anchors we're returning are not representative of what gets selected.
  2. Implement keybindings to quick edit selection #3758 MoveSelectionAnchor: moving the selection anchors don't actually represent what is selected. So expanding by character on a double click selection really shows this.

Both of these issues are blocked until this gets resolved.

Proposed Approach

  • On click,
    • decide if this was a single/double/triple click
    • this establishes an expansion mode
    • set start/end at pointer
    • edit start/end based on expansion mode + glyphs
  • On drag,
    • if moving to the "right" of start, set end. Otherwise, set start.
    • from current cursor position, expand appropriately and update target endpoint
@carlos-zamora carlos-zamora added 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 4, 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 4, 2020
@carlos-zamora carlos-zamora added the Priority-1 A description (P1) label Feb 4, 2020
@carlos-zamora carlos-zamora added this to the Terminal v0.9 milestone Feb 4, 2020
@carlos-zamora carlos-zamora mentioned this issue Feb 4, 2020
Merged
1 task
@DHowett-MSFT
Copy link
Contributor

Wait, this is already in PR? Whoa. Yanking triage tag.

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 7, 2020
@carlos-zamora
Copy link
Member Author

Wait, this is already in PR? Whoa. Yanking triage tag.

Nope. Not in PR. Just related to the referred issue.

@carlos-zamora carlos-zamora self-assigned this Feb 7, 2020
@DHowett-MSFT
Copy link
Contributor

Okay. Can you build an estimate on fixing this one?

@carlos-zamora
Copy link
Member Author

Okay. Can you build an estimate on fixing this one?

I'd say 3 days of actual work. I'm having trouble wrapping my head around how to actually implement it.

@carlos-zamora carlos-zamora mentioned this issue Feb 13, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label Feb 13, 2020
ghost pushed a commit that referenced this issue Feb 20, 2020
@carlos-zamora
Make ScreenInfoUiaProvider::GetSelection() Return One Selection (#4466)
d0c8221 
## Summary of the Pull Request
We used to return multiple text ranges to represent one selection. We only support one selection at a time, so we should only return one range.

Additionally, I moved all TriggerSelection() calls to the renderer from Terminal to TermControl for consistency. This ensures we only call it _once_ when we make a change to our selection state.

## References
#2447 - helps polish Signaling for Selection
#4465 - This is more apparent as the problem holding back Signaling for Selection

## PR Checklist
* [x] Closes #4452 

Tested using Accessibility Insights.
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
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) 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.

Move rect expansion to textbuffer; refactor selection code microsoft/terminal
2 participants
@carlos-zamora @DHowett-MSFT