medialize / ally.js

JavaScript library to help modern web applications with accessibility concerns
http://allyjs.io/
MIT License
1.53k stars 83 forks source link

Fix for Firefox accidentally triggering click after focus change on space #162

Closed backflip closed 6 years ago

backflip commented 6 years ago

Firefox has a rather weird bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1220143 Would you consider merging a PR introducing a fix for this? See Workaround block in http://jsbin.com/teciva/edit?html,js,output

rodneyrehm commented 6 years ago

Of course I'm open to PRs :) But I'm afraid I don't fully understand why this is a problem. Your fiddle says:

When using non-native button elements, it is recommended to add a keydown event listener triggering a click event on enter and space for accessibility reasons.

But I'm afraid that's a slightly off interpretation, as WAI-ARIA Authoring Practices, 2.6 Button says that Space and Enter should activate the button. In code that could look like:

function activateButton () {
  console.log('open dialog');
}

var element = document.querySelector('#should-be-a-button');
element.addEventListener('click', activateButton, false);
element.addEventListener('keydown', function(event) {
  if (event.keyCode === 13 || event.keyCode === 32) {
    event.preventDefault();
    activateButton(event);
  }
}, false);

Dispatching DOM events to trigger your own handlers should be avoided, as calling the appropriate function directly is much more efficient and robust. Unless you have to dispatch the click event (for some reason I don't yet understand): don't.

backflip commented 6 years ago

Good point! The main reason is that there are lots of external libraries involved in this application, bringing many click event listeners with them which are not under my control. So the most flexible approach seemed to generically trigger a click event on space and enter.

UPDATE: I have changed the wording in the demo accordingly.

rodneyrehm commented 6 years ago

focus-targeted events may seem "weird" when changing focus in the middle of an event sequence. consider:

keydown on #alpha
- focus shifed to #bravo in event handler
keyup on #bravo
keypress on #bravo
click on #bravo (not universal)

so what you want to do is delay the shift of focus until keyup/keypress/click have executed:

alpha.addEventListener('keydown', function() {
  setTimeout(function() {
    bravo.focus();
  });
}, false);

the setTimeout() (or setImmediate() for browsers that support it) will queue that function to be executed after the event cascade is complete. This way any click events sent by the browser, or synthetically sent by your handler, should be dispatched to #alpha, not #bravo - thereby side-stepping your problem of "the close button being invoked immediately".


This browser comparison table for the event sequences is a few months (years) old, but I don't expect things have changed much since. It shows how the target element changes and how browser behavior is all over the place…

backflip commented 6 years ago

Thanks a lot for the details! I agree that waiting to shift the focus would be cleaner. However, I'm relying on external libraries like https://github.com/edenspiekermann/a11y-dialog taking care of maintaining focus on their own. So I can't delay the focus shift there. But it could certainly make more sense to change this behavior upstream in the affected libraries than attempt to fix it here.

backflip commented 6 years ago

Btw: Couldn't this happen with ally.maintain.tabFocus as well if it isn't used in combination with setTimeout /setImmediate?

backflip commented 6 years ago

Hm, or I just switch the Space event listener from keydown to keyup, seems to fix the problem and would be consistent with how most browsers treat Space on native buttons.

Demo: http://jsbin.com/tijuqes/edit?html,js,output

rodneyrehm commented 6 years ago

Couldn't this happen with ally.maintain.tabFocus as well if it isn't used in combination with setTimeout /setImmediate?

you mean the split event sequence? sure.

Hm, or I just switch the Space event listener from keydown to keyup

I would probably stick to keydown, but open a11y-dialog using setTimeout()


I assume your problem is solved and we can close this issue?

backflip commented 6 years ago

I assume your problem is solved and we can close this issue?

Absolutely. I very much appreciate you taking time to investigate this and pointing me in the right direction!