Improve accessibility

singingwolfboy commented

2026-01-23 00:14:50 +01:00

(Migrated from github.com)

Web accessibility is important, and we should try to make our components accessible by default. The W3C provides an interactive example of the correct way to make an accessible toolbar; this pull request tries to integrate their suggestions into this project. It does the following:

MenuItems render as <button> elements by default, instead of <div> elements.
All event handlers for the mousedown event on these buttons have changed to handle the click event, instead. mousedown only works for users that use a mouse; click is device-independent, and also works for keyboard-based usage.
Appropriate ARIA roles have been added where necessary, as well as various ARIA attributes such as aria-pressed for active MenuItems, aria-checked for active items in a Dropdown, etc.
The toolbar now uses a roving tabindex to manage focus.
Dropdown contents render as list elements by default; <ol> if options.ordered is set to true, <ul> otherwise.
The menu can be positioned either before or after the editor in the DOM (important for sequencing tab stops), and can be set to either a horizontal or vertical orientation (important for determining which arrow keys are used to move the focus within the menu).

This is my first attempt to contribute to ProseMirror; I hope I'm doing it right! The CONTRIBUTING.md file mentioned running a linter using npm run lint, but that doesn't seem to be set up for this project.

Web accessibility is important, and we should try to make our components accessible by default. The W3C provides [an interactive example of the correct way to make an accessible toolbar](https://www.w3.org/WAI/ARIA/apg/patterns/toolbar/examples/toolbar/); this pull request tries to integrate their suggestions into this project. It does the following: * MenuItems render as `<button>` elements by default, instead of `<div>` elements. * All event handlers for the `mousedown` event on these buttons have changed to handle the `click` event, instead. `mousedown` only works for users that use a mouse; `click` is device-independent, and also works for keyboard-based usage. * Appropriate ARIA roles have been added where necessary, as well as various ARIA attributes such as `aria-pressed` for active MenuItems, `aria-checked` for active items in a Dropdown, etc. * The toolbar now uses a [roving `tabindex`](https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#kbd_roving_tabindex) to manage focus. * Dropdown contents render as list elements by default; `<ol>` if `options.ordered` is set to true, `<ul>` otherwise. * The menu can be positioned either before or after the editor in the DOM (important for sequencing tab stops), and can be set to either a horizontal or vertical orientation (important for determining which arrow keys are used to move the focus within the menu). This is my first attempt to contribute to ProseMirror; I hope I'm doing it right! The `CONTRIBUTING.md` file mentioned running a linter using `npm run lint`, but that doesn't seem to be set up for this project.

marijnh commented

2026-01-23 19:46:26 +01:00

(Migrated from github.com)

Thanks for working on this. It's something that should probably have happened long ago. Though my official stance is still that this is more of a demonstration of how to implement a menu than a production-quality menu, that demonstration should at least point people towards accessibility.

All event handlers for the mousedown event on these buttons have changed to handle the click event

Does using a key handler that reacts to space and enter also provide a way around this? Mouse users don't want their editor to lose focus when they click a toolbar button, which is the reason we were using mousedown.

It seems like the four setFocusTo* methods could be shortened by having a setFocusToIndex helper.

Is there a pressing use case that we'd need the ol and horizontal orientation support in drop-downs for? Those seem like they might cost more code than they are worth.

And can we set up the default styles so that the menu looks the way it used to? I.e. no button borders and backgrounds.

Thanks for working on this. It's something that should probably have happened long ago. Though my official stance is still that this is more of a demonstration of how to implement a menu than a production-quality menu, that demonstration should at least point people towards accessibility. > All event handlers for the mousedown event on these buttons have changed to handle the click event Does using a key handler that reacts to space and enter also provide a way around this? Mouse users don't want their editor to lose focus when they click a toolbar button, which is the reason we were using mousedown. It seems like the four `setFocusTo*` methods could be shortened by having a `setFocusToIndex` helper. Is there a pressing use case that we'd need the `ol` and horizontal orientation support in drop-downs for? Those seem like they might cost more code than they are worth. And can we set up the default styles so that the menu looks the way it used to? I.e. no button borders and backgrounds.

marijnh commented

2026-01-23 20:42:14 +01:00

(Migrated from github.com)

Oh also, there's an issue with keyboard focus motion not skipping hidden items.

A way to move out of a drop-down menu with keyboard (I guess with escape) would probably be good to have.

Oh also, there's an issue with keyboard focus motion not skipping hidden items. A way to move out of a drop-down menu with keyboard (I guess with escape) would probably be good to have.

singingwolfboy commented

2026-01-25 12:18:09 +01:00

(Migrated from github.com)

Does using a key handler that reacts to space and enter also provide a way around this? Mouse users don't want their editor to lose focus when they click a toolbar button, which is the reason we were using mousedown.

Turns out the event object passed to a click event handler has a detail attribute that counts mouse clicks. We can use that to determine whether the event was triggered by the keyboard (0 clicks) or the mouse (1 or more clicks). I've added a workaround so that when a mouse user clicks a toolbar button, the focus is immediately restored to the editor after spec.run() is finished.

This is not quite the same behavior as before. Previously, the editor stayed focused consistently; with this change, the editor loses focus when the toolbar button is clicked, but immediately regains focus afterwards. I would argue that the new behavior is actually more correct -- but I'm not sure how much that matters. I don't see a way to keep the previous behavior while making this properly accessible.

It seems like the four setFocusTo* methods could be shortened by having a setFocusToIndex helper.

Oh also, there's an issue with keyboard focus motion not skipping hidden items.

Good call, on both counts! This ended up being rather finicky to implement correctly, and I had to write not just a setFocusToIndex() method, but also a makeContentIndexMover() method to make a function that could determine which index to move to, based on which items are hidden. I also had to (almost) duplicate this code three times: once for MenuBarView, once for Dropdown, and once for DropdownSubmenu. The three are not quite the same, because of small differences in how those three classes are implemented. I could maybe try to refactor this into a single standalone class and make these three classes inherit from it -- but that's an even bigger change, and this pull request is getting rather large and complicated already. I decided to leave the duplicated code as-is, and let you decide what is the best way to deal with the situation.

Is there a pressing use case that we'd need the ol and horizontal orientation support in drop-downs for? Those seem like they might cost more code than they are worth.

I figured that ol support would be very useful for headings (h1 to h6). I don't have a pressing use case for horizontal orientation support, but I could easily imagine people wanting this feature -- and since it wasn't hard to make the keybindings dynamically based on the orientation, I figured I would include it. If you really want to take it out, we can, but I'd prefer to leave it in.

And can we set up the default styles so that the menu looks the way it used to? I.e. no button borders and backgrounds.

I believe I've fixed this, but I haven't done extensive browser testing. Please let me know if I missed anything.

> Does using a key handler that reacts to space and enter also provide a way around this? Mouse users don't want their editor to lose focus when they click a toolbar button, which is the reason we were using mousedown. Turns out the `event` object passed to a `click` event handler has [a `detail` attribute that counts mouse clicks](https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/detail). We can use that to determine whether the event was triggered by the keyboard (0 clicks) or the mouse (1 or more clicks). I've added a workaround so that when a mouse user clicks a toolbar button, the focus is immediately restored to the editor after `spec.run()` is finished. This is not _quite_ the same behavior as before. Previously, the editor stayed focused consistently; with this change, the editor loses focus when the toolbar button is clicked, but immediately regains focus afterwards. I would argue that the new behavior is actually _more correct_ -- but I'm not sure how much that matters. I don't see a way to keep the previous behavior while making this properly accessible. > It seems like the four `setFocusTo*` methods could be shortened by having a `setFocusToIndex` helper. > Oh also, there's an issue with keyboard focus motion not skipping hidden items. Good call, on both counts! This ended up being rather finicky to implement correctly, and I had to write not just a `setFocusToIndex()` method, but also a `makeContentIndexMover()` method to make a function that could determine _which_ index to move to, based on which items are hidden. I also had to (almost) duplicate this code three times: once for `MenuBarView`, once for `Dropdown`, and once for `DropdownSubmenu`. The three are not _quite_ the same, because of small differences in how those three classes are implemented. I could _maybe_ try to refactor this into a single standalone class and make these three classes inherit from it -- but that's an even bigger change, and this pull request is getting rather large and complicated already. I decided to leave the duplicated code as-is, and let you decide what is the best way to deal with the situation. > Is there a pressing use case that we'd need the `ol` and horizontal orientation support in drop-downs for? Those seem like they might cost more code than they are worth. I figured that `ol` support would be very useful for headings (`h1` to `h6`). I don't have a pressing use case for horizontal orientation support, but I could easily imagine people wanting this feature -- and since it wasn't hard to make the keybindings dynamically based on the orientation, I figured I would include it. If you really want to take it out, we can, but I'd prefer to leave it in. > And can we set up the default styles so that the menu looks the way it used to? I.e. no button borders and backgrounds. I _believe_ I've fixed this, but I haven't done extensive browser testing. Please let me know if I missed anything.

marijnh commented

2026-01-26 09:56:12 +01:00

(Migrated from github.com)

I've added a workaround so that when a mouse user clicks a toolbar button, the focus is immediately restored to the editor after spec.run() is finished.

Unfortunately, that won't do. As you notice, it adds superfluous focus/blur churn, but also, if the button for example opens a dialog (to prompt for a link, say), and focuses that, forcing focus to the editor is not appropriate. We really do need to avoid the focus moving in the first place.

> I've added a workaround so that when a mouse user clicks a toolbar button, the focus is immediately restored to the editor after spec.run() is finished. Unfortunately, that won't do. As you notice, it adds superfluous focus/blur churn, but also, if the button for example opens a dialog (to prompt for a link, say), and focuses that, forcing focus to the editor is not appropriate. We really do need to avoid the focus moving in the first place.

singingwolfboy commented

2026-01-26 13:13:40 +01:00

(Migrated from github.com)

I thought about this a little more, and realized something very simple and obvious: it's the mousedown event that causes the focus to change, we can prevent that from happening by calling event.preventDefault(), and mousedown is only triggered by the mouse (so doing this won't interfere with keyboard-based interactions). Fixing the focus problem was as simple as adding this one line:

dom.addEventListener("mousedown", e => e.preventDefault())

Thanks for pushing me to think a little harder about this; I don't know why I didn't think of it before!

I thought about this a little more, and realized something very simple and obvious: it's the `mousedown` event that causes the focus to change, we can prevent that from happening by calling `event.preventDefault()`, and `mousedown` is _only_ triggered by the mouse (so doing this won't interfere with keyboard-based interactions). Fixing the focus problem was as simple as adding this one line: ```js dom.addEventListener("mousedown", e => e.preventDefault()) ``` Thanks for pushing me to think a little harder about this; I don't know why I didn't think of it before!

marijnh commented

2026-01-27 12:23:18 +01:00

(Migrated from github.com)

Thanks! I've merged this as e4cd7082cd, and followed up with 373628e31d to simplify some of the code and address a few minor issues. Could you take a look and see if the revised version still works for you?

Thanks! I've merged this as e4cd7082cd1c8003631581f3c180a5f121fb9e50, and followed up with 373628e31d607db45f899f05b719 to simplify some of the code and address a few minor issues. Could you take a look and see if the revised version still works for you?

singingwolfboy commented

2026-01-27 15:36:33 +01:00

(Migrated from github.com)

It seems to work great! I've found only one very minor bug; and it may have been in my version of the code as well: re-entering a submenu resets the focus to the first element, but does not reset the focusIndex, so the focus jumps unexpectedly when pressing the down arrow.

To reproduce this, use the dev editor available from running bin/pm dev-start. Use the keyboard to shift your focus to the toolbar, and use the arrow keys to move to the "Type..." dropdown. Use the keyboard to focus the "Heading" item, and press the right arrow to open the submenu and focus "Level 1". Press the down arrow twice, so that "Level 3" is now focused. Press the left arrow to close the submenu, and then the right arrow to open it again; "Level 1" is now focused once again. However, at this point, if you press the down arrow, the item that recieves focus is not "Level 2", as you might expect, but "Level 4"!

It seems to work great! I've found only one very minor bug; and it may have been in my version of the code as well: re-entering a submenu resets the focus to the first element, but does _not_ reset the focusIndex, so the focus jumps unexpectedly when pressing the down arrow. To reproduce this, use the dev editor available from running `bin/pm dev-start`. Use the keyboard to shift your focus to the toolbar, and use the arrow keys to move to the "Type..." dropdown. Use the keyboard to focus the "Heading" item, and press the right arrow to open the submenu and focus "Level 1". Press the down arrow twice, so that "Level 3" is now focused. Press the left arrow to close the submenu, and then the right arrow to open it again; "Level 1" is now focused once again. However, at this point, if you press the down arrow, the item that recieves focus is not "Level 2", as you might expect, but "Level 4"!

marijnh commented

2026-01-28 08:39:24 +01:00

(Migrated from github.com)

Thanks for catching that. Patch 661ec54 should help.

Thanks for catching that. Patch 661ec54 should help.

singingwolfboy commented

2026-01-28 10:21:16 +01:00

(Migrated from github.com)

Great! I found two more issues. One I was able to solve with this patch: #39. The other is more tricky to solve.

I noticed that when the "Heading" submenu is open, none of the items receive aria-checked="true". When the cursor is on a heading in the editor, that item in the submenu is correctly disabled, but the aria-checked attribute remains set to "false". I tracked this down to the update function inside the render method of MenuItem: if the menu item is not enabled, then the active method is never called, and it is assumed that the menu item is not active.

Is this an overzealous optimization, or is there a reason why the code is like this? For radio buttons in general, it should be possible for one item to be both active and disabled simultaneously: it doesn't make sense to set a "heading 1" to "heading 1" a second time, but that doesn't change the fact that it is a "heading 1" currently, and the radio button is active.

Great! I found two more issues. One I was able to solve with this patch: #39. The other is more tricky to solve. I noticed that when the "Heading" submenu is open, none of the items receive [`aria-checked="true"`](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-checked). When the cursor is on a heading in the editor, that item in the submenu is correctly disabled, but the `aria-checked` attribute remains set to "false". I tracked this down to the `update` function inside the `render` method of `MenuItem`: if the menu item is not enabled, then the `active` method is never called, and it is assumed that the menu item is not active. Is this an overzealous optimization, or is there a reason why the code is like this? For radio buttons in general, it should be possible for one item to be both active and disabled simultaneously: it doesn't make sense to set a "heading 1" to "heading 1" a second time, but that doesn't change the fact that it _is_ a "heading 1" currently, and the radio button is active.

marijnh commented

2026-01-29 09:42:35 +01:00

(Migrated from github.com)

none of the items receive aria-checked="true"

Since these dropdowns are defined as generic all-purpose menus, not strictly radio controls, I'm starting to feel that menuitem might a better role than menuitemradio. That would also give us at lease one less tricky attribute to manage. Does that sound reasonable?

> none of the items receive `aria-checked="true"` Since these dropdowns are defined as generic all-purpose menus, not strictly radio controls, I'm starting to feel that `menuitem` might a better role than `menuitemradio`. That would also give us at lease one less tricky attribute to manage. Does that sound reasonable?

singingwolfboy commented

2026-01-29 09:51:20 +01:00

(Migrated from github.com)

none of the items receive aria-checked="true"

Since these dropdowns are defined as generic all-purpose menus, not strictly radio controls, I'm starting to feel that menuitem might a better role than menuitemradio. That would also give us at lease one less tricky attribute to manage. Does that sound reasonable?

For the general case, that makes sense -- although then it makes sense to allow callers to specify an option to set options in the dropdown to menuitemcheckbox or menuitemradio, instead of the default.

However, I'll remind you that you wanted to take out the orientation option that I originally included, because we didn't have a pressing use case for it. There is a pressing use case for setting these items to menuitemradio: they help users with assistive technology better understand the structure of the interface they are using, in this specific case (i.e. selecting a block type for the text that is currently selected in the editor).

I'm fine with making menuitem the default role for dropdown items, but I do feel that there should be a way for callers to specify that it should be menuitemradio instead, and if so, the dropdown should handle setting aria-checked appropriately.

> > none of the items receive `aria-checked="true"` > > Since these dropdowns are defined as generic all-purpose menus, not strictly radio controls, I'm starting to feel that `menuitem` might a better role than `menuitemradio`. That would also give us at lease one less tricky attribute to manage. Does that sound reasonable? For the general case, that makes sense -- although then it makes sense to allow callers to specify an option to set options in the dropdown to `menuitemcheckbox` or `menuitemradio`, instead of the default. However, I'll remind you that you wanted to take out the `orientation` option that I originally included, because we didn't have a pressing use case for it. There _is_ a pressing use case for setting these items to `menuitemradio`: they help users with assistive technology better understand the structure of the interface they are using, in this _specific_ case (i.e. selecting a block type for the text that is currently selected in the editor). I'm fine with making `menuitem` the default role for dropdown items, but I do feel that there should be a way for callers to specify that it should be `menuitemradio` instead, and if so, the dropdown should handle setting `aria-checked` appropriately.

marijnh commented

2026-01-29 09:52:27 +01:00

(Migrated from github.com)

Another issue I noticed is that picking an item from a drop-down with the keyboard will not close the drop-down. This is especially jarring with menu items that open a dialog, moving the focus away from the menu but leaving it open.

I'm not sure whether the proper solution is to always close all open menus when an item is activated, or to do focus tracking in order to only do this when the action moves focus away.

Another issue I noticed is that picking an item from a drop-down with the keyboard will not close the drop-down. This is especially jarring with menu items that open a dialog, moving the focus away from the menu but leaving it open. I'm not sure whether the proper solution is to always close all open menus when an item is activated, or to do focus tracking in order to only do this when the action moves focus away.

singingwolfboy commented

2026-01-29 11:11:25 +01:00

(Migrated from github.com)

Another issue I noticed is that picking an item from a drop-down with the keyboard will not close the drop-down. This is especially jarring with menu items that open a dialog, moving the focus away from the menu but leaving it open.

I discovered earlier today that apparently, Enter and Space are suppose to do subtly different things on a menuitemradio role: Enter should cause the menu to close, but Space should not. See MDN reference here. I'm still learning about web accessibility: there's a lot of depth here!

Considering that, this is another reason why it makes sense to have the dropdown options use the menuitem role by default. Enter and Space are supposed to behave identically for menuitem.

> Another issue I noticed is that picking an item from a drop-down with the keyboard will not close the drop-down. This is especially jarring with menu items that open a dialog, moving the focus away from the menu but leaving it open. I discovered earlier today that apparently, `Enter` and `Space` are suppose to do subtly different things on a `menuitemradio` role: `Enter` should cause the menu to close, but `Space` should not. [See MDN reference here.](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/menuitemradio_role#keyboard_interactions) I'm still learning about web accessibility: there's a lot of depth here! Considering that, this is another reason why it makes sense to have the dropdown options use the `menuitem` role by default. [`Enter` and `Space` are supposed to behave identically for `menuitem`.](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/menuitem_role#enter)

marijnh commented

2026-02-04 12:02:14 +01:00

(Migrated from github.com)

I've pushed 0d374f45aa and b9abb22ed9. The latter follows Quill and Google Docs in refocusing the editor when a menu item is picked while focus is on that item, and makes closing of menus a bit more robust, autoclosing them when they lose focus, so that even if no click events are involved, they don't stick around open.

I've pushed 0d374f45aa845a23f and b9abb22ed9e0d52a5c. The latter follows Quill and Google Docs in refocusing the editor when a menu item is picked while focus is on that item, and makes closing of menus a bit more robust, autoclosing them when they lose focus, so that even if no click events are involved, they don't stick around open.

singingwolfboy commented

2026-02-16 20:33:03 +01:00

(Migrated from github.com)

Looks good to me! Could we get a new release of this project on NPM, so people can start using this more accessible code?

marijnh commented

2026-02-17 08:23:19 +01:00

(Migrated from github.com)

Let's. I've tagged 1.3.0.

singingwolfboy commented

2026-02-17 14:27:36 +01:00

(Migrated from github.com)

Thank you, @marijnh! 🙏

singingwolfboy commented

2026-02-17 16:48:16 +01:00

(Migrated from github.com)

Actually, could you please re-deploy the prosemirror.net website, to use the updated code? I don't think anything needs to change in the prosemirror/website repo, I think it just needs to be re-deployed.

Actually, could you please re-deploy the prosemirror.net website, to use the updated code? I don't think anything needs to change in the `prosemirror/website` repo, I think it just needs to be re-deployed.

marijnh commented

2026-02-17 17:17:08 +01:00

(Migrated from github.com)

Should be up to date now.

👍 1

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

Improve accessibility #38

Pull request closed