EuiIcon no longer takes focus in Edge and IE unless tabIndex is specified as a value other than "-1"#900
Conversation
cjcenizal
force-pushed
the
icon-ie-focusable
branch
from
00a1cfe to
ad6efc9
Compare
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
This is the basic idea, but I just had a comment about the default nature of icons and the value that's being checked.
src/components/icon/icon.js
Outdated
|
|
||
| return <Svg className={classes} style={optionalCustomStyles} {...rest} />; | ||
| // This is a fix for IE and Edge, which ignores tabindex="-1" on an SVG, but respects | ||
| // focusable="false". We want to default SVGs to *not* be focusable. |
There was a problem hiding this comment.
I'm not sure we want to default this to not be focusable as I worry what that does to accessibility when icons are used to give meaning and therefore need their aria-label read. I think there are a few instances where we will want to automatically remove them via the component that uses it, like buttons/inputs, but I worry that the default usage of icons would mean they're never read by a screen reader.
src/components/icon/icon.js
Outdated
| return <Svg className={classes} style={optionalCustomStyles} {...rest} />; | ||
| // This is a fix for IE and Edge, which ignores tabindex="-1" on an SVG, but respects | ||
| // focusable="false". We want to default SVGs to *not* be focusable. | ||
| const focusable = tabIndex === '0' ? 'true' : 'false'; |
There was a problem hiding this comment.
I'd rather this logic be: const focusable = tabIndex === '-1' ? 'false' : 'true' because any other number besides -1 will allow it to be focusable. Also, that then directly relates to your comment about IE, otherwise it's confusing as to why you speak about -1 value but then check for 0 value.
There was a problem hiding this comment.
Another option would be to tie the focusable prop to aria-hidden="false" as this indicates that the icon provides no extra value to non-sighted users and can be ignored (even from tabbing). Though we might want to check for both in case a consumer adds tabindex="-1" we also want to make sure IE removes the tabbing that case.
|
Thanks! I've addressed your feedback. |
CHANGELOG.md
Outdated
| **Bug fixes** | ||
| - Removed `.nvmrc` file from published npm package ([#892](https://github.com/elastic/eui/pull/892)) | ||
| - `EuiComboBox` no longer shows the _clear_ icon when it's a no-op ([#890](https://github.com/elastic/eui/pull/890)) | ||
| - `EuiIcon` no longer takes focus in Edge and IE unless `tabIndex='0'` is specified ([#900](https://github.com/elastic/eui/pull/900)) |
cjcenizal
force-pushed
the
icon-ie-focusable
branch
from
0d8460e to
f8bf644
Compare
cjcenizal
force-pushed
the
icon-ie-focusable
branch
from
f8bf644 to
78a8af9
Compare
cjcenizal
changed the title
2 participants
Addresses suggestion from #898 (review)