search

jupyterlab / lumino

Lumino is a library for building interactive web applications
https://lumino.readthedocs.io/
Other
635 stars 126 forks source link

MenuBar: do not focus on hover #607

Closed gabalafou closed 1 year ago

gabalafou commented 1 year ago

When the MenuBar class was created, it was designed so that whenever a user hovers over an item in the menu with a mouse, the active index would change to the index of the item being hovered. The setter for the active index also had a side effect that would update the DOM in order to apply the active CSS class to the item being hovered so that clients consuming this class could style the item (just like the :hover pseudo-class).

Later, in an attempt to make the Lumino menu bar more accessible, tabindex=0 values were added to the menubar items via #187. With the menu bar items keyboard-focusable (but with no CSS styling to indicate it), it made sense to try to synchronize the active index with the item focussed.

However, this had one unfortunate side effect, which was that whenever a user moved their mouse over a menu bar item, the app would move browser focus from anywhere else in the app onto that item.

This PR is an attempt to fix that issue and to put the menu bar's focus management on better footing.

Originally I refactored the menu bar class to simply not change activeIndex on mousemove, but I realized that that would probably break a lot of downstream code. For years, since the beginning it seems, the convention of the Lumino menu bar has been that if an item in the menu bar is hovered, then Lumino will apply the lm-mod-active class. In the interest of not breaking people's code, I decided to keep that convention.

marthacryan commented 1 year ago

Thank you so much for working on this - and for the super thorough descriptions, research, and codepens! It really helps to have all the context with this stuff cause it can be sort of subtle. I haven't tested this locally but as the person that added a lot of the changes that you're addressing in this PR, I approve! I also might recommend someone that's worked a bit more with Lumino checks this out too (@afshin) cause it's been a while since I made changes to Lumino.

fcollonval commented 1 year ago

Hey @gabalafou would you mind running:

yarn api
yarn lint

To fix the CI?

gabalafou commented 1 year ago

@fcollonval done! But running yarn api changed a bunch of API doc files that have nothing to do with the changes in this PR, not sure what's going on...