Skip to content

ComboBox: Make comboBox menus wrap #4482

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

Merged
jspurlin merged 6 commits into from
Apr 7, 2018
Merged

ComboBox: Make comboBox menus wrap #4482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8f5fe0a
Make comboBoxes wrap per the ATHEN feedback
jspurlin Apr 5, 2018
1dc89c8
add unit tests and account for a non-focusable element at the beginni…
jspurlin Apr 6, 2018
2f3cd65
rush change
jspurlin Apr 6, 2018
1d7fc6f
update test comments
jspurlin Apr 6, 2018
40578e3
Merge branch 'master' of https://github.com/jspurlin/office-ui-fabric…
jspurlin Apr 7, 2018
19eda4d
Add a test to check the wrapping with non-focusable content above
jspurlin Apr 7, 2018
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
11 changes: 11 additions & 0 deletions common/changes/office-ui-fabric-react/jspurlin-MakeComboBoxesWrap_2018-04-06-19-49.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "ComboBox: Make focus wrap in in the menu (to align with dropdwn and customer feedback)_",
"type": "patch"
}
],
"packageName": "office-ui-fabric-react",
"email": "jspurlin@microsoft.com"
}
43 changes: 43 additions & 0 deletions packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { KeyCodes } from '../../Utilities';

import { ComboBox } from './ComboBox';
import { IComboBox, IComboBoxOption } from './ComboBox.types';
import { SelectableOptionMenuItemType } from '../../utilities/selectableOption/SelectableOption.types';
import { expectOne, expectMissing } from '../../common/testUtilities';

const DEFAULT_OPTIONS: IComboBoxOption[] = [
Expand All @@ -22,6 +23,12 @@ const DEFAULT_OPTIONS2: IComboBoxOption[] = [
{ key: '2', text: 'Foo' },
{ key: '3', text: 'Bar' }
];
const DEFAULT_OPTIONS3: IComboBoxOption[] = [
{ key: '0', text: 'Zero', itemType: SelectableOptionMenuItemType.Header },
{ key: '1', text: 'One' },
{ key: '2', text: 'Foo' },
{ key: '3', text: 'Bar' }
];

describe('ComboBox', () => {
it('Renders ComboBox correctly', () => {
Expand Down Expand Up @@ -268,6 +275,42 @@ describe('ComboBox', () => {
expect(wrapper.find('input').props().value).toEqual('Foo');
});

it('Can change selected option with keyboard, looping from top to bottom', () => {
const wrapper = mount(
<ComboBox
label='testgroup'
defaultSelectedKey='1'
options={ DEFAULT_OPTIONS2 }
/>);
wrapper.find('input').simulate('keydown', { which: KeyCodes.up });
wrapper.update();
expect(wrapper.find('input').props().value).toEqual('Bar');
});

it('Can change selected option with keyboard, looping from bottom to top', () => {
const wrapper = mount(
<ComboBox
label='testgroup'
defaultSelectedKey='3'
options={ DEFAULT_OPTIONS2 }
/>);
wrapper.find('input').simulate('keydown', { which: KeyCodes.down });
wrapper.update();
expect(wrapper.find('input').props().value).toEqual('One');
});

it('Can change selected option with keyboard, looping from top to bottom', () => {
const wrapper = mount(
<ComboBox
label='testgroup'
defaultSelectedKey='1'
options={ DEFAULT_OPTIONS3 }
/>);
wrapper.find('input').simulate('keydown', { which: KeyCodes.up });
wrapper.update();
expect(wrapper.find('input').props().value).toEqual('Bar');
});

it('Cannot insert text while disabled', () => {
const wrapper = mount(
<ComboBox
Expand Down
103 changes: 63 additions & 40 deletions packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,8 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
title={ title }
/>
<IconButton
className={'ms-ComboBox-CaretDown-button'}
styles={this._getCaretButtonStyles()}
className={ 'ms-ComboBox-CaretDown-button' }
styles={ this._getCaretButtonStyles() }
role='presentation'
aria-hidden={ isButtonAriaHidden }
data-is-focusable={ false }
Expand All @@ -359,7 +359,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
/>
</div>

{isOpen && (
{ isOpen && (
(onRenderContainer as any)({
...this.props,
onRenderList,
Expand All @@ -368,13 +368,13 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
options: this.state.currentOptions.map((item, index) => ({ ...item, index: index }))
},
this._onRenderContainer)
)}
) }
{
errorMessage &&
<div
className={this._classNames.errorMessage}
className={ this._classNames.errorMessage }
>
{errorMessage}
{ errorMessage }
</div>
}
</div>
Expand Down Expand Up @@ -481,7 +481,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
}
displayValues.push(currentPendingValue !== '' ? currentPendingValue : (this._indexWithinBounds(currentOptions, index) ? currentOptions[index].text : ''));
} else {
for (let idx = 0; selectedIndices && (idx < selectedIndices.length); idx ++) {
for (let idx = 0; selectedIndices && (idx < selectedIndices.length); idx++) {
const index: number = selectedIndices[idx];
displayValues.push(this._indexWithinBounds(currentOptions, index) ? currentOptions[index].text : suggestedDisplayValue);
}
Expand Down Expand Up @@ -933,11 +933,11 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {

return (
<Callout
isBeakVisible={false}
gapSpace={0}
doNotLayer={false}
directionalHint={DirectionalHint.bottomLeftEdge}
directionalHintFixed={true}
isBeakVisible={ false }
gapSpace={ 0 }
doNotLayer={ false }
directionalHint={ DirectionalHint.bottomLeftEdge }
directionalHintFixed={ true }
{ ...calloutProps }
className={ css(this._classNames.callout, calloutProps ? calloutProps.className : undefined) }
target={ this._comboBoxWrapper.value }
Expand All @@ -952,7 +952,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
<div className={ this._classNames.optionsContainerWrapper } ref={ this._comboBoxMenu }>
{ (onRenderList as any)({ ...props }, this._onRenderList) }
</div>
{onRenderLowerContent(this.props, this._onRenderLowerContent)}
{ onRenderLowerContent(this.props, this._onRenderLowerContent) }
</Callout>
);
}
Expand All @@ -967,12 +967,12 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
const id = this._id;
return (
<div
id={id + '-list'}
className={this._classNames.optionsContainer}
aria-labelledby={id + '-label'}
id={ id + '-list' }
className={ this._classNames.optionsContainer }
aria-labelledby={ id + '-label' }
role='listbox'
>
{options.map((item) => (onRenderItem as any)(item, this._onRenderItem))}
{ options.map((item) => (onRenderItem as any)(item, this._onRenderItem)) }
</div>
);
}
Expand Down Expand Up @@ -1002,8 +1002,8 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
return (
<div
role='separator'
key={key}
className={this._classNames.divider}
key={ key }
className={ this._classNames.divider }
/>
);
}
Expand Down Expand Up @@ -1039,32 +1039,32 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
onMouseMove={ this._onOptionMouseMove.bind(this, item.index) }
onMouseLeave={ this._onOptionMouseLeave }
role='option'
aria-selected={ isSelected ? 'true' : 'false'}
ariaLabel= {item.text }
aria-selected={ isSelected ? 'true' : 'false' }
ariaLabel={ item.text }
disabled={ item.disabled }
> { <span ref={ isSelected ? this._selectedElement : undefined }>
{ onRenderOption(item, this._onRenderOptionContent) }
</span>
}
</CommandButton>
) : (
<Checkbox
id={id + '-list' + item.index}
ref={'option' + item.index}
key={item.key}
data-index={item.index}
styles={optionStyles}
className={'ms-ComboBox-option'}
data-is-focusable={true}
onChange={this._onItemClick(item.index!)}
label={item.text}
role='option'
aria-selected={ isSelected ? 'true' : 'false' }
checked={isSelected}
>
{onRenderOption(item, this._onRenderOptionContent)}
</Checkbox>
)
<Checkbox
id={ id + '-list' + item.index }
ref={ 'option' + item.index }
key={ item.key }
data-index={ item.index }
styles={ optionStyles }
className={ 'ms-ComboBox-option' }
data-is-focusable={ true }
onChange={ this._onItemClick(item.index!) }
label={ item.text }
role='option'
aria-selected={ isSelected ? 'true' : 'false' }
checked={ isSelected }
>
{ onRenderOption(item, this._onRenderOptionContent) }
</Checkbox>
)
);
}

Expand Down Expand Up @@ -1187,7 +1187,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {

private _onRenderOptionContent = (item: IComboBoxOption): JSX.Element => {
const optionClassNames = getComboBoxOptionClassNames(this._getCurrentOptionStyles(item));
return <span className={optionClassNames.optionText}>{item.text}</span>;
return <span className={ optionClassNames.optionText }>{ item.text }</span>;
}

/**
Expand Down Expand Up @@ -1325,7 +1325,30 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
currentOptions
} = this.state;

index = this._getNextSelectableIndex(index, searchDirection);
// update index to allow content to wrap
if (searchDirection === SearchDirection.forward && index >= currentOptions.length - 1) {
index = -1;
} else if (searchDirection === SearchDirection.backward && index <= 0) {
index = currentOptions.length;
}

// get the next "valid" index
const indexUpdate = this._getNextSelectableIndex(index, searchDirection);

// if the two indicies are equal we didn't move and
// we should attempt to get get the first/last "valid" index to use
// (Note, this takes care of the potential cases where the first/last
// item is not focusable), otherwise use the updated index
if (index === indexUpdate) {
if (searchDirection === SearchDirection.forward) {
index = this._getNextSelectableIndex(-1, searchDirection);
Copy link
Copy Markdown
Member

@christiango christiango Apr 6, 2018

Choose a reason for hiding this comment

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

I'm not super familiar with this codepath, does the unit test cover the code in the if(index == indexUpdate ) branch?

Copy link
Copy Markdown
Contributor Author

@jspurlin jspurlin Apr 7, 2018

Choose a reason for hiding this comment

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

Good call. I've added another test to exercise this codepath

} else if (searchDirection === SearchDirection.backward) {
index = this._getNextSelectableIndex(currentOptions.length, searchDirection);
}
} else {
index = indexUpdate;
}

if (this._indexWithinBounds(currentOptions, index)) {
this._setPendingInfoFromIndex(index);
}
Expand Down