feat(tab): implement setFocusOnActivate #722
Conversation
Codecov Report
@@ Coverage Diff @@
## rc0.11.0 #722 +/- ##
============================================
- Coverage 95.18% 95.16% -0.02%
============================================
Files 73 73
Lines 2905 2914 +9
Branches 441 442 +1
============================================
+ Hits 2765 2773 +8
- Misses 45 46 +1
Partials 95 95
Continue to review full report at Codecov.
|
packages/tab/index.tsx
Outdated
| @@ -59,6 +60,7 @@ export default class Tab extends React.Component<TabProps, TabState> { | |||
|
|
|||
| static defaultProps: Partial<TabProps> = { | |||
| active: false, | |||
| focusOnActivate: false, | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I can change this to true, but I'm wondering what the reasoning behind this default is since it seems to me that focusing on activate is the non-standard scenario.
Tabs that are activated via mouse or keyboard navigation will already have focus so this setting mainly affects activation that happens a different way, e.g. on component mount. I think for accessibility reasons tabs should not be stealing focus by default but rather it should be something the developer explicitly opts into.
There was a problem hiding this comment.
Ya I definitely agree with you, and it cost me a few hours tracking down what was going on with our catalog app. However if we don't change this to true, we are initially out of sync with MDC Web's foundational layer, which causes bugs.
I think we should open an issue with MDC Web and then reflect it back here. What do you think?
There was a problem hiding this comment.
@ben-mckernan I actually talked to the team. The answer for default true is a11y. If a tab is programatically activated, the screenreader should know. Without focusing, you'd need to implement aria-live, which is not the recommended way of handling a11y things.
There was a problem hiding this comment.
Ah ok, interesting. I definitely need to read up more on these a11y recommendations it seems. Thanks for letting me know!
test/unit/tab/index.test.tsx
Outdated
| }); | ||
|
|
||
| test('when props.focusOnActivate is true, an active tab should be focused on mount', () => { | ||
| const wrapper = mount<Tab>(<Tab active focusOnActivate />, {attachTo: document.body}); |
There was a problem hiding this comment.
just tried running tests using npm run test:watch. It seems like this isn't valid. Please try using this pattern: 764ed40#diff-5e69e52eb0131589a6b4841a086ff648R65
There was a problem hiding this comment.
Do you mean the warnings from react about rendering directly to the body? Seems I didn't see those when running locally 😢
There was a problem hiding this comment.
Yup! Those warnings -- The tests pass, but they do produce warnings. I saw you pushed the changes. I will check them out now
ghost
deleted the
Fixes: #676