Fix documentation #510

[n-chardon]

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

[coveralls]

Coverage increased (+0.0005%) to 99.484% when pulling 4758cbf on n-chardon:n-chardon-fix-issue510 into a2f2c54 on evcohen:master.

[jessebeach]

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: https://github.com/evcohen/eslint-plugin-jsx-a11y/issues/510#issuecomment-452923532

[n-chardon]

Role=none does not exist in the ARIA specification. You are suggesting using ARIA roles to fix a false positive in a linting tool, and the example in the current documentation leads to accessibility issues : it is the reason I took the time to make modifications to the documentation. There are at least two legitimate use cases I encountered in my job for using a event handler on a static element: I will provide them quickly.

[n-chardon]

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.

[jessebeach]

@n-chardon you convinced me. Please update the PR and I'll merge it. Thank you for taking the time to explain your reasoning.

[n-chardon]

@jessebeach thanks for taking the time to read me. I have updated the PR, please let me know if there is anything else to do on my side.

[ljharb]

In case you ran prettier on this, be aware that it’s markdown formatter is significantly worse (sometimes) than its js one.

[jessebeach]

@n-chardon as @ljharb mentioned, please propose the changes without the formatting changes. Sorry for the churn!

[n-chardon]

@ljharb eveything is up to date, and hopefully, much cleaner.

[ljharb]

@n-chardon thanks; I rebased it again and reverted a few more unrelated formatting changes.

@jessebeach i think this is ready for your review again :-)

LGTM, but deferring to @jessebeach

[jessebeach]

Good to go. The test failures are unrelated to these changes (at least, I cannot see any connection that is sane or obvious.).

[jessebeach]

I updated ESLint to the latest version locally on master and started getting the same errors as the Travis run. We'll get this committed once I get the ESLint issue sorted, which may be in a week because I go on vacation in 2 days I won't be looking at a computer for a week :)

[jessebeach]

eslint/eslint#12119

[TrevorBurnham]

@n-chardon 👋 I'm just now seeing this. Re:

If you add role="presentation" to the wrapper <div>, assistive technology may fail on children elements.

My understanding is that role="presentation" is a no-op on a <div>. E.g. in the markup

<div role="presentation">
  <button>Click me</button>
</div>

the computed role for the button is "button" (according to the Accessibility inspector in Chrome). Can you show an example where setting role="presentation" on a <div> makes a difference?

[n-chardon]

@TrevorBurnham you are right: in your example, the button will still be exposed as a button. However, I remember encountering an issue where role="presentation" erased the semantics of children. I will get back to you with an example.