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.

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

Add modifier to filter keyboard events. #442

Merged
merged 32 commits into from Nov 21, 2022

Conversation

NakajimaTakuya
Copy link
Contributor

@NakajimaTakuya NakajimaTakuya commented Sep 6, 2021

from #440

There are many cases where we want to support keyboard operations as part of accessibility.

In such cases, determining which key is pressed each time and filtering it in the function may promote a divergence between the function name and the behavior when adhering to the teaching that declarative function names should be given.

prevTab(evt) {
  if (evt.key ! == 'ArrowLeft') { return; } // here
  ...
}

So, I thought it might be possible to express this filtering on the HTML side to make the function implementation more straightforward.

<button type="button" role="tab" data-action="keydown.left->tabs#prevTab">xxx</button>
//  syntax: `data-action="{event}.{filter_modifier}->{controller}#{action}"`

I know that there is a third party library that does this filter on the HTML side, using a different attribute value than data-action.
However, I'm hoping that this can be integrated into data-action as a Stimulus feature to make this simpler to achieve.

const keyMappings: { [key: string]: string } = {
  "enter":  "Enter",
  "tab":    "Tab",
  "esc":    "Escape",
  "space":  " ",
  "up":     "ArrowUp",
  "down":   "ArrowDown",
  "left":   "ArrowLeft",
  "right":  "ArrowRight",
  "home":   "Home",
  "end":    "End"
}

Here is a mapping of the keys supported by the filter modifier.
(I selected the keys that are often used mainly for accessibility support.)

@dhh
Copy link
Member

dhh commented Sep 11, 2021

This is very cool, and much simpler than I had imagined. Please do proceed. I'd like to see that in Stimulus ❤️

@NakajimaTakuya
Copy link
Contributor Author

@dhh
I'm really pleased to see the positive response to this proposal. 😍

Let me know what you think.
The modifiers I'm proposing are limited to a few keys.
Do you think it would be better to convert event.key with specific rules (ex.kebab-case) to widen the range of support?
(I assume that special keys such as the spacebar are supported separately.)

Takuya Nakajima [LIFULL] added 4 commits September 13, 2021 19:29
The modifier was in the wrong position.
It should be before the target.

* before: evt@global.modifier->...
* after:  evt.modifier@global->...
before:`${event}@${target}.${filter}->${identifier}#${methodName}`
after: `${event}.${filter}@${target}->${identifier}#${methodName}`
@dhh
Copy link
Member

dhh commented Sep 23, 2021

Haven't forgotten about this. Just want to get 3.0 out the door first, then revisit this for 3.1. Still thinking and weighing the very nice implementation and syntax of this PR against the limited scope.

@NakajimaTakuya
Copy link
Contributor Author

@dhh
Thank you for telling me your plans.

Don't worry, I'm in no hurry.
I hope this feature will be included, but just as much I hope that Stimulus will continue to be the most fun for you.
Take your time and think about it. I'll always be here.😄

@@ -2,6 +2,19 @@ import { ActionDescriptor, parseActionDescriptorString, stringifyEventTarget } f
import { Token } from "../mutation-observers"
import { camelize } from "./string_helpers"

const keyMappings: { [key: string]: string } = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this possibility be put on the Stimulus application? Maybe this way the mapping can be modified on a per application basis this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the comment was not clear.

The use case would be running Stimulus but being able to pass in a custom (or extended) keyMappings object when initialising the application (maybe as part of the schema) https://github.com/hotwired/stimulus/blob/main/src/core/schema.ts

// src/application.js
import { Application, defaultSchema } from "@hotwired/stimulus"

const customSchema = {
  ...defaultSchema,
  keyMappings: {...defaultSchema.keyMappings, a: "a", w: "w", s: "s", d: "d" },
}

window.Stimulus = Application.start(document.documentElement, customSchema);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for explaining it all so well! ❤️
I agree with your suggestion.
Give me a minute. :)

@NakajimaTakuya
Copy link
Contributor Author

@lb-
Sorry to keep you waiting.
I've tried to accommodate you, but what do you think?

@radiantshaw
Copy link
Contributor

radiantshaw commented Apr 2, 2022

@NakajimaTakuya Do you think it would be a good idea to also make the modifier accept an integer value? Like keyup.13->controller#action. This would allow the use of any key on the keyboard and it won't be limited to the mapping.

Edit: Okay I now see that custom mapping can be passed to the Application object to support more keys. But it would mean that the keys are not necessarily standardized across different applications. One of the main motives of having an integer as a modifier would be to standardize the filter.

@NakajimaTakuya
Copy link
Contributor Author

@radiantshaw I'm assuming that taking an integer value means using KeyboardEvent.keyCode?
(Sorry if this doesn't mean keyCode.)

I think that's already deprecated by the spec.
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode

Also, with or without it, I'm not sure I'd be motivated to adopt it too aggressively since it depends on the physical keys on the keyboard.

@radiantshaw
Copy link
Contributor

@NakajimaTakuya Yeah you're right. Sorry I didn't know about the deprecations. Seems like we're good.

But on a separate note, I was thinking if it's possible to also have syntax for shiftKey, metaKey, and ctrlKey. Something like keyup.ctrl+a->controller#action (the syntax is just for illustration purpose), that will check if ctrlKey was set to true when a was pressed. Do you think it's a good idea to implement this? If yes, then can it be done in the same PR or in a different one?

@NakajimaTakuya
Copy link
Contributor Author

@radiantshaw
I think that's a good thing.
At first I was making this PR a minimum requirement with the desire to keep it simple and bug free.
When I saw your issue (#530), I changed my mind that this modifier should be chainable with a meta key.
And I was waiting for an opportunity.
I'll try to work on that with this PR.

@NakajimaTakuya
Copy link
Contributor Author

I found one issue as soon as I started my research.
It is the following case.

<td role="gridcell"
  tabindex="0"
  data-action=""
    keydown.home->grid#focusFirstCellInRow
    keydown.ctrl.home->grid#focusFirstCellInGrid
  "
  ...
>

If ctrl+home is pressed (assuming this element has focus), there are several possible behaviors.

  • Both the focusFirstCellInRow and focusFirstCellInGrid functions work.
  • Only the focusFirstCellInGrid function works.

Some other libraries seem to default to the former behavior, and then add a .extract modifier to make it behave in the latter way.

vue's document: .exact Modifier

That seems somewhat complicated to me.
Therefore, I have a hunch that either this metakey support will not be implemented, or it will default to the latter behavior.

I would like to hear what the owners think.
@dhh what do you think?

@lb-
Copy link
Contributor

lb- commented May 12, 2022

Reading through this, I think it looks pretty powerful from my perspective and gives some room to add more functionality later. Thanks for taking on some ideas and running with it @NakajimaTakuya

@NakajimaTakuya
Copy link
Contributor Author

@lb- Thanks for the review :)

@NakajimaTakuya
Copy link
Contributor Author

NakajimaTakuya commented May 24, 2022

#442 (comment)

On second thought, the latter proposal breaks backwards compatibility.

Only the focusFirstCellInGrid function works.

If the existing code had the following implementation, the code that wanted the ctrl + home key to fire as well would no longer work.

<td role="gridcell"
  tabindex="0"
  data-action="
    keydown.home->grid#focusFirstCellInGrid
    ...
  "
  ...
>
focusFirstCellInGrid(evt) {
  if (!evt.ctrlKey) {
    return;
  }
  ....
}

The only way out seems to be to support the .exact modifier by process of elimination, but what do you guys think about this?
Personally, I find it less intuitive.

NakajimaTakuya and others added 5 commits November 19, 2022 12:21
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
Comment on lines +85 to +98
Filter | Key Name
-------- | --------
enter | Enter
tab | Tab
esc | Escape
space | " "
up | ArrowUp
down | ArrowDown
left | ArrowLeft
right | ArrowRight
home | Home
end | End
[a-z] | [a-z]
[0-9] | [0-9]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A suggestion for refinement - using backticks on the code bits.

| Filter  | Key Name   |
| ------- | ---------- |
| `enter` | Enter      |
| `tab`   | Tab        |
| `esc`   | Escape     |
| `space` | " "        |
| `up`    | ArrowUp    |
| `down`  | ArrowDown  |
| `left`  | ArrowLeft  |
| `right` | ArrowRight |
| `home`  | Home       |
| `end`   | End        |
| [a-z]   | [a-z]      |
| [0-9]   | [0-9]      |

OR - if the side borders is not really what we do (by the looks of things).

Filter  | Key Name
------- | ----------
`enter` | Enter
`tab`   | Tab
`esc`   | Escape
`space` | " "
`up`    | ArrowUp
`down`  | ArrowDown
`left`  | ArrowLeft
`right` | ArrowRight
`home`  | Home
`end`   | End
[a-z]   | [a-z
[0-9]   | [0-9

<div data-action="keydown.shift+a->listbox#selectAll" role="option" tabindex="0">...</div>
```

The list of supported modifier keys is shown below.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, my bad - I was confused, I will suggest a table approach in a min if that is ok.

docs/reference/actions.md Outdated Show resolved Hide resolved
docs/reference/actions.md Outdated Show resolved Hide resolved
docs/reference/actions.md Outdated Show resolved Hide resolved
docs/reference/actions.md Outdated Show resolved Hide resolved
NakajimaTakuya and others added 5 commits November 19, 2022 15:42
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
docs/reference/actions.md Outdated Show resolved Hide resolved
NakajimaTakuya and others added 2 commits November 19, 2022 16:46
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
@songjiz
Copy link

songjiz commented Nov 29, 2022

@NakajimaTakuya

I found an issue. This PR break bootstrap events, like shown.bs.dropdown.

class ZIndexController extends Stimulus.Controller {

  static values = { step: { type: Number, default: 1 } }

  increment(e) {
    const by = e.params.by ? Number(e.params.by) : this.stepValue
    this.value += by
  }

  decrement(e) {
    const by = e.params.by ? Number(e.params.by) : this.stepValue
    this.value -= by
  }

  set value(val) {
    this.element.style.zIndex = Number(val)
  }

  get value() {
    return Number(getComputedStyle(this.element).zIndex)
  }

})
<div data-controller="z-index">
   <div class="dropdown">
       <button type="button" aria-expanded="false" data-bs-toggle="dropdown" data-action="shown.bs.dropdown->z-index#increment hidden.bs-dropdown->z-index#decrement">
       <div class="dropdown-menu">
           ...
       </div>
  </div>
</div>

shown will be parsed as methodName of the ActionDescriptor, and bs.dropdown will be parsed as keyFilter of the ActionDescriptor. then z-index#increment method will not be executed

@lb-
Copy link
Contributor

lb- commented Nov 29, 2022

@NakajimaTakuya may I suggest you raise a standalone issue for this.

@radiantshaw
Copy link
Contributor

@songjiz One solution I can think of is adding Stimulus-specific escape characters. For example, if you want the shown.bs.dropdown event to be taken literally, then you can use it as data-action="shown\.bs\.dropdown->z-index#increment". But my hunch is that this might get ugly quickly, so we can also reserve some characters that behave like escape mechanisms. For eg: data-action="[shown.bs.dropdown]->z-index#increment" where anything inside the [] would be taken literally.

@NakajimaTakuya
Copy link
Contributor Author

@songjiz
@lb-
@radiantshaw
Thanks, I have set up an issue on this.
#612

@seb-jean
Copy link
Contributor

@NakajimaTakuya I think PageUp and PageDown are keys that are also often used primarily for accessibility support

@NakajimaTakuya
Copy link
Contributor Author

@seb-jean

There are certainly a few widgets that have page up/page down assignments!
I guess "feed", "grid", "treegrid", etc. fall into this category.
I think keyMappings is extensible, so we can add it in userland.

https://stimulus.hotwired.dev/reference/actions#:~:text=If%20you%20need%20to%20support%20other%20keys

@seb-jean
Copy link
Contributor

@seb-jean

There are certainly a few widgets that have page up/page down assignments! I guess "feed", "grid", "treegrid", etc. fall into this category. I think keyMappings is extensible, so we can add it in userland.

https://stimulus.hotwired.dev/reference/actions#:~:text=If%20you%20need%20to%20support%20other%20keys

I created PR #677

@seb-jean
Copy link
Contributor

I think it would be interesting to add [a-z]. I know it already exists but you can only use one character at a time.
This would be used for https://www.w3.org/WAI/ARIA/apg/patterns/menu-button/examples/menu-button-actions-active-descendant/#kbd2_label and for https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-autocomplete-list/#kbd_label_listbox.

Current, it would be like this

<button data-action="keydown.a->menu#log keydown.b->menu#log keydown.c->menu#log keydown.d->menu#log ..."></button>

I wish I had something like this

<button data-action="keydown.[a-z]->menu#log"></button>

@seb-jean
Copy link
Contributor

@NakajimaTakuya I tried to implement the filter to include all letters from a to z without having to type 26 actions. Unfortunately, I don't see how to do it. Would you have an idea?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants