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

Fix documentation #510 #522

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@n-chardon
Copy link

n-chardon commented Jan 9, 2019

Fixes #510
Removed mention of the role attribute. Suggestion for fixing the error by disabling the rule for the element.

Fix documentation #510
Fixed #510 : removed mention of the role attribute. Suggestion for fixing the error with a comment.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 9, 2019

Coverage Status

Coverage remained the same at 99.443% when pulling 081bf7a on n-chardon:n-chardon-fix-issue510 into fff6a4b on evcohen:master.

@ljharb

ljharb approved these changes Jan 9, 2019

@@ -40,19 +40,21 @@ Note: Adding a role to your element does **not** add behavior. When a semantic H

### Case: This element is not a button, link, menuitem, etc. It is catching bubbled events from elements that it contains

If your element is catching bubbled click or key events from descendant elements, then the proper role for this element is `presentation`.
If your element is catching bubbled click or key events from descendant elements, there are no appropriate roles for your element : you will have to deactivate the rule. Consider explaining the reason for disabling the rule as well.

This comment has been minimized.

@ljharb

ljharb Jan 9, 2019

Collaborator
Suggested change Beta
If your element is catching bubbled click or key events from descendant elements, there are no appropriate roles for your element : you will have to deactivate the rule. Consider explaining the reason for disabling the rule as well.
If your element is catching bubbled click or key events from descendant elements, there are no appropriate roles for your element: you will have to deactivate the rule. Consider explaining the reason for disabling the rule as well.

@ljharb ljharb added the documentation label Jan 9, 2019

@jessebeach jessebeach self-requested a review Jan 10, 2019

@jessebeach
Copy link
Collaborator

jessebeach left a comment

I disagree with the recommendation. I would rather expand it and provide more examples. This rule is catching a pattern that can lead to ambiguous semantics. It's unfortunate that the value of role is presentation. I think the confusion would be reduced if the value we used were none. But we have the token we have.

I would rather that the documentation move in the direction of encouraging developers to separate their Application Structures from the User Interface. Putting click-handlers on elements that have non-interactive or static semantics is just a bad code smell and a habit that will most certainly lead to inaccessible interfaces: #510 (comment)

@n-chardon

This comment has been minimized.

Copy link

n-chardon commented Jan 10, 2019

@n-chardon

This comment has been minimized.

Copy link

n-chardon commented Jan 10, 2019

First example: With the ARIA combobox design pattern, there are two subcomponents: the textbox, and the listbox component. Keyboard interactions are managed on the textbox, but mouse interactions are managed on the listbox. If you add a wrapper <div> with a mouse event handler around the listbox, it will trigger the rule. If you add role="presentation" to the wrapper <div>, assistive technology may fail on children elements.

Second example: you want to make a whole <div> clickable, which contains lots of information and lots of markup. To make it usable for screenreader users , you should not wrap the whole block in a <button> tag. Instead, you will create a button inside the block, that will work with event bubbling : no actual event handler is needed for the <button> itself. Being natively focusable, the <button> element will also cater for the keyboard users' needs. See a quick example: https://codepen.io/n-chardon/pen/XoPYPR

Putting event handlers on static elements is not a code smell, from my point of view, given the above examples. Putting useless role attributes on elements is a code smell to me.

Regarding the Application Structure suggestion, I have a question: such a component would not trigger the rule, but still allow to create code that will not follow the rule: am I correct on this?

I understand that is a limitation of static code analysis, but I am a bit uneasy with suggesting ways around the rule that will create the problematic code, without any warning from the linter. My opinion is that disabling the rule in this case is much more explicit of the developer's intentions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment