Keyboard Selection Spec #2840

carlos-zamora · 2019-09-21T21:38:21Z

Summary of the Pull Request

This introduces a spec for keyboard selection. This enables the user to create and update a selection without the use of a mouse or stylus.

References

Contributes to #715

Validation Steps Performed

N/A

zadjii-msft

I have mostly thought experiment questions

doc/specs/Keyboard-Selection.md

mahmoudajawad · 2019-10-28T13:28:32Z

When can we expect this PR to be merged? This is almost the last thing stopping me from using Terminal over ConEmu.

zadjii-msft · 2019-11-12T14:51:54Z

@carlos-zamora With #3391 about to merge, I'd want to see those changes reflected in this PR if possible :)

doc/specs/Keyboard-Selection.md

zadjii-msft

A couple new questions. I want to see the things about tmux-style mark mode, and bindings for the conhost-style mark mode, but overall looks good.

doc/specs/Keyboard-Selection.md

zadjii-msft

Consider this approving the non mark-mode parts of this spec. The holdSelectionAnchor comment confuses me, could you elaborate on that?

doc/specs/Keyboard-Selection.md

carlos-zamora · 2020-05-08T06:22:59Z

This is a bit annoying but I feel like Mark Mode is just big/confusing enough that it deserves its own space. So please check out #5804 if you want to have a closer discussion there.

That also means that the spec in its current state better fits #3758. So if you want to use the current implementation of this feature, you can build that branch.

Lingering question(s) that I have for anybody that comes along to review this:

Different/Cleaner name for the keybindings (and args)?

zadjii-msft

I'm definitely okay with everything in the spec here. I don't think we need to necessarily wait on #5804 to be approved to be able to merge this into master IMO. I think what's here is much more generally acceptable, and we'll have a LOT more discussion on the other thread, so let's not hold this one up ☺️

doc/specs/Keyboard-Selection.md

zadjii-msft · 2021-08-11T14:18:02Z

Heck let's just merge this. #10824 is like close enough to what's laid out here and I'm not sure we need to really discuss more.

nomenclature changed: - anchor --> endpoint - moveSelectionPoint --> updateSelection - expandBy --> mode - cell --> char - SelectionExpansionMode --> SelectionExpansion - Direction --> SelectionDirection

carlos-zamora · 2021-09-16T22:17:50Z

Discussed offline with @DHowett:

we should remove the customizable keybindings (unless requested later)
instead, handle this in ControlInteractivity. Benefit is that all consumers get it for free and they don't appear in the command palette. This is just how the terminal works.
If we get feedback during Preview that this isn't desired, we can reevaluate. I'd rather we don't have the option now, and add it in later, than have customizability now and remove it.

ofek · 2021-09-16T23:43:12Z

we should remove the customizable keybindings

Which ones? Would the defaults eventually still satisfy this use case? #6528

I'm just trying to figure when I can finally switch!

carlos-zamora · 2021-09-17T00:29:59Z

we should remove the customizable keybindings

Which ones? Would the defaults eventually still satisfy this use case? #6528

I'm just trying to figure when I can finally switch!

@ofek The idea is to remove the updateSelection action and just have these work "naturally".

terminal/doc/specs/Keyboard-Selection.md

Lines 91 to 109 in 9a5c2d2

    
           // Character Selection 
        
           { "command": {"action": "updateSelection", "direction": "left",  "mode": "char" }, "keys": "shift+left" }, 
        
           { "command": {"action": "updateSelection", "direction": "right", "mode": "char" }, "keys": "shift+right" }, 
        
           { "command": {"action": "updateSelection", "direction": "up",    "mode": "char" }, "keys": "shift+up" }, 
        
           { "command": {"action": "updateSelection", "direction": "down",  "mode": "char" }, "keys": "shift+down" }, 
        
           // Word Selection 
        
           { "command": {"action": "updateSelection", "direction": "left",  "mode": "word" }, "keys": "ctrl+shift+left" }, 
        
           { "command": {"action": "updateSelection", "direction": "right", "mode": "word" }, "keys": "ctrl+shift+right" }, 
        
           // Viewport Selection 
        
           { "command": {"action": "updateSelection", "direction": "left",  "mode": "view" }, "keys": "shift+home" }, 
        
           { "command": {"action": "updateSelection", "direction": "right", "mode": "view" }, "keys": "shift+end" }, 
        
           { "command": {"action": "updateSelection", "direction": "up",    "mode": "view" }, "keys": "shift+pgup" }, 
        
           { "command": {"action": "updateSelection", "direction": "down",  "mode": "view" }, "keys": "shift+pgdn" }, 
        
           // Buffer Corner Selection 
        
           { "command": {"action": "updateSelection", "direction": "up",    "mode": "buffer" }, "keys": "ctrl+shift+home" }, 
        
           { "command": {"action": "updateSelection", "direction": "down",  "mode": "buffer" }, "keys": "ctrl+shift+end" },

Hoping to fix up the spec and that other PR this week! 🙂

zadjii-msft · 2021-09-22T15:32:57Z

@DHowett We've discussed this like 80 times in team syncs since the first block - I'm 🤏 close to dismissing you since we've got a verbal approve

carlos-zamora · 2021-09-23T17:57:09Z

Alright, I'm dismissing @DHowett's review. We've talked about it in person a few times this past week, and the implementation includes those changes. We're having an internal bug bash soon and we still have some time before release, so we'll use that time to polish this feature if anything comes up.

Discussed in person. We'll iterate on this if more changes are needed.

DHowett

Great spec! Just a couple concerns.

I feel like we are missing some discussion about swapping the active selection endpoint, though I may have missed it(?); I would hope that it would cover what happens when the two endpoints are more than one screen apart (so, swapping would require jumping up and down).

Call out a future concern around how keyboard selection may be slower than mouse selection and we will need to consider how to handle a text buffer that is changing while the user is selecting it. What if the top endpoint circles out of the buffer before the user locks the selection?

How does it interact with VT mouse mode (right now it is an impediment to starting a selection(!)) which also uses shift as a modifier?

It may not be sufficient to say “keyboard selection is more accessible”; there are probably accessibility APIs we can plug into so folks don’t have to discover that they can use shift+arrows to move around.

I would suggest having key events (vkey, scancode, modifiers) go directly into ControlInteractivity and not have TermControl use _core.TryXxx (Sorry, you may have already done this). A clearer delineation in Interactivity about what we are capturing to handle “as part of a terminal control” versus what we are giving the core to process as a terminal-encoded output event would be helpful.

We never expect crashes… but you may want to mention the test collateral you’re going to write for this!

Sorry to block you again! We are in the home stretch, however.

Implements the following keyboard selection non-configurable key bindings: - shift+arrow --> move endpoint by character - ctrl+shift+left/right --> move endpoint by word - shift+home/end --> move to beginning/end of line - ctrl+shift+home/end --> move to beginning/end of buffer This was purposefully done in the ControlCore layer to make keyboard selection an innate part of how the terminal functions (aka a shared component across terminal consumers). ## References #715 - Keyboard Selection #2840 - Spec ## Detailed Description of the Pull Request / Additional comment The most relevant section is `TerminalSelection.cpp`, where we define how each movement operates. It's basically a giant embedded switch-case statement. We leverage a lot of the work done in a11y to perform the movements. ## Validation Steps Performed - General cases: - test all of the key bindings added - Corner cases: - `char`: wide glyph support - `word`: move towards, away, and across the selection pivot - automatically scroll viewport - ESC (and other key combos) are still clearing the selection properly

carlos-zamora · 2021-09-23T21:53:05Z

I feel like we are missing some discussion about swapping the active selection endpoint, though I may have missed it(?); I would hope that it would cover what happens when the two endpoints are more than one screen apart (so, swapping would require jumping up and down).

I'll cover that in #5804. It'll fit better over there as a wholistic story of "how do we create a selection", whereas this one is more just "how do we update one we created with the mouse". It could honestly go in either but this spec is intended to cover the basics, and that's more of an advanced concept imo.

EDIT: oh, it's already there haha

Call out a future concern around how keyboard selection may be slower than mouse selection

I don't actually think it'll be slower than mouse selection (or even slower enough to matter tbh). Mouse selection already relies on the same function calls on the text buffer:

move by char (wide glyphs) --> both need to check if we're on a wide glyph and update appropriately
move by word --> same text buffer calls (especially if you consider double-click + drag)
move by line/viewport --> just add/subtract the width of the viewport or clamp to the sides; should be a O(1)
move by buffer --> O(1); buffer's origin and mutable bottom are really quick-n-easy to get

The only difference I can think for mouse selection is that we convert the viewport position to a buffer position. Am I missing something here?

we will need to consider how to handle a text buffer that is changing while the user is selecting it. What if the top endpoint circles out of the buffer before the user locks the selection?

Isn't this something we already have to worry about (and handle) with mouse selection? I guess the only addition would be "if both endpoints moved off the buffer when the buffer circled, we need to ensure IsSelectionActive() is false"?

How does it interact with VT mouse mode (right now it is an impediment to starting a selection(!)) which also uses shift as a modifier?

I'm confused by your comments here. When in VT Mouse Mode, shift+click creates a selection. The bindings covered in this spec are used to alter the selection that was created. We're handling these key bindings exactly like we handle ESC when a selection is present (in any context): we alter the selection if one is present, otherwise we send the event to the app.

It may not be sufficient to say “keyboard selection is more accessible”; there are probably accessibility APIs we can plug into so folks don’t have to discover that they can use shift+arrows to move around.

The UIA APIs are already implemented, so yes, it's up to the screen reader to expose that. That's out of our control. However, having a more direct in-house solution is always preferable. We're bridging the gap between different experiences based on the user, and creating a consistent experience for all of our users.

As for discoverability, shift+arrows are already standard in Conhost. Windows users are pretty familiar with that experience. I guess this might be an issue for users who come from a different environment, but even text editors are consistent in having shift+arrows be how you select text.

I would suggest having key events (vkey, scancode, modifiers) go directly into ControlInteractivity and not have TermControl use _core.TryXxx (Sorry, you may have already done this). A clearer delineation in Interactivity about what we are capturing to handle “as part of a terminal control” versus what we are giving the core to process as a terminal-encoded output event would be helpful.

_core.TrySendKeyEvent() already exists. I think we talked a bit about moving that over to ControlInteractivity altogether, because key event handling makes sense to be near mouse event handling. That's an implementation detail imo though. Like, it's not really an issue until we complete the interactivity layer work right?

We never expect crashes… but you may want to mention the test collateral you’re going to write for this!

Isn't that out of scope for the spec? Like, can't you say that about just about any feature that gets specced?

carlos-zamora · 2021-09-23T23:04:02Z

@DHowett FYI, updated #5804 with some of the things I missed from your review. I think you'll find that to be a more interesting read. 😉

elouanKeryell-Even · 2021-09-24T08:24:19Z

🎉🎉🎉

## Summary of the Pull Request This introduces a selection marker overlay that tells the user which endpoint is currently being moved by the keyboard. The selection markers are respect font size changes and `cursor` color. ## References #715 - Keyboard Selection #2840 - Keyboard Selection Spec #5804 - Mark Mode Spec ## Detailed Description of the Pull Request / Additional comments - `TermControl` layer: - Use a canvas (similar to the one used for hyperlinks) to be able to draw the selection markers. - If we are notified that the selection changed, update the selection markers appropriately. - `UpdateSelectionMarkersEventArgs` lets us distinguish between mouse and keyboard selections. `ClearMarkers` is set to true in the following cases... 1. Mouse selection, via SetEndSelectionPoint 2. `LeftClickOnTerminal`, pretty self-explanatory 3. a selection created from searching for text - `ControlCore` layer: - Responsible for notifying `TermControl` to update the selection markers when a selection has changed. - Transfers info (the selection endpoint positions and which endpoint we're moving) from the terminal core to the term control. - `TerminalCore` layer: - Provides the viewport position of the selection endpoints. ## Validation Steps Performed - mouse selection (w/ and w/out shift) --> no markers - keyboard selection --> markers - markers update appropriately when we pivot the selection - markers scroll when you hit a boundary - markers take the color of the cursor color setting - markers are resized when the font size changes

## Summary of the Pull Request This is a spec specifically dedicated to Mark Mode. It's an addition to the Keyboard Selection spec. I felt that it makes the most sense to make this a separate PR because there's a lot of ideas that are very specific to Mark Mode, and this gives us the space to modify some of that behavior and get a good look at how other terminal emulators designed this feature. ## References #2840 - Keyboard Selection Spec (base spec/branch/PR) ## PR Checklist * [X] Contributes to #715

carlos-zamora self-assigned this Sep 21, 2019

zadjii-msft requested changes Sep 24, 2019

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 Sep 24, 2019

fpqc mentioned this pull request Sep 28, 2019

Support xterm VT sequence (OSC 52) for setting the clipboard #2946

Closed

zadjii-msft mentioned this pull request Oct 2, 2019

SHIFT-Curso, CTRL-C and CTRL-V not working but where in old console #3030

Closed

carlos-zamora mentioned this pull request Oct 9, 2019

Insert Mode? Mark mode #3135

Closed

carlos-zamora mentioned this pull request Nov 22, 2019

Mark Mode: Toggle instead of holding shift #3663

Closed

miniksa reviewed Nov 22, 2019

View reviewed changes

zadjii-msft reviewed Nov 25, 2019

View reviewed changes

doc/specs/Keyboard-Selection.md Outdated Show resolved Hide resolved

doc/specs/Keyboard-Selection.md Outdated Show resolved Hide resolved

doc/specs/Keyboard-Selection.md Outdated Show resolved Hide resolved

doc/specs/Keyboard-Selection.md Show resolved Hide resolved

carlos-zamora mentioned this pull request Nov 28, 2019

Implement keybindings to quick edit selection #3758

Closed

5 tasks

zadjii-msft reviewed Jan 3, 2020

View reviewed changes

doc/specs/Keyboard-Selection.md Show resolved Hide resolved

doc/specs/Keyboard-Selection.md Outdated Show resolved Hide resolved

doc/specs/Keyboard-Selection.md Outdated Show resolved Hide resolved

zadjii-msft requested review from DHowett-MSFT and leonMSFT January 3, 2020 21:10

zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal. labels Jan 3, 2020

leonMSFT reviewed Jan 6, 2020

View reviewed changes

doc/specs/Keyboard-Selection.md Outdated Show resolved Hide resolved

doc/specs/Keyboard-Selection.md Outdated Show resolved Hide resolved

carlos-zamora mentioned this pull request Jan 16, 2020

Missing cmd shortcuts in Terminal #4253

Closed

zadjii-msft mentioned this pull request Feb 3, 2020

Allow end-users to select and copy shell output without the mouse #4441

Closed

zadjii-msft mentioned this pull request Mar 9, 2020

Terminal doesn't copy on Enter (like conhost does) #4839

Closed

zadjii-msft mentioned this pull request Mar 18, 2020

Moving the cursor when scrolling in the history to select text #4985

Closed

carlos-zamora mentioned this pull request Mar 18, 2020

Scenario: Keyboard Text Selection #4993

Open

11 tasks

carlos-zamora mentioned this pull request May 8, 2020

Introduce Mark Mode spec (add-on) #5804

Merged

1 task

zadjii-msft approved these changes May 8, 2020

View reviewed changes

doc/specs/Keyboard-Selection.md Outdated Show resolved Hide resolved

doc/specs/Keyboard-Selection.md Outdated Show resolved Hide resolved

leonMSFT approved these changes Jun 10, 2020

View reviewed changes

DHowett mentioned this pull request Jun 16, 2020

[accessibility] Add support for a text selection mode #6528

Closed

carlos-zamora mentioned this pull request Aug 3, 2021

Add selection marker overlays for keyboard selection #10865

Merged

carlos-zamora added 2 commits September 14, 2021 09:52

Merge branch 'main' into dev/cazamor/spec-keyboard-selection

a918d60

fix copyOnSelect behavior & change some nomenclature

9a5c2d2

nomenclature changed: - anchor --> endpoint - moveSelectionPoint --> updateSelection - expandBy --> mode - cell --> char - SelectionExpansionMode --> SelectionExpansion - Direction --> SelectionDirection

carlos-zamora assigned DHowett and unassigned carlos-zamora Sep 14, 2021

make key bindings non-configurable

8311eb6

This comment has been minimized.

Sign in to view

spell check

c2dac82

zadjii-msft requested review from DHowett and removed request for DHowett-MSFT September 22, 2021 15:33

carlos-zamora unassigned DHowett Sep 23, 2021

carlos-zamora merged commit e75f848 into main Sep 23, 2021

carlos-zamora deleted the dev/cazamor/spec-keyboard-selection branch September 23, 2021 17:58

DHowett requested changes Sep 23, 2021

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 Sep 23, 2021

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 23, 2021

zadjii-msft mentioned this pull request Dec 2, 2021

Vi mode (READ: Allow selection keys to be rebound to different keys) #11868

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyboard Selection Spec #2840

Keyboard Selection Spec #2840

carlos-zamora commented Sep 21, 2019 •

edited

Loading

zadjii-msft left a comment

mahmoudajawad commented Oct 28, 2019

zadjii-msft commented Nov 12, 2019

zadjii-msft left a comment

zadjii-msft left a comment

carlos-zamora commented May 8, 2020

zadjii-msft left a comment

zadjii-msft commented Aug 11, 2021

carlos-zamora commented Sep 16, 2021

ofek commented Sep 16, 2021

carlos-zamora commented Sep 17, 2021

This comment has been minimized.

zadjii-msft commented Sep 22, 2021

carlos-zamora commented Sep 23, 2021

DHowett left a comment

carlos-zamora commented Sep 23, 2021 •

edited

Loading

carlos-zamora commented Sep 23, 2021

elouanKeryell-Even commented Sep 24, 2021

Keyboard Selection Spec #2840

Keyboard Selection Spec #2840

Conversation

carlos-zamora commented Sep 21, 2019 • edited Loading

Summary of the Pull Request

References

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

mahmoudajawad commented Oct 28, 2019

zadjii-msft commented Nov 12, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora commented May 8, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft commented Aug 11, 2021

carlos-zamora commented Sep 16, 2021

ofek commented Sep 16, 2021

carlos-zamora commented Sep 17, 2021

This comment has been minimized.

zadjii-msft commented Sep 22, 2021

carlos-zamora commented Sep 23, 2021

DHowett left a comment

Choose a reason for hiding this comment

carlos-zamora commented Sep 23, 2021 • edited Loading

carlos-zamora commented Sep 23, 2021

elouanKeryell-Even commented Sep 24, 2021

carlos-zamora commented Sep 21, 2019 •

edited

Loading

carlos-zamora commented Sep 23, 2021 •

edited

Loading