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

Date Picker / Combo Box: Use mouseover event instead of mousemove #4256

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jul 8, 2021

Description

This pull request seeks to improve performance of USWDS JavaScript by limiting the amount of event handling that occurs in response to mouse events.

Additional information

USWDS component scripts typically use event delegation for handling UI events, meaning that the actual event listener is attached to the document, and not to individual elements. A side-effect of this is that events are bound regardless of the presence of any particular components in the page.

The Date Picker and Combo Box components each bind events for mousemove in order to apply a focus effect when the cursor is placed over items within the element. mousemove can be a very noisy event, since it is emitted for every pixel movement (see demo). This can be problematic in combination with event delegation, where every movement of the cursor anywhere in the body is handled multiple times for each of the components, regardless whether any instances of that component exist in the page.

Truthfully, I'm not sure I understand the goal in programmatically focusing the component elements in response to cursor movement, but the changes here don't aim to change the behavior, but instead serve as a refactoring of the existing implementation.

Future improvements could consider...

  • Not using JavaScript to respond to mouse events at all, instead leaning primarily on CSS styling (i.e. :hover)
  • Apply event handlers at the component root instead of the document, so that the events are limited to only when instances of the component are present on the page, and only when cursor events occur inside the element.

This may indirectly fix #4184, based on debugging at #4184 (comment) and #4184 (comment).


Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this.

I agree with you; in the near future we shouldn't depend on JS for styling things that :hover should cover. Event handlers on component instead of document is something we want to work towards to make our components more modular.

@aduth
Copy link
Contributor Author

aduth commented Jul 13, 2021

Truthfully, I'm not sure I understand the goal in programmatically focusing the component elements in response to cursor movement, but the changes here don't aim to change the behavior, but instead serve as a refactoring of the existing implementation.

I was thinking about this a bit more, and if I had to guess, the idea with focusing the element when it's moused over might be to emulate a behavior similar to a standard <select> element, where if one mouses over an option and then proceeds to use arrow keys to navigate up or down, the selected item changes to the one before or after the moused-over item.

I'd be curious if there's related accessibility guidance for this, since similar examples in WCAG (e.g. navigation menu) don't have this expectation, and forced focusing on hover could be undesirable in that it would cause most assistive technology to announce every item on mouseover (which isn't the case for a <select>, in my testing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught TypeError: t.target.closest is not a function. In the header
3 participants