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

Keyboard usability/acessibility patches #54

Merged
merged 6 commits into from Apr 26, 2013

Conversation

ygormutti
Copy link

Hi, Marghoob!

I've made some changes in the plugin so the user can focus, blur, filter and open the dropdown just by using the keyboard. Also, I've fixed some bugs and implemented focus/blur event handling.

I've tried many ways of firing these "new" events consistently across the browsers we support (Chrome, FF, IE) without breaking keyboard usability, but the best I could get so far still has a bug in IE: the events are triggered unnecessarily if the filter box is used. At least they are always triggered in the right order, so I decided to leave it that way. I think this occurs because the .off() method of jQuery doesn't work in IE in the same way as in the other browsers.

Could you please read my comments on the commits, give feedback on them and, if you like them, pull my changes? (and figure out how to fix this bug, maybe?)

Thank you very much for the plugin!

PS: I didn't test multi-selection nor list mode, because we only use the default mode here.

Ygor Mutti added 6 commits February 14, 2013 11:02
This turns the wrapper into a focusable element. Also, the select element
becomes unable to get focus until the dropdown is destroyed.
The arrows keys, characters keys and the enter key open the dropdown. The
character keys also show the filter box if the dropdown isn't a list.
... but only if isn't multiple selector or list. Also, this fixes a minor
bug that happens when the user clicks somewhere else with the dropdown
open, making the title different from the selected option in the select.
Some juggling with the event handlers was needed to avoid calling them
unnecessarily when focus goes from wrapper to filter field.

fireEventIfExists was triggering events twice. Looks like an else was
missing. Changed from trigger to triggerHandler because "trigger"ing focus
on the select makes the wrapper lose focus and breaks keyboard usability.
@marghoobsuleman
Copy link
Owner

Awesome! Let me test it :)

@@ -594,8 +596,14 @@ function dd(element, settings) {
$("#" + childid + " li." + _styles.enabled).off("mousedown");
$("#" + childid + " li." + _styles.enabled).off("mouseup");
};
var _triggerBypassingHandler = function (id, evt_n, handler) {
Copy link
Author

Choose a reason for hiding this comment

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

I tried to pass extra arguments when triggering events, instead of using this way of bypassing events, so in the event handler I could check for the extra argument and skip the event or not, but extra arguments don't work when the event is blur and the element is focused (https://forum.jquery.com/topic/extra-arguments-passed-to-trigger-doesn-t-reach-blur-event-handler-when-element-has-focus)

UPDATE: this misbehavior is a bug http://bugs.jquery.com/ticket/13428

@daveh42
Copy link

daveh42 commented Mar 18, 2013

Hi Mamutti,

I have tried your modified code and it seems to work well for me except there isn't any visual indication to the user when the dropdown receives the focus. Any ideas on how to provide an indication to the user that the dropdown has the focus?

Thanks, Dave

@ygormutti
Copy link
Author

Hi, Dave!

If I remember correctly, the versions I've tested of Chrome and Firefox show a pretty border around the dropdown selector and IE (always IE...) shows an ugly dotted border around the entire thing. I'm on a hurry now, but I promise I'll provide screenshots later.

Could you please specify your test conditions?

PS: as a workaround you can add another div around the selector and use the focus event to put a border in that div

@daveh42
Copy link

daveh42 commented Mar 18, 2013

Hi Mamutti,

I discovered what was causing my problem. I was using a reset style sheet that was overriding the focus highlighting. I removed the reset style sheet and it works now. Thanks for your addition to this great plugin.

Dave

@ygormutti
Copy link
Author

Thank you for testing this patch, Dave.

marghoobsuleman added a commit that referenced this pull request Apr 26, 2013
Keyboard usability/acessibility patches
@marghoobsuleman marghoobsuleman merged commit 567372d into marghoobsuleman:master Apr 26, 2013
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.

None yet

3 participants