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.

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

feat(tab-bar): Add focusOnActivate flag #3748

Merged
merged 3 commits into from
Oct 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/mdc-tab-bar/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ Mixin | Description

## `MDCTabBar` Properties and Methods

Property | Value Type | Description
--- | --- | ---
`focusOnActivate` | `boolean` (write-only) | Sets whether tabs focus themselves when activated. Defaults to `true`.
`useAutomaticActivation` | `boolean` (write-only) | Sets how tabs activate in response to keyboard interaction. Automatic (`true`) activates as soon as a tab is focused with arrow keys; manual (`false`) activates only when the user presses space/enter. The default is automatic (`true`).

Method Signature | Description
--- | ---
`activateTab(index: number) => void` | Activates the tab at the given index.
Expand Down Expand Up @@ -136,6 +141,7 @@ Method Signature | Description
Method Signature | Description
--- | ---
`activateTab(index: number) => void` | Activates the tab at the given index.
`setUseAutomaticActivation(useAutomaticActivation: boolean) => void` | Sets how tabs activate in response to keyboard interaction. Automatic (`true`) activates as soon as a tab is focused with arrow keys; manual (`false`) activates only when the user presses space/enter.
`handleKeyDown(evt: Event) => void` | Handles the logic for the `"keydown"` event.
`handleTabInteraction(evt: Event) => void` | Handles the logic for the `"MDCTab:interacted"` event.
`scrollIntoView(index: number) => void` | Scrolls the Tab at the given index into view.
4 changes: 4 additions & 0 deletions packages/mdc-tab-bar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ class MDCTabBar extends MDCComponent {
return new MDCTabBar(root);
}

set focusOnActivate(focusOnActivate) {
this.tabList_.forEach((tab) => tab.focusOnActivate = focusOnActivate);
}

set useAutomaticActivation(useAutomaticActivation) {
this.foundation_.setUseAutomaticActivation(useAutomaticActivation);
}
Expand Down
4 changes: 3 additions & 1 deletion packages/mdc-tab/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@ Mixin | Description

Property | Value Type | Description
--- | --- | ---
`active` | `boolean` | Allows getting the active state of the tab.
`active` | `boolean` (read-only) | Allows getting the active state of the tab.
`focusOnActivate` | `boolean` (write-only) | Sets whether the tab should focus itself when activated. Defaults to `true`.

Method Signature | Description
--- | ---
Expand Down Expand Up @@ -182,6 +183,7 @@ Method Signature | Description
--- | ---
`handleClick() => void` | Handles the logic for the `"click"` event.
`isActive() => boolean` | Returns whether the tab is active.
`setFocusOnActivate(focusOnActivate: boolean) => void` | Sets whether the tab should focus itself when activated.
`activate(previousIndicatorClientRect: ClientRect=) => void` | Activates the tab. `previousIndicatorClientRect` is an optional argument.
`deactivate() => void` | Deactivates the tab.
`computeDimensions() => MDCTabDimensions` | Returns the dimensions of the tab.
Expand Down
16 changes: 13 additions & 3 deletions packages/mdc-tab/foundation.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ class MDCTabFoundation extends MDCFoundation {
constructor(adapter) {
super(Object.assign(MDCTabFoundation.defaultAdapter, adapter));

/** @private {function(?Event): undefined} */
this.handleClick_ = () => this.handleClick();
/** @private {boolean} */
this.focusOnActivate_ = true;
}

/**
Expand All @@ -93,6 +93,14 @@ class MDCTabFoundation extends MDCFoundation {
return this.adapter_.hasClass(cssClasses.ACTIVE);
}

/**
* Sets whether the tab should focus itself when activated
* @param {boolean} focusOnActivate
*/
setFocusOnActivate(focusOnActivate) {
this.focusOnActivate_ = focusOnActivate;
}

/**
* Activates the Tab
* @param {!ClientRect=} previousIndicatorClientRect
Expand All @@ -102,7 +110,9 @@ class MDCTabFoundation extends MDCFoundation {
this.adapter_.setAttr(strings.ARIA_SELECTED, 'true');
this.adapter_.setAttr(strings.TABINDEX, '0');
this.adapter_.activateIndicator(previousIndicatorClientRect);
this.adapter_.focus();
if (this.focusOnActivate_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be tempted to make this a parameter for the activate function that can be passed by the TabBar. Then, focusOnActivate won't need to be stored in each Tab instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

So TabBar would pass it's value for focusOnActivate to a Tab via the activate method. I realize this makes more changes to the Tab foundation interface. If you're trying to prevent breaking changes, we can roll with the existing solution. Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your idea is the first idea I had, but I thought against it for a couple of reasons:

  • It would make the activate API more awkward, given that it already has an optional ClientRect parameter
  • Semantically, it seems unlikely that you'd want to call this with different values across the same instance's lifetime; you'd probably just want to set the behavior once and roll with it

It wouldn't necessarily be a breaking change that way, since it could also be optional with a default.

Copy link
Contributor

Choose a reason for hiding this comment

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

True true, those are all good points.

this.adapter_.focus();
}
}

/**
Expand Down
4 changes: 4 additions & 0 deletions packages/mdc-tab/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ class MDCTab extends MDCComponent {
return this.foundation_.isActive();
}

set focusOnActivate(focusOnActivate) {
this.foundation_.setFocusOnActivate(focusOnActivate);
}

/**
* Activates the tab
* @param {!ClientRect=} computeIndicatorClientRect
Expand Down
29 changes: 29 additions & 0 deletions test/unit/mdc-tab-bar/mdc-tab-bar.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ class FakeTab {
this.computeIndicatorClientRect = td.function();
this.computeDimensions = td.function();
this.active = false;
this.focusOnActivate = true;
}
}

Expand Down Expand Up @@ -109,6 +110,34 @@ function setupTest() {
return {root, component};
}

function setupMockFoundationTest() {
const root = getFixture();
const MockFoundationConstructor = td.constructor(MDCTabBarFoundation);
const mockFoundation = new MockFoundationConstructor();
const component = new MDCTabBar(root, mockFoundation, (el) => new FakeTab(el), (el) => new FakeTabScroller(el));
return {root, component, mockFoundation};
}

test('focusOnActivate setter updates setting on all tabs', () => {
const {component} = setupTest();

component.focusOnActivate = false;
component.tabList_.forEach((tab) => assert.isFalse(tab.focusOnActivate));

component.focusOnActivate = true;
component.tabList_.forEach((tab) => assert.isTrue(tab.focusOnActivate));
});

test('useAutomaticActivation setter calls foundation#setUseAutomaticActivation', () => {
const {component, mockFoundation} = setupMockFoundationTest();

component.useAutomaticActivation = false;
td.verify(mockFoundation.setUseAutomaticActivation(false));

component.useAutomaticActivation = true;
td.verify(mockFoundation.setUseAutomaticActivation(true));
});

test('#adapter.scrollTo calls scrollTo of the child tab scroller', () => {
const {component} = setupTest();
component.getDefaultFoundation().adapter_.scrollTo(123);
Expand Down
16 changes: 15 additions & 1 deletion test/unit/mdc-tab/foundation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,26 @@ test('#activate activates the indicator', () => {
td.verify(mockAdapter.activateIndicator({width: 100, left: 200}));
});

test('#activate focuses the root node', () => {
test('#activate focuses the root node by default', () => {
const {foundation, mockAdapter} = setupTest();
foundation.activate({width: 100, left: 200});
td.verify(mockAdapter.focus());
});

test('#activate focuses the root node if focusOnActivate is true', () => {
const {foundation, mockAdapter} = setupTest();
foundation.setFocusOnActivate(true);
foundation.activate({width: 100, left: 200});
td.verify(mockAdapter.focus());
});

test('#activate does not focus the root node if focusOnActivate is false', () => {
const {foundation, mockAdapter} = setupTest();
foundation.setFocusOnActivate(false);
foundation.activate({width: 100, left: 200});
td.verify(mockAdapter.focus(), {times: 0});
});

test('#deactivate does nothing if not active', () => {
const {foundation, mockAdapter} = setupTest();
foundation.deactivate();
Expand Down
8 changes: 7 additions & 1 deletion test/unit/mdc-tab/mdc-tab.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,12 +165,18 @@ function setupMockFoundationTest(root = getFixture()) {
return {root, component, mockFoundation};
}

test('#active getter calls isActive', () => {
test('#active getter calls foundation.isActive', () => {
const {component, mockFoundation} = setupMockFoundationTest();
component.active;
td.verify(mockFoundation.isActive(), {times: 1});
});

test('#focusOnActivate setter calls foundation.setFocusOnActivate', () => {
const {component, mockFoundation} = setupMockFoundationTest();
component.focusOnActivate = false;
td.verify(mockFoundation.setFocusOnActivate(false), {times: 1});
});

test('#activate() calls activate', () => {
const {component, mockFoundation} = setupMockFoundationTest();
component.activate();
Expand Down