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

๐ŸŽ›๏ธ Replace MouseTrap usage with built in Stimulus keyboard support #10167

Closed
1 of 5 tasks
lb- opened this issue Feb 27, 2023 · 13 comments ยท Fixed by #11740
Closed
1 of 5 tasks

๐ŸŽ›๏ธ Replace MouseTrap usage with built in Stimulus keyboard support #10167

lb- opened this issue Feb 27, 2023 · 13 comments ยท Fixed by #11740
Labels
javascript Pull requests that update Javascript code type:Enhancement

Comments

@lb-
Copy link
Member

lb- commented Feb 27, 2023

โ„น๏ธ Part of the Stimulus ๐ŸŽ›๏ธ RFC 78

Is your proposal related to a problem?

At the moment we pull in the Mousetrap library for only two keyboard shortcuts. While this library is small (5KB), it is no longer actively maintained and Stimulus 3.2 has built in support for keyboard shortcuts that match most of the Mousetrap features.

Describe the solution you'd like (v1)

See details (older approach, not recommended now)

We should replace the two Mousetrap keyboard shortcut usages by extending the w-action / ActionController to support a click method and activate these on the relevant elements via Stimulus actions.

See https://stimulus.hotwired.dev/reference/actions#keyboardevent-filter

โœ… 1. Extend ActionController to support a click method.

See client/src/controllers/ActionController.ts

This controller supports a post method that will post a form submit when a button is clicked. It would be good to extend this to support another method click that simply calls .click() on the controlled element.

Note: It may make sense to make a dedicated controller for this, but it seems a bit overkill, plus we want to avoid a keyboard shortcut controller as basically any action can be triggered by a keyboard shortcut.

2. Add a mod keyboard key

3. Update existing usage (preview panel)

  • Update wagtail/admin/templates/wagtailadmin/shared/side_panel_toggles.html to add the controller.
  • Note: We could pull this up into the Python code, however there are already a lot of data attributes so it may make sense to keep adding to the HTML template.
{% load wagtailadmin_tags %} {% for panel in side_panels %}
<button
  type="button"
  class="{{ nav_icon_button_classes }} expanded:w-text-primary"
  aria-label="{{ panel.toggle_aria_label }}"
  aria-expanded="false"
  data-tippy-content="{{ panel.title }}"
  data-tippy-offset="[0,0]"
  data-tippy-placement="bottom"
  data-side-panel-toggle="{{ panel.name }}"
  data-controller="w-action"
  data-action="{% if panel.name == 'preview' %}keyup.mod+p@window->w-action#click{% endif %}"
>
  {% icon name=panel.toggle_icon_name classname=nav_icon_classes %}
</button>
{% endfor %}
  • Update existing non-Draftail simple keyboard shortcuts to Stimulus actions

4. Update existing usage (save button)

Note: If parts 1-3 are done as one PR, that will be a great way to get early feedback.

The existing save button mod+s works on any button with action-save, which is over 12 files.

For example - wagtail/admin/templates/wagtailadmin/pages/action_menu/page_locked.html

We will likely need to add some backwards compatibility here as this is a behaviour that is very likely used by packages and other developers building on Wagtail.

Firstly though, each of the action-save classes should be removed and instead replaced with the suitable data attributes.

Example:

{% load i18n wagtailadmin_tags %}
<button
  type="submit"
  class="button {% if is_revision %}warning{% endif %}"
  disabled
  data-controller="action"
  data-action="keyup.mod+s@window->w-action#click"
>
  {% if icon_name %}{% icon name=icon_name %}{% endif %} {% trans 'Page locked'
  %}
</button>

After that, we will likely need to add an afterLoad callback on ActionController to find any .action-save elements without the controller attribute and then add the data attributes.

You can see an example of this approach in client/src/controllers/ProgressController.ts.

  • Update existing non-Draftail complex keyboard shortcuts to Stimulus actions

5. Remove mousetrap

  • Mousetrap can now be removed from wagtail/admin/templates/wagtailadmin/pages/_editor_js.html
  • Also, remove the actual file itself wagtail/admin/static_src/wagtailadmin/js/vendor/mousetrap.min.js
  • Finally, this removal will need to be called out in the release notes, it is possible that custom code has used Mousetrap, expecting it to be in the global window namespace
  • Remove mousetrap library

Describe the solution you'd like (v2) Preferred

It seems like waiting for hotwired/stimulus#715 to be merged is not ideal, also, we probably want to build on a Stimulus controller approach to make it both easy to use simply and maybe one day have a nice 'presentation' of this one day.

Additionally, mousetrap is actually a pretty solid library and handles lots of combinations of keys, but we should be using this via an NPM package instead.

Hence, the proposal is as follows

  1. Create a new controller KeyboardController which would have the identifier w-keyboard.
  2. This controller would be used on the elements that receive the keyboard trigger and have a simple value only approach to start, the value would be key.
  3. The controller would, by default, simply call click on the element.
  4. We would use the mousetrap npm library https://www.npmjs.com/package/mousetrap instead of having it added via the editor_html file. Note: Happy if we want to use https://www.npmjs.com/package/hotkeys-js (but we need a solution for mod built in and this library does not have that see https://github.com/jaywcjlove/hotkeys-js/issues/390
  5. We would need a unit tested controller, and this can be built on do do more complex stuff down the line.

Example HTML

{% load i18n wagtailadmin_tags %}
<button type="submit" class="button {% if is_revision %}warning{% endif %}" disabled data-controller="w-kbd" data-kbd-key-value="mod+s">
  {% if icon_name %}{% icon name=icon_name %}{% endif %} {% trans 'Page locked' %}
</button>

Basic controller code

import { Controller } from '@hotwired/stimulus';
import Mousetrap from 'mousetrap'; // Remember to add to package.json and run `npm install`

/**
 * Adds the ability for an element to to be triggered by a keyboard shortcut.
 *
 * @example
 * ```html
 * <button data-controller="w-kbd" data-w-kbd-key="a">Activate me with <kbd>a</kbd></button>
 * ```
 *
 * @example
 * ```html
 * <button data-controller="w-kbd" data-keyboard-key="mod+p">
 *   Trigger with <kbd>Command โŒ˜</kbd> + <kbd>p</kbd> on macOS or <kbd>Ctrl</kbd> + <kbd>p</kbd> on Windows
 * </button>
 * ```
 */
export class KeyboardController extends Controller<HTMLButtonElement> {
  static values = {
    key: String,
  };

  declare keyValue: string;

  initialize() {
    this.handleKey = this.handleKey.bind(this);
  }

  handleKey(event: Event) {
    event.preventDefault();
    this.element.click();
  }

  keyValueChanged(key, previousKey) {
    // See https://stimulus.hotwired.dev/reference/values#change-callbacks

    if (previousKey && previousKey !== key) {
      Mousetrap.unbind(previousKey, this.handleKey);
    }

    Mousetrap.bind(key, this.handleKey);
  }
}

Related implementations

Describe alternatives you've considered

  • Leave as is, but we are only using this library for two small keyboard shortcuts and the current approach is fully supported by Stimulus without too much difference.

Additional context

  • This is part of a larger goal to migrate to Stimulus
  • A robust solution here may also be the starting point to solve for Keyboard shortcut documentation for editor in the Wagtail UIย #3949
  • Long term we may use this approach for other keyboard shortcuts implemented in ad-hoc ways (e.g. the comment button), or even new keyboard shortcuts like opening the side menu
  • Not a good first issue but good for anyone semi-confident with Jest testing, JavaScript and who has taken the time to read the Stimulus docs in full.
  • This is also part of a larger goal to avoid wagtail/admin/templates/wagtailadmin/pages/_editor_js.html being used everywhere and minimise global usage in JS See Stop importing _editor_js.html everywhereย #2936
  • Long term, we may further build on ActionController for other event dispatching via keyboard shortcuts such as 'focus' or something else.
  • An earlier approach at pulling out the keyboard shorts, pre-Stimulus, can be found in this closed PR Rework existing page editor shortcuts behaviour to be easier to build onย #8900
@suyash5053
Copy link
Contributor

I have started working on this issue.

@lb-
Copy link
Member Author

lb- commented Mar 8, 2023

Awesome @suyash5053 reach out if you need a hand or if something is not clear.

Be sure to read through Stimulus' docs on how keyboard filters work.

https://stimulus.hotwired.dev/reference/actions#keyboardevent-filter

Also, if it helps, feel free to do an initial PR that covers item 1 or 1 & 2 before handling the others. Even if it's a draft PR it may be good to get some early feedback.

@Lovelyfin00
Copy link
Contributor

Hey @suyash5053
Still working on this?

@suyash5053
Copy link
Contributor

Hey @Lovelyfin00 , yup I'm still working on this.

lb- pushed a commit to suyash5053/wagtail that referenced this issue Apr 17, 2023
lb- pushed a commit that referenced this issue Apr 17, 2023
@lb-
Copy link
Member Author

lb- commented Oct 24, 2023

Note to all - this is probably blocked until we get some traction on hotwired/stimulus#654 & hotwired/stimulus#715 on the Stimulus side.

To avoid unnecessary code churn, we can wait for now, but we may want to revisit this if we do not get any movement on the Stimulus repo for some time.

@lb-
Copy link
Member Author

lb- commented Feb 17, 2024

OK, I have updated this issue with a suggested "Describe the solution you'd like (v2) Preferred" section.

This is probably a better way for long term usage of keyboard shortcuts in Wagtail, still relies on Mousetrap (as a package) but gives us an abstract way to manage this and will be easier to replace the library with something else later if we want.

It also makes it much easier to add a keyboard shortcut to an element without having to know much about Stimulus action keyboard listeners and will support a broader range of keyboard shortcut declarations.

@NXPY123
Copy link
Contributor

NXPY123 commented Mar 7, 2024

Should the KeyboardController be made inside the client/src/controllers directory?

@lb-
Copy link
Member Author

lb- commented Mar 7, 2024

@NXPY123 - yep, all our Stimulus controllers go in that same folder.

@zerolab
Copy link
Contributor

zerolab commented Mar 8, 2024

I stumbled upon https://github.com/ai/keyux the other day and thought of this issue (source)

@NXPY123
Copy link
Contributor

NXPY123 commented Mar 9, 2024

I stumbled upon https://github.com/ai/keyux the other day and thought of this issue (source)

Is this being suggested over Mousetrap?

@lb-
Copy link
Member Author

lb- commented Mar 9, 2024

@NXPY123 for now let's keep the plan as is, a smaller migration to still use MouseTrap but via NPM and Stimulus.

I've had a quick look at the library and it's still quite new, it solves some additional problems that Mousetrap doesn't. However, it doesn't seem to account for a dynamic mod key per OS.

I do like it's use of aria-keyshortcuts, we may borrow that idea in our code either on this adoption or a future enhancement.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-keyshortcuts

I also like that it visually indicates what button is being activated when the shortcut key is pressed.

Anyway, it's a nice approach and we could adopt it easily in the future if needed.

My thinking generally is that the Stimulus attributes and approach solves for;

  • a. A consistent way to do things in the HTML (data attributes).
  • b. A reliable way to ensure that the behaviour attached to the html gets run no matter how the html is added. Initial page load, dynamic inlinePanel, dynamic StreamField blocks or Async modal - whatever.
  • c. Easy-ish to change any underlying implementation or library used. If we want to adopt hotkey.js or keyux or roll our own keyboard code, almost no html should need to change.

Cc @zerolab

@NXPY123
Copy link
Contributor

NXPY123 commented Mar 10, 2024

The third advantage you mentioned is huge, especially considering that it lets us modify and add new controllers in a central location with a proper hierarchy and easy access. It makes it incredibly flexible with allowance for any future adaptions of new packages. I think it would also help people new to that part of the codebase get started quickly and also debug issues easily.

@lb-
Copy link
Member Author

lb- commented Mar 10, 2024

Another alternative to consider for future is tinykeys https://github.com/jamiebuilds/tinykeys

lb- added a commit to NXPY123/wagtail that referenced this issue Apr 7, 2024
@lb- lb- closed this as completed in #11740 Apr 7, 2024
lb- added a commit that referenced this issue Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code type:Enhancement
Projects
Development

Successfully merging a pull request may close this issue.

5 participants