spectrum-web-components icon indicating copy to clipboard operation
spectrum-web-components copied to clipboard

[Bug]: SWC uses keyup handlers more often than it should

Open adixon-adobe opened this issue 3 years ago • 3 comments

Code of conduct

  • [X] I agree to follow this project's code of conduct.

Impacted component(s)

Overlay and many more

Expected behavior

SWC should default to using keydown events unless there's a specific reason to use keyup.

Actual behavior

There's many places where SWC handles events with keyup. This makes it difficult if you have another handler for that key registered to keydown. Rather than being able to use event bubbling you'd need to add specific logic based on the handling SWC is doing.

The example we hit was the escape key handling in the overlay implementation. We have a keydown handler that was triggering for a component under the overlay.

Screenshots

No response

What browsers are you seeing the problem in?

No response

How can we reproduce this issue?

  1. Go to '...'
  2. Click on '....'
  3. Scroll to '....'
  4. Check console
  5. See error

Sample code that illustrates the problem

No response

Logs taken while reproducing problem

No response

adixon-adobe avatar Oct 06 '21 17:10 adixon-adobe

In talking with @charlessuh more about this, I think there's also a case for modal overlays to trap the escape key in the capture phase.

adixon-adobe avatar Oct 07 '21 14:10 adixon-adobe

We recently updated SWC and saw some additional problems around this area. We found that our dialogs were no longer intercepting key presses as a result of this SWC update, not sure the cause, but we found that we needed to explicitly stop propagation of keypress events. It seemed like it was related to the above, would be good to standardize on keyup for everything if possible.

bmancini-adobe avatar May 26 '22 20:05 bmancini-adobe

Getting a catalog of this together. keyup is used as the event name in ActionButton.ts, ButtonBase.ts, ColorArea.ts, overlay-stack.ts, and Radio.ts.

ActionButton.ts: leverages keyup to track the length of the press action to synthesize longpress and cannot be removed without a very different approach. In theory we could do some work with counting keypress events instead, but that seems less reliable that what we're doing now. Chance of change: none.

ButtonBase.ts: leverage keyup to synthesize click events on Button elements and its derivatives. I guess this could be moved to keydown but would not correctly map to what you get out of a native <button> element. Chance of change: low.

ColorArea.ts: keyup is used to track which modifier keys are down at any one time. Probably can move to https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/getModifierState and manage those keys in a possibly more stable way. Chance of change: good.

overlay-stack.ts: manages whether to close a modal or not. It seems that the <dialog> element does handle this on keydown, so happy to move this into alignment with that. Chance of change: High.

Radio.ts: leverage keyup to synthesize click events on the element. I guess this could be moved to keydown but would not correctly map to what you get out of a native <button> element. Chance of change: low.

With this in mind we'd definitely accept a PR with updates to Color Area and the Overlay Stack, if you were interested in hurrying this forward. If this is blocking for you, we can definitely raise the priority on our end, but can pretty solidly booked for at least the next two weeks.

I'd take reasoning or expanded ideas on the other elements that have keyup handlers, but am no very likely to accept changes there without a really solid alternative.

Thoughts?

Westbrook avatar May 26 '22 22:05 Westbrook