Add keyboard navigation to hyperlinks #13405

carlos-zamora · 2022-06-30T18:27:52Z

Summary of the Pull Request

Adds support to navigate to clickable hyperlinks using only the keyboard. When in mark mode, the user can press [shift+]tab to go the previous/next hyperlink in the text buffer. Once a hyperlink is selected, the user can press Ctrl+Enter to open the hyperlink.

References

#4993

PR Checklist

Closes Keyboard navigation of clickable link #6649
Documentation updated at Document tabbing to hyperlink MicrosoftDocs/terminal#558

Detailed Description of the Pull Request / Additional comments

Main change
- The OpenHyperlink event needs to be piped down to ControlCore now so that we can open a hyperlink at that layer.
- SelectHyperlink(dir) searches the buffer in a given direction and finds the first hyperlink, then selects it.
- "finding the hyperlink" is the tough part because the pattern tree takes in buffer coordinates, searches through the buffer in that given space, then stores everything relative to the defined space. Normally, the given buffer coordinates would align with the viewport's start and end. However, we're now trying to search outside of the viewport (sometimes), so we need to manage two coordinate systems at the same time.
- convertToSearchArea() lambda was used to convert a given coordinate into the search area coordinate system. So if the search area is the visible viewport, we spit out a viewport position. If the search area is the next viewport, we spit out a position relative to that.
- extractResultFromList() lambda takes the list of patterns from the pattern tree and spits out the hyperlink we want. If we're searching forwards, we get the next one. Otherwise, we get the previous one. We explicitly ignore the one we're already on. If we don't find any, we return nullopt.
- Now that we have all these cool tools, we use them to progressively search through the buffer to find the next/previous hyperlink. Start by searching the visible viewport after (or before) the current selection. If we can't find anything, go to the next "page" (viewport scrolled up/down). Repeat this process until something comes up.
- If we can't find anything, nothing happens. We don't wrap around.
Other relevant changes
- the copy action is no longer bound to Enter. Instead, we manually handle it in ControlCore.cpp. This also lets us handle Shift+Enter appropriately without having to take another key binding.
- _ScrollToPoint was added. It's a simple function that just scrolls the viewport such that the provided buffer position is in view. This was used to de-duplicate code.
- _ScrollToPoint was added into the ToggleMarkMode() function. Turns out, we don't scroll to the new selection when we enter mark mode (whoops!). We should do that and we should backport this part of the change too. I'll handle that.
- add some clarity when some functions are using the viewport position vs the buffer position. This is important because the pattern interval tree works in the viewport space.

Validation Steps Performed

case: all hyperlinks are in the view
- ✅ get next link
- ✅ get prev link
✅ case: need to scroll down for next link
✅ case: need to scroll up for next link

carlos-zamora · 2022-06-30T18:30:09Z

Why is this a draft?

[Bug] it doesn't work well when we've scrolled a bit
[Polish] still a few TODOs sprinkled throughout
I removed Enter as a default keybinding. Need to add it into the mark mode stuff as a hard-coded thing.
- Or (I like this idea more) ctrl+enter opens the keybinding. Enter just copies it.

Figured I'd publish this as a draft PR for now because there may be some major changes people may want to see at this point in the implementation, and I want to move on to work on other stuff for a bit.

zadjii · 2022-06-30T18:37:33Z

Ctrl+Enter, cause clicking on a link requires Ctrl 🤔?

src/cascadia/TerminalCore/TerminalSelection.cpp

src/cascadia/TerminalControl/ControlCore.cpp

src/cascadia/TerminalCore/TerminalSelection.cpp

src/cascadia/TerminalCore/Terminal.hpp

PankajBhojwani

Discovered a bug that causes a hang (think its a deadlock) - repro steps:

Select a link via keyboard
Hit 'Enter'
Hang

carlos-zamora · 2022-07-12T00:39:09Z

Discovered a bug that causes a hang (think its a deadlock) - repro steps:

Select a link via keyboard

Hit 'Enter'

Hang

Fixed!

src/cascadia/TerminalCore/Terminal.hpp

This reverts commit 12867b5, reversing changes made to 3f21996. # Conflicts: # src/cascadia/TerminalCore/TerminalSelection.cpp

carlos-zamora · 2022-07-12T19:41:09Z

@msftbot merge this in 10 minutes

ghost · 2022-07-12T19:41:14Z

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Tue, 12 Jul 2022 19:51:13 GMT, which is in 10 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

carlos-zamora · 2022-07-12T19:52:27Z

Good job bot -.- ugh

DHowett

3/8

src/cascadia/TerminalControl/ControlCore.cpp

DHowett · 2022-07-12T19:54:12Z

src/cascadia/TerminalControl/ControlCore.cpp

-                _terminal->SelectAll();
-                _updateSelectionUI();
-                return true;
+                if (modifiers.IsCtrlPressed() && vkey == 'A')


FWIW: I feel like this is below a "dotted line" boundary where ControlInteractivity and ControlCore meet. This is all interactive stuff, and all specifies the interaction model for the control. @zadjii-msft may be more (or less?) opinionated about this, but like... it feels like we're blending the layers in a way that is not mutually beneficial to all the components.

We should discuss!

oof, just ran into this:

terminal/src/cascadia/TerminalControl/ControlCore.cpp

Lines 1606 to 1616 in 574a72b

// This one's really pushing the boundary of what counts as "encapsulation".

// It really belongs in the "Interactivity" layer, which doesn't yet exist.

// There's so many accesses to the selection in the Core though, that I just

// put this here. The Control shouldn't be futzing that much with the

// selection itself.

void ControlCore::LeftClickOnTerminal(const til::point terminalPosition,

const int numberOfClicks,

const bool altEnabled,

const bool shiftEnabled,

const bool isOnOriginalPosition,

bool& selectionNeedsToBeCopied)

I think this may just be another example of the dotted lines not working right. :/

Maybe this is a case of bad naming. In my head, both WPF and UWP controls would use Interactivity as the owner for the core. I'm not sure there's a world where they'd be separate.

Curiously, TrySendKeyEvent doesn't exist on Interactivity, it only exists on Core. That's kinda weird. Probably an artifact of how the control/core refactor happened - moving this to the right place wasn't needed so it was left in a weird place. In an ideal world:

yea TrySendKeyEvent should be exposed off interactivity, with this selection code in it

Core should expose to Interactivity stuff like SelectAll, SelectHyperlink, TryOpenSelectedHyperlink (ew), etc.

I think. But I wouldn't call this blocking.

src/cascadia/TerminalControl/ControlCore.cpp

DHowett · 2022-07-12T19:55:38Z

src/cascadia/TerminalControl/ControlCore.idl

@@ -153,6 +153,6 @@ namespace Microsoft.Terminal.Control
        event Windows.Foundation.TypedEventHandler<Object, FoundResultsArgs> FoundMatch;
        event Windows.Foundation.TypedEventHandler<Object, ShowWindowArgs> ShowWindowChanged;
        event Windows.Foundation.TypedEventHandler<Object, UpdateSelectionMarkersEventArgs> UpdateSelectionMarkers;
-
+        event Windows.Foundation.TypedEventHandler<Object, OpenHyperlinkEventArgs> OpenHyperlink;


FWIW: I had my feelings about ControlCore/Interactivity when I got to this line. I was mostly fine before I saw this, then it made me think twice. This is a user-interaction-originated event.

DHowett · 2022-07-12T19:59:04Z

_ScrollToPoint was added. It's a simple function that just scrolls the viewport such that the provided buffer position is in view. This was used to de-duplicate code.

Did you deduplicate existing code while you were here? Mark scrolling also needs to scroll to a point, and there is an active feature request that discusses where exactly to scroll. We should make sure these things use the same implementation. #13349

_ScrollToPoint was added into the ToggleMarkMode() function. Turns out, we don't scroll to the new selection when we enter mark mode (whoops!). We should do that and we should backport this part of the change too. I'll handle that.

What?

DHowett · 2022-07-12T20:00:34Z

src/cascadia/TerminalCore/Terminal.cpp

@@ -46,6 +46,7 @@ Terminal::Terminal() :
    _altGrAliasing{ true },
    _blockSelection{ false },
    _selectionMode{ SelectionInteractionMode::None },
+    _isTargetingUrl{ false },


Terminals don't necessarily "target" things; what does this mean?

This means that the selection is currently targeting a URL. (I'll rename it)

Side note, there's a lot of members that are specific to selection. It would be nice to pull all of these out into a class or struct like this...

struct SelectionState { // also add declarations for SelectionAnchors, ExpansionMode, InteractionMode, Endpoint std::optional<SelectionAnchors> _selection; bool _blockMode; std::wstring _wordDelimiters; ExpansionMode _multiClickMode; InteractionMode _interactionMode; bool _isTargetingUrl; Endpoint _selectionEndpoint; bool _anchorInactiveSelectionEndpoint; };

I kinda want to pull all of TerminalSelection.cpp into its own class too but idk what the right way to clean all of this up would be. The reason I bring this up is because I hate having to add the word "selection" to every var related to it (i.e. _isTargettingUrl --> _selectionIsTargetingUrl). Thoughts? Maybe this should be a task I file under CodeHealth?

src/cascadia/TerminalCore/Terminal.cpp

DHowett · 2022-07-12T20:03:32Z

src/cascadia/TerminalCore/Terminal.hpp

@@ -249,6 +250,12 @@ class Microsoft::Terminal::Core::Terminal final :
        Down
    };

+    enum class SearchDirection


Huh. Don't we already have an enum for this?

src\buffer\out\search.h 27- 28-class Search final 29-{ 30-public: 31: enum class Direction

Whether it is usable here is a different story...

Yeah, it's just not really worth it imo. I had a bool before but I think it's ok to have this enum be a bit redundant (at least for now).

DHowett · 2022-07-12T20:06:07Z

src/cascadia/TerminalSettingsModel/defaults.json

@@ -382,7 +382,6 @@
        // Clipboard Integration
        { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+shift+c" },
        { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+insert" },
-        { "command": { "action": "copy", "singleLine": false }, "keys": "enter" },


now users cannot unbind it, and it no longer works for mouse selection.........?

Correct. I mean, I see the argument for both sides, so I'm not sure what the right answer is.

Enter as a key binding

Pros...

Configurable (aka, unbindable)

works with mouse selection "out-of-the-box"

Other notes...

we should probably bind Shift+Enter to copy w/ singleLine = false then? For consistency with mouse-copy?

we should probably bind Ctrl+Enter to a new action like "open hyperlink"?

Enter as a mark mode binding

Pros...

these features are explicitly a part of "mark mode"

Other notes...

we're technically taking key chords away from the user, but a selection is present so key chords are designed to clear the selection anyways... so not really?

Enter as a "that's just how things work" binding

(aka we make it so that "Enter" and "Shift+Enter" copy when a selection is active... always... regardless of how the selection was made)

Pros...

consistency

Cons...

no configurability

Thoughts?

Also:

Option to not clear selection on copy #3884

Suggestion: Copy without dismissing selection #13438

frankly I never loved binding enter by default, I was just like, okay with it. The shift+enter point is a good one, and I similarly thought this would be a openHyperlink action.

~~Hmm. I think after reading~~

~~but a selection is present so key chords are designed to clear the selection anyways... so not really~~

~~I'm kinda cool with either of the second two.~~

Uhg idk.

What if I'm a user that wants enter to "copy, not dismiss"?

What if I want to bind Ctrl+Enter to openAutoSuggestMenu (as an example).

~~When would a user have ctrl+enter bound in their client application s.t. they would not want a selection to cause a hyperlink to be opened when pressed...~~ Okay this scenario doesn't seem to make sense.

I'm not sure I've convinced myself one way or the other

both-both-is-good.gif

DHowett · 2022-07-12T20:06:52Z

src/cascadia/TerminalCore/TerminalSelection.cpp

+            _scrollOffset -= amtBelowView;
+        }
+        _NotifyScrollEvent();
+        _activeBuffer().TriggerScroll();


Is it right to tell the text buffer that scrolling happened? Does this kick the renderer?

Yup.

_NotifyScrollEvent() --> tell TermControl to update the scroll bar

TriggerScroll() --> tell the renderer to re-render the contents

DHowett · 2022-07-12T20:10:32Z

src/cascadia/TerminalCore/TerminalSelection.cpp

+    _selection->start = result->first;
+    _selection->pivot = result->first;
+    _selection->end = result->second;
+    bufferSize.DecrementInBounds(_selection->end);


when you make selection exclusive, rename end so things like this immediately become build breaks. you don't have to keep the rename, just use it to highlight issues where we might be mistreating the endpoint!

DHowett · 2022-07-12T20:12:18Z

src/cascadia/TerminalCore/TerminalSelection.cpp

@@ -630,6 +770,7 @@ void Terminal::ClearSelection()
 {
    _selection = std::nullopt;
    _selectionMode = SelectionInteractionMode::None;
+    _isTargetingUrl = false;


who resets _isTargetingUrl when somebody (1) makes a mouse selection? or (2) extends an automatic URL search selection by shift-clicking?

It's only set in SelectHyperlink() (you tabbed to the hyperlink).

It's then reset when you...

clear the selection

enter/exit mark mode (I guess this one is unnecessary)

use keyboard selection to modify the selection

So if you make a mouse selection (or extend it via shift+click), we're no longer in mark mode so ctrl+enter doesn't work.

That said, if we choose a different path here, _isTargetingUrl's value does matter and should be reset there. Heck, in general, it's probably just good practice to update _isTargetingUrl then so I might as well do it.

carlos-zamora · 2022-07-12T21:17:14Z

_ScrollToPoint was added. It's a simple function that just scrolls the viewport such that the provided buffer position is in view. This was used to de-duplicate code.

Did you deduplicate existing code while you were here? Mark scrolling also needs to scroll to a point, and there is an active feature request that discusses where exactly to scroll. We should make sure these things use the same implementation. #13349

Just looked into it. ScrollToMark() uses Terminal::UserScrollViewport(). It's definitely possible to deduplicate the code (or better, just always use one or the other), but it turns out that they both behave pretty differently and appropriately for their respective scenario. Here's a gif showing the two side by side:

On the left, we're using UserScrollViewport() and on the right, we're using _ScrollToPoint(). The demo is that we're just entering mark mode and moving up/down by viewport. What I've noticed is that UserScrollViewport() keeps the point at the top of the view, whereas _ScrollToPoint() keeps the point at the top/bottom (where it was before). I think that's the behavior we want for each scenario tbh.

_ScrollToPoint was added into the ToggleMarkMode() function. Turns out, we don't scroll to the new selection when we enter mark mode (whoops!). We should do that and we should backport this part of the change too. I'll handle that.

What?

Do this:

generate output in the terminal
scroll to the top
enter mark mode
Bug: we should scroll to the cursor, but instead we just sit there like if nothing happened.

zadjii-msft · 2022-07-12T21:29:52Z

Ah so _ScrollToPoint behaves like the nearestEdge I had theorized on for scrollToMark

scrollToMark

Marks on scrollbar: smarter navigation (center the mark vertically) #13449 - center or some other setting that controls how the mark is scrolled in.

Maybe top (current) /center (as proposed) /nearestEdge (when scrolling down, put the mark at the bottom line of viewport , up -> top line)?

(quoted from #11000)

I ultimately just went with always scrolling to the top cause it was easier to implement off the bat, and because it seemed sensible to always plop the prompt at the start of the viewport (with it's output beneath it). There's been some discussion on this subject ☺️

Adds `ScrollToPoint()` from #13405 to be able to scroll to the selection marker when (1) mark mode is entered and (2) `selectAll` is called. This change is a combination of #13656 and a minor part of #13405. Epic: #4993

## Summary of the Pull Request Polishes #13405 with some additional feedback found in testing and post-mortem reviews. Such feedback includes: - [x] ControlInteractivity vs ControlCore split ([link](#13405 (comment))) - [x] clearing the selection should be under lock when copying text via "Enter" - [x] move mark mode keybindings into a helper function - [x] decide if "Enter" should be configurable or non-configurable ([link](#13405 (comment))) - [x] rename `_isTargetingUrl` - [x] bugfix: ctrl+enter when the link is outside of the viewport ## References Original PR: #13405 Relevant issue: #6649 Epic: #4993

ghost · 2022-09-13T17:06:05Z

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

Handy links:

carlos-zamora mentioned this pull request Jun 30, 2022

Scenario: Keyboard Text Selection #4993

Open

11 tasks

lhecker reviewed Jun 30, 2022

View reviewed changes

carlos-zamora force-pushed the dev/cazamor/kbd-sln/switchSelectionEndpoint branch 2 times, most recently from 2e48423 to 02d6a07 Compare June 30, 2022 23:55

DHowett force-pushed the dev/cazamor/kbd-sln/switchSelectionEndpoint branch from 02d6a07 to 5e49772 Compare July 1, 2022 01:10

ghost deleted the branch main July 1, 2022 01:44

ghost closed this Jul 1, 2022

Add keyboard navigation to hyperlinks

23f2f65

carlos-zamora reopened this Jul 1, 2022

carlos-zamora changed the base branch from dev/cazamor/kbd-sln/switchSelectionEndpoint to main July 1, 2022 17:08

carlos-zamora force-pushed the dev/cazamor/kbd-sln/tab-to-link branch from 6f4cc16 to 23f2f65 Compare July 1, 2022 17:09

carlos-zamora added 2 commits July 7, 2022 13:07

apply feedback; fix scroll bug; use ctrl+enter

d183cac

add _ScrollToPoint() for deduplication

0ba6f67

carlos-zamora marked this pull request as ready for review July 7, 2022 20:28

DHowett added a commit that referenced this pull request Jul 8, 2022

PRE-MERGE #13405 Add keyboard navigation to hyperlinks

b7005ee

zadjii-msft self-assigned this Jul 8, 2022

carlos-zamora added 2 commits July 11, 2022 11:26

address some more of Leonard's feedback

df2e0ca

bugfix: open hyperlink when in scrolled position

8fdf294

lhecker approved these changes Jul 11, 2022

View reviewed changes

carlos-zamora added Area-Accessibility Issues related to accessibility and removed Area-Input Related to input processing (key presses, mouse, etc.) Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Jul 11, 2022

PankajBhojwani approved these changes Jul 11, 2022

View reviewed changes

src/cascadia/TerminalCore/Terminal.hpp Outdated Show resolved Hide resolved

PankajBhojwani requested changes Jul 11, 2022

View reviewed changes

zadjii-msft assigned PankajBhojwani and unassigned zadjii-msft Jul 12, 2022

DHowett added a commit that referenced this pull request Jul 12, 2022

PRE-MERGE #13405 Add keyboard navigation to hyperlinks

12867b5

PankajBhojwani reviewed Jul 12, 2022

View reviewed changes

src/cascadia/TerminalCore/Terminal.hpp Outdated Show resolved Hide resolved

DHowett added a commit that referenced this pull request Jul 12, 2022

Revert "PRE-MERGE #13405 Add keyboard navigation to hyperlinks"

364db1f

This reverts commit 12867b5, reversing changes made to 3f21996. # Conflicts: # src/cascadia/TerminalCore/TerminalSelection.cpp

whoops

519e50e

PankajBhojwani approved these changes Jul 12, 2022

View reviewed changes

carlos-zamora unassigned PankajBhojwani Jul 12, 2022

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 12, 2022

DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 12, 2022

ghost merged commit 7abed9f into main Jul 12, 2022

ghost deleted the dev/cazamor/kbd-sln/tab-to-link branch July 12, 2022 19:51

DHowett requested changes Jul 12, 2022

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 Jul 12, 2022

DHowett reviewed Jul 12, 2022

View reviewed changes

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 12, 2022

carlos-zamora mentioned this pull request Jul 13, 2022

[Follow-up] Polish keyboard navigation to hyperlinks #13494

Merged

6 tasks

This was referenced Aug 2, 2022

Mark Mode should preempt key bindings, and work with anything that causes scrolling... #13533

Closed

[1.15] Scroll to sln marker for Mark Mode & SelectAll #13660

Merged

ghost mentioned this pull request Sep 13, 2022

Keyboard navigation of clickable link #6649

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add keyboard navigation to hyperlinks #13405

Add keyboard navigation to hyperlinks #13405

carlos-zamora commented Jun 30, 2022 •

edited

Loading

carlos-zamora commented Jun 30, 2022 •

edited

Loading

zadjii commented Jun 30, 2022

PankajBhojwani left a comment

carlos-zamora commented Jul 12, 2022

carlos-zamora commented Jul 12, 2022

ghost commented Jul 12, 2022

carlos-zamora commented Jul 12, 2022

DHowett left a comment

DHowett Jul 12, 2022

carlos-zamora Jul 13, 2022

zadjii-msft Jul 13, 2022

DHowett Jul 12, 2022

DHowett commented Jul 12, 2022

DHowett Jul 12, 2022

carlos-zamora Jul 12, 2022

DHowett Jul 12, 2022

DHowett Jul 12, 2022

carlos-zamora Jul 12, 2022

DHowett Jul 12, 2022

carlos-zamora Jul 12, 2022

zadjii-msft Jul 13, 2022

zadjii-msft Aug 15, 2022

DHowett Jul 12, 2022

carlos-zamora Jul 12, 2022

DHowett Jul 12, 2022

DHowett Jul 12, 2022

carlos-zamora Jul 12, 2022

carlos-zamora commented Jul 12, 2022

zadjii-msft commented Jul 12, 2022

ghost commented Sep 13, 2022

	// This one's really pushing the boundary of what counts as "encapsulation".
	// It really belongs in the "Interactivity" layer, which doesn't yet exist.
	// There's so many accesses to the selection in the Core though, that I just
	// put this here. The Control shouldn't be futzing that much with the
	// selection itself.
	void ControlCore::LeftClickOnTerminal(const til::point terminalPosition,
	const int numberOfClicks,
	const bool altEnabled,
	const bool shiftEnabled,
	const bool isOnOriginalPosition,
	bool& selectionNeedsToBeCopied)

Add keyboard navigation to hyperlinks #13405

Add keyboard navigation to hyperlinks #13405

Conversation

carlos-zamora commented Jun 30, 2022 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

carlos-zamora commented Jun 30, 2022 • edited Loading

Why is this a draft?

zadjii commented Jun 30, 2022

PankajBhojwani left a comment

Choose a reason for hiding this comment

carlos-zamora commented Jul 12, 2022

carlos-zamora commented Jul 12, 2022

ghost commented Jul 12, 2022

carlos-zamora commented Jul 12, 2022

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Jul 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enter as a key binding

Enter as a mark mode binding

Enter as a "that's just how things work" binding

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Jul 12, 2022

zadjii-msft commented Jul 12, 2022

ghost commented Sep 13, 2022

carlos-zamora commented Jun 30, 2022 •

edited

Loading

carlos-zamora commented Jun 30, 2022 •

edited

Loading