-
Notifications
You must be signed in to change notification settings - Fork 375
fix(Table) Table tooltips not triggered via keyboard #8758
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
Conversation
581cff6 to
54e31b9
Compare
|
Preview: https://patternfly-react-pr-8758.surge.sh A11y report: https://patternfly-react-pr-8758-a11y.surge.sh |
2d67690 to
33c696a
Compare
33c696a to
9cdff55
Compare
nicolethoen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm noticing that this example: modifiers with table text still doesn't receive keyboard focus to trigger the tooltip.
I could update the example to be the following and it would work:
export const TableTextModifiers: React.FunctionComponent = () => {
const [focused, setFocused] = React.useState(false);
return (
<TableComposable aria-label="Table text">
<Thead>
<Tr>
<Th width={30}>{columnNames.truncate}</Th>
<Th>{columnNames.wrap}</Th>
</Tr>
</Thead>
<Tbody>
<Tr>
<Td
onFocus={() => setFocused(true)}
onBlur={() => setFocused(false)}
tabindex={0}
dataLabel={columnNames.truncate}>
<TableText focused={focused} wrapModifier="truncate">This text will truncate instead of wrap.</TableText>
</Td>
<Td
dataLabel={columnNames.wrap}
>
<TableText wrapModifier="nowrap">
<a href="#">This is a link that needs to be on one line and fully readable.</a>
</TableText>
</Td>
</Tr>
</Tbody>
</TableComposable>
);
}
i'm not sure if there's a way we could update the API to make it a little simpler in the case of the TableText directly in a Td or Th with no sortable wrapper.
P.S. I was able to get the tests passing by rebasing and force pushing. You'll want to pull to update your branch before making subsequent commits.
|
I am trying to find a way to make it more universal. |
9cdff55 to
dbd75f3
Compare
thatblindgeye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part it looks like the tooltips are working with keyboard. I am noticing that when using VoiceOver and navigating a table with it, the tooltip doesn't render when truncated text receives VO focus (using the Modifiers with table text example). Compare that to our tooltip react ref example which does get the tooltip rendered on VO focus (even if the button in the example is replaced with a span). While the actual td or th element may receive keyboard focus when tabbing, VO (possibly other screen readers? would need to check) places focus on the content of the td or th instead it seems.
Adding the pf-c-table__button class to a button and placing TableText inside of it sort of worked for me, after removing the tabindex from the td in the example. @nicolethoen WDYT of something like that? It may be more beneficial to use buttons when using tooltips in general, even in cases of truncation, just because of possible issues assistive tech may face when tooltips are place on static elements like a span or div.
|
@thatblindgeye |
thatblindgeye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolethoen good point about the tooltip, screenreader focus may only really matter if a custom tooltip is passed in, but I believe there's still that (unrelated to this PR) issue where custom tooltips aren't rendering correctly. And that may also depend on whether using an info button would make more sense instead of a tooltip on the cell itself in that case.
dbd75f3 to
ab319c6
Compare
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #8158
Additional issues: Implements changes from this PR. Thanks @kmcfaul for this!