Highlighter: Fix hinting state for keyboard-based selection #1691
Conversation
Before this change, keyboard-based selection would happen a segments at a time, generating a marked segment and triggering focus at each step. Now we're using hinting state, just like the mouse, until shift is released, and only then adding the mark to the hinted segments. This unbreaks NRI writing annotation functionality, and allows us to have specific ScreenReader cues for hinting.
WCAG asks for only single-key actions to be undoable, but this is such low effort
src/Nri/Ui/Highlighter/V5.elm
Outdated
| @@ -570,6 +572,16 @@ performAction action ( model, cmds ) = | |||
| ( { model | selectionStartIndex = Nothing, selectionEndIndex = Nothing }, cmds ) | |||
|
|
|||
|
|
|||
| isFirstLastHinted : Maybe ( Int, Int ) -> Highlightable marker -> Bool | |||
| isFirstLastHinted hintingIndices { index } = | |||
There was a problem hiding this comment.
| isFirstLastHinted hintingIndices { index } = | |
| isFirstOrLastHinted hintingIndices { index } = |
This is the first thing I see in the file so when I read it, I have no context to know whether "and" or "or" makes sense. Nice to have it in the name.
src/Nri/Ui/Highlighter/V5.elm
Outdated
| , Key.shiftLeft (Keyboard <| SelectionApplyTool highlightable.index) | ||
| , Key.shift (Keyboard <| SelectionReset highlightable.index) | ||
| [ -- Key.shift doesn't work on keyUp, bc `shiftKey` is False on keyUp | ||
| { keyCode = 16 |
There was a problem hiding this comment.
What does keyCode 16 mean? Is it the shift key itself? Or is it the case that the shift key doesn't have its own code and is only represented by the shiftKey field?
My guess is that it is represented in both fields and that 16 is the shift key. It's a while since I dealt with keypresses. I have not looked inside the Key lib, I could probably work it out if I did! I assume the Key.shift you mention is a named value you could import instead of hardcoding 16 but it is a record with shiftKey=True? If that's the case, maybe it could be written as shift.keyCode instead of 16, or { shift | shiftKey = False } or something.
There was a problem hiding this comment.
Oh good call on doing shift.keyCode. { shift | shiftKey = False} would require a let assignment bc it's a function that takes a parameter. shift.keyCode is neater, then.
Yep 16 is the keycode for shift.
The shiftKey field indicates whether shift was held during the event, so you can disambiguate cases like this, where there's no unique keycode for a shifted right arrow, like there probably are for uppercase letters.
For the weird behavior that forces us to do this, I can guess w3c thought "when you release shift you're not holding shift amirite, so shiftKey gotta be false on keyup but true on keydown."
There was a problem hiding this comment.
can't shift.keyCode bc it needs a parameter, so i did a small compromise. pushed it!
Co-authored-by: Brian Carroll <[email protected]>
|
"Selecting comment 9" means you are in the midst of highlighting text to make a comment, of which this will be the 9th of the assignment (or sequentially, the 9th in order)? |
|
@bendansby yes! |
|
on the numbering, the number is up to the use-case. quizzes will use real names like "selecting evidence", writing is using weird numbers rn but we have an issue to fix it |
|
Technically speaking, one is not selecting a comment, but selecting text to make a comment. Selecting a comment would be a different action. But perhaps the intention is to abbreviate a complex phrase into something simple? What do you think? |
|
"Selecting text for [highlight-name]" is not terribly longer. Do you think it's clearer / less confusing? |
|
I do! If you agree, let's go with that. |
|
updated the video w/ the new msg, pushed addressing all review feedback so far =] |
🔧 Modifying a component
Context
TL;DR: Shift+arrow now only apply the highlight when shift is released. "Hinting" mode is used while shift is pressed.
The way Highlighter keyboard-based selection worked made it very difficult to implement annotation functionality for writing.
Successive focus messages after each shift+arrow would throw a wrench on our code, and immediate creation of highlights would force us create+expand+expand+expand highlights, which would generate backend messages on each cycle.
Making kb-based selection work like the mouse, using hinting first, then applying marks to hinted segments all at once, when shift is released, is an elegant solution that not only completely solves our problems at the writing annotations side, but also opens the door to add friendlier screenreader cues to hinting state.
Specific things I want feedback on: Is the hinting screenreader cue ok?
🖼️ What does this change look like?
highlighter-new-msg.mov
Component completion checklist
nriDescription