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

Issue #54: Focus ring does not account for radio buttons #57

Closed
wants to merge 4 commits into from

Conversation

majornista
Copy link

Per TODO in documentation:

TODO: ideally, we also trigger keyboard modality following a keyboard
event which activates an element or causes a mutation; this still needs
to be implemented.

Include other types of keyboard input than the tab key that may produce
focus navigation.

Per TODO in documentation:

> TODO: ideally, we also trigger keyboard modality following a keyboard
event which activates an element or causes a mutation; this still needs
to be implemented.

Include other types of keyboard input than the tab key that may produce
focus navigation.
@robdodson
Copy link
Collaborator

@marcoscaceres This PR is flagged as having IPR issues but I'm not sure when it's ok to "Mark as non-substantive". Are there rules that help define what is and isn't ok?

@marcoscaceres
Copy link
Contributor

@marcoscaceres This PR is flagged as having IPR issues but I'm not sure when it's ok to "Mark as non-substantive". Are there rules that help define what is and isn't ok?

The rule is: typos, grammar, rephrasing sentences, fixing up or adding examples/diagrams/notes, are all non-substantive. Anything that affects functionality/behavior or changes and algorithm is substantive.

If you are unsure, or it's unclear, then it's safer to say it's substantive and ask the contributor to join the WICG. And as always, if you are still unsure - just ping the chairs for review.

@robdodson
Copy link
Collaborator

ok so @majornista will need to visit this page https://www.w3.org/community/wicg/, read through it, and click 'Join Group', is that correct @marcoscaceres ?

@marcoscaceres
Copy link
Contributor

is that correct @marcoscaceres ?

Yep, that is correct. Sorry for the additional pain.

@majornista
Copy link
Author

Thanks, @robdodson and @marcoscaceres. I've joined the group, so we should now be able to discuss this PR more substantively.

…dEvent

Implements a more elaborate way of determining whether a keyboard event
should flag `hadKeyboardEvent`, based on the key pressed and input type
or role of the element receiving keyboard interaction.

Also adds `mousedown` handler to remove `focus-ring` class on mouse
interaction.
@srinivasvenigalla
Copy link

srinivasvenigalla commented Oct 31, 2017

Can you please add "button" to the "selectable":"role" please? This solves the problem of initial focus into a dialog.

Per WICG#57 (comment):

> Add "button" to the "selectable":"role" please? This solves the
problem of initial focus into a dialog.
# Conflicts:
#	dist/focus-ring.js
#	src/focus-ring.js
@robdodson
Copy link
Collaborator

This should be addressed by #68

@robdodson robdodson closed this Nov 15, 2017
@srinivasvenigalla
Copy link

srinivasvenigalla commented Dec 6, 2017

Hi @majornista, could you please add "menuitem" to selectables? Menuitems are also clickable..

@robdodson
Copy link
Collaborator

@srinivasvenigalla this PR was closed by #68. The polyfill does not special case ARIA roles because it's meant to match the browser heuristic and that does not take ARIA into account.

For menuitem are you expecting that it should match :focus-ring? If so, I think what you actually want is #42 (comment) which is an additional CSS property we're proposing that will let you hint to the browser when :focus-ring should match.

Until we add that property to the polyfill, the easiest thing to do is manually add the .focus-ring class yourself.

If there are elements which should always have a focus ring shown, authors may explicitly add the focus-ring class. If explicitly added, it will not be removed on blur.

@srinivasvenigalla
Copy link

srinivasvenigalla commented Dec 7, 2017

@robdodson if we add .focus-ring manually, it would display the focus ring for mouse also..

@robdodson
Copy link
Collaborator

@srinivasvenigalla sorry my mistake.

Thinking about this a bit more, I think what you're requesting should already work with the latest version because it sets keyboard modality if the user uses the arrow keys. https://github.com/WICG/focus-ring/blob/master/src/focus-ring.js#L82-L87

So if your menuitems are just div's with tabindex=0 and the user presses the up/down arrow keys to move between them, that should apply .focus-ring.

Can you verify you're using the latest version?

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

4 participants