Closed lb- closed 5 months ago
I have started working on this issue.
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.
Hey @suyash5053 Still working on this?
Hey @Lovelyfin00 , yup I'm still working on this.
Note to all - this is probably blocked until we get some traction on https://github.com/hotwired/stimulus/issues/654 & https://github.com/hotwired/stimulus/pull/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.
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.
Should the KeyboardController
be made inside the client/src/controllers
directory?
@NXPY123 - yep, all our Stimulus controllers go in that same folder.
I stumbled upon https://github.com/ai/keyux the other day and thought of this issue (source)
I stumbled upon https://github.com/ai/keyux the other day and thought of this issue (source)
Is this being suggested over Mousetrap?
@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;
Cc @zerolab
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.
Another alternative to consider for future is tinykeys https://github.com/jamiebuilds/tinykeys
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. - [x] Has been done via https://github.com/wagtail/wagtail/pull/10212 #### 2. Add a `mod` keyboard key - On Mac this should to `command` whereas on Windows and Linux it maps to `ctrl`. As per the Mousetrap docs. https://craig.is/killing/mice - This can be done by creating a custom key mapping - see https://github.com/hotwired/stimulus/issues/654 & the Stimulus docs above. - Important: validate the code in the linked issue actually works, and it should also have unit tests added. - This will need to be updated in `client/src/includes/initStimulus.ts` - [ ] Add `mod` keyboard key to Stimulus controller #### 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. ```html {% load wagtailadmin_tags %} {% for panel in side_panels %} {% 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: ```html {% load i18n wagtailadmin_tags %} ``` 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 libraryDescribe the solution you'd like (v2) Preferred
It seems like waiting for https://github.com/hotwired/stimulus/pull/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
KeyboardController
which would have the identifierw-keyboard
.value
only approach to start, the value would bekey
.click
on the element.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 formod
built in and this library does not have that see https://github.com/jaywcjlove/hotkeys-js/issues/390Example HTML
Basic controller code
Related implementations
Describe alternatives you've considered
Additional context
wagtail/admin/templates/wagtailadmin/pages/_editor_js.html
being used everywhere and minimise global usage in JS See #2936ActionController
for other event dispatching via keyboard shortcuts such as 'focus' or something else.