-
-
Notifications
You must be signed in to change notification settings - Fork 26
Fix keyboard activation in Mosaic editor (issue #587) #654
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
base: master
Are you sure you want to change the base?
Fix keyboard activation in Mosaic editor (issue #587) #654
Conversation
…ter/Space to activate buttons (fixes plone#587)
thet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thank you.
Again, tests would be nice, but the setup could be quite tedious so I don't require this.
Oh, we don't have any mosaic JavaScript tests, so tests are obsolete anyways.
|
@thet Thank you so much for the appreciation :) Can this PR be merged now? |
petschki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested these changes. Since the elements are not focusable with the keyboard, the events do not get fired (or only in some rare and random cases)
The general keyboard behavior of mosaic layout editor could need some love. I suggest to look at the event intialization here https://github.com/plone/plone.app.mosaic/blob/master/resources/js/mosaic.layout.js#L309 to inject more convenient keyboard evens. Key combinations with Shift or Ctrl would be also cool (for example to access "add" menu on the top right, or save the whole document with "CMD" + "Enter" ... users getting more and more used to that)
| // Ensure keyboard activation (Enter / Space) triggers the same action | ||
| btn.addEventListener("keydown", function (ev) { | ||
| if (ev.key === "Enter" || ev.key === " ") { | ||
| ev.preventDefault(); | ||
| // trigger the click handler attached above | ||
| btn.click(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my tests this event never gets fired, because tile buttons are not accessible right now via keyboard.
| }) | ||
| // ensure Enter/Space keyboard activation triggers the click handler | ||
| .on("keydown", function (ev) { | ||
| if (ev.key === "Enter" || ev.key === " ") { | ||
| ev.preventDefault(); | ||
| $(this).trigger("click"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here. My tests didn't fire the event at all.
Ensure Enter/Space activates Mosaic toolbar and tile buttons even when other handlers prevent default. This adds explicit key handlers to toolbar and tile buttons to trigger click on Enter/Space. Fixes #587.