Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(tab): implement setFocusOnActivate #722

Merged
merged 5 commits into from Mar 12, 2019
Merged

feat(tab): implement setFocusOnActivate #722

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bec9e7d
feat(tab): implement setFocusOnActivate
Mar 7, 2019
0c943c8
change default to true
Mar 11, 2019
3f1a434
add additional unit tests
Mar 11, 2019
d9c5df9
render tests to wrapper DOM node
Mar 12, 2019
4782568
add focusOnActivate default value to README
Mar 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/tab/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class MyApp extends React.Component {
Prop Name | Type | Description
--- | --- | ---
active | boolean | If true will activate the tab and indicator.
focusOnActivate | boolean | If true will focus itself when activated.
className | string | Classes to appear on className attribute of root element.
isFadingIndicator | boolean | Enables a fading indicator, instead of sliding (default).
indicatorContent | element | Element that will appear within the `<TabIndicator />` element.
Expand Down
17 changes: 13 additions & 4 deletions packages/tab/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import TabRipple, {TabRippleProps} from './TabRipple';

export interface TabProps extends React.HTMLProps<HTMLButtonElement> {
active?: boolean;
focusOnActivate?: boolean;
isFadingIndicator?: boolean;
indicatorContent?: React.ReactNode;
minWidth?: boolean;
Expand Down Expand Up @@ -59,6 +60,7 @@ export default class Tab extends React.Component<TabProps, TabState> {

static defaultProps: Partial<TabProps> = {
active: false,
focusOnActivate: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaults to true:

https://github.com/material-components/material-components-web/tree/master/packages/mdc-tab#mdctab-properties-and-methods

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that seems reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, interesting. I definitely need to read up more on these a11y recommendations it seems. Thanks for letting me know!

className: '',
isFadingIndicator: false,
indicatorContent: null,
Expand All @@ -76,9 +78,11 @@ export default class Tab extends React.Component<TabProps, TabState> {
};

componentDidMount() {
const {active, focusOnActivate} = this.props;
this.foundation = new MDCTabFoundation(this.adapter);
this.foundation.init();
if (this.props.active) {
this.foundation.setFocusOnActivate(focusOnActivate);
if (active) {
this.foundation.activate();
}
}
Expand All @@ -88,10 +92,14 @@ export default class Tab extends React.Component<TabProps, TabState> {
}

componentDidUpdate(prevProps: TabProps) {
if (this.props.active !== prevProps.active) {
if (this.props.active) {
const {active, focusOnActivate, previousIndicatorClientRect} = this.props;
if (focusOnActivate !== prevProps.focusOnActivate) {
this.foundation.setFocusOnActivate(focusOnActivate);
}
if (active !== prevProps.active) {
if (active) {
// If active state is updated through props, previousIndicatorClientRect must also be passed through props
this.activate(this.props.previousIndicatorClientRect);
this.activate(previousIndicatorClientRect);
} else {
this.deactivate();
}
Expand Down Expand Up @@ -176,6 +184,7 @@ export default class Tab extends React.Component<TabProps, TabState> {
const {
/* eslint-disable */
active,
focusOnActivate,
previousIndicatorClientRect,
className,
isFadingIndicator,
Expand Down
24 changes: 24 additions & 0 deletions test/unit/tab/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,30 @@ test('if props.active updates to false, foundation.deactivate is called', () =>
td.verify(wrapper.instance().deactivate(), {times: 1});
});

test('calls foundation.setFocusOnActivate when props.focusOnActivate changes from false to true', () => {
const wrapper = shallow<Tab>(<Tab focusOnActivate={false} />);
wrapper.instance().foundation.setFocusOnActivate = td.func();
wrapper.setProps({focusOnActivate: true});
td.verify(wrapper.instance().foundation.setFocusOnActivate(true), {times: 1});
});

test('calls foundation.setFocusOnActivate when props.focusOnActivate changes from true to false', () => {
const wrapper = shallow<Tab>(<Tab focusOnActivate />);
wrapper.instance().foundation.setFocusOnActivate = td.func();
wrapper.setProps({focusOnActivate: false});
td.verify(wrapper.instance().foundation.setFocusOnActivate(false), {times: 1});
});

test('when props.focusOnActivate is true, an active tab should be focused on mount', () => {
const wrapper = mount<Tab>(<Tab active focusOnActivate />, {attachTo: document.body});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just tried running tests using npm run test:watch. It seems like this isn't valid. Please try using this pattern: 764ed40#diff-5e69e52eb0131589a6b4841a086ff648R65

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the warnings from react about rendering directly to the body? Seems I didn't see those when running locally 😢

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! Those warnings -- The tests pass, but they do produce warnings. I saw you pushed the changes. I will check them out now

assert.equal(document.activeElement, wrapper.getDOMNode());
});

test('when props.focusOnActivate is false, an active tab should not be focused on mount', () => {
This conversation was marked as resolved.
Show resolved Hide resolved
const wrapper = mount<Tab>(<Tab active focusOnActivate={false} />, {attachTo: document.body});
assert.notEqual(document.activeElement, wrapper.getDOMNode());
});

test('#adapter.addClass adds class to state.classList', () => {
const wrapper = shallow<Tab>(<Tab />);
wrapper.instance().adapter.addClass('test-class');
Expand Down