Skip to content

ComboBox/SplitButton Expand on Touch #4353

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
chang47 merged 48 commits into from
May 2, 2018
Merged

ComboBox/SplitButton Expand on Touch #4353

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
07b16f9
Add autoexpand on touch to align with the desired behavior
jspurlin Mar 12, 2018
3502f3a
update to use pointer events for comboBox and splitButton
jspurlin Mar 14, 2018
1deccdd
update snapshot
jspurlin Mar 14, 2018
4f36e14
remove unused variable
jspurlin Mar 14, 2018
06806a1
Add preventDefault and stopImmediatePropagation so that IE/Edge will …
jspurlin Mar 14, 2018
e6e0276
test change
Mar 22, 2018
78ee211
removed test push
Mar 22, 2018
aea2641
resolved merge conflict
Mar 23, 2018
020ed81
added change file
Mar 23, 2018
c60caca
update bundle size
Mar 23, 2018
8b888d8
temp combo box changes
Mar 27, 2018
fbb8857
added touch start event and added test cases
Mar 30, 2018
4c5720e
updated tests with else case
Apr 3, 2018
90c6ac1
resolved merge
Apr 3, 2018
b8b07c9
removed unneeded comment
Apr 3, 2018
fab1029
updated change list
Apr 3, 2018
0293d75
updated code to deal with async race conditions
Apr 3, 2018
fa7671e
removed stopping evnet handling that stops firefox from opening the g…
Apr 4, 2018
c284b76
cleaned up the code; made sure to cancel actions when they're finished
Apr 4, 2018
d6be2f4
fixed minor nitpick on style
Apr 4, 2018
30a2933
added if case to deal with touch event if there is no onclick; remove…
Apr 4, 2018
1fe7db7
removed unnecessary imports to reduce file size
Apr 5, 2018
ad563fe
fixed build problems; added constants
Apr 5, 2018
1702de4
changed readonly to const
Apr 5, 2018
f20265a
solved build problem with string types
Apr 5, 2018
ec0bd53
fixed missing const that's causing build break with defined type
Apr 5, 2018
51e730f
resolved merge
Apr 9, 2018
f8fea3b
resolved merge; moved reference of splitbutton container to component…
chang47 Apr 11, 2018
dd096d8
Merge https://github.com/OfficeDev/office-ui-fabric-react into jspurl…
chang47 Apr 11, 2018
acce342
added comment for time out
chang47 Apr 11, 2018
dbaa322
increased max size limit of bundle
chang47 Apr 11, 2018
e3ebdf7
Merge branch 'master' into jspurlin/ComboBoxExpandOnTouch
dzearing Apr 12, 2018
e57e6e9
resolve merge conflict
Apr 12, 2018
dd13382
updated contextual menu to address comments
Apr 12, 2018
089233c
added issue regarding dynamically generating a primary action into th…
Apr 12, 2018
a94b7c5
increased the size limit
Apr 12, 2018
71ade9a
updated with better comments and grammatical fixes
Apr 12, 2018
b68bf62
pulled latest from fabric
chang47 Apr 30, 2018
16a88a7
fixed edge async menu close bug
chang47 Apr 30, 2018
cec47d0
update test
chang47 Apr 30, 2018
6d3810e
bumped bundle size
chang47 Apr 30, 2018
1f9fd86
fixed gammar and moved some code around
chang47 Apr 30, 2018
8ce9bc7
made better callback name
chang47 Apr 30, 2018
9b45fef
added pointer and touch event
chang47 Apr 30, 2018
fb63fb8
fixed semicolon
chang47 Apr 30, 2018
e7d0229
updated bundle size
May 1, 2018
9d247be
resovled merge
May 2, 2018
d3b1d13
fixed bundle size to include keytip change
May 2, 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-ComboBoxExpandOnTouch_2018-03-23-22-33.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": "Added ontouch support to expand menus for split buttons and combo boxes",

@dzearing dzearing Mar 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SplitButton/ComboBox: added onTouch support for menu expansion.

@chang47 chang47 Apr 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated!

"type": "minor"
}
],
"packageName": "office-ui-fabric-react",
"email": "chiechan@microsoft.com"
}
37 changes: 34 additions & 3 deletions packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
private _descriptionId: string;
private _ariaDescriptionId: string;
private _classNames: IButtonClassNames;
private _processingTouch: boolean;

constructor(props: IBaseButtonProps, rootClassName: string) {
super(props);
Expand Down Expand Up @@ -192,13 +193,23 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
return this._onRenderContent(tag, buttonProps);
}

public componentDidMount() {
if (this._isSplitButton && this._splitButtonContainer) {
this._events.on(this._splitButtonContainer.value, 'pointerdown', this._onPointerDown, true);

@dzearing dzearing Mar 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can't you use react eventing? You should avoid using native events when possible.

@joschect joschect Mar 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

React does not support this yet, you could potentially mix it in with a ... but you won't get type safety. Based on this issue react/react#499 it looks like there was some disagreement within the react community as to whether or not pointer events should get added since they still aren't supported by IOS but someone could probably add that now.

}
}

public componentDidUpdate(prevProps: IBaseButtonProps, prevState: IBaseButtonState) {
// If Button's menu was closed, run onAfterMenuDismiss
if (this.props.onAfterMenuDismiss && prevState.menuProps && !this.state.menuProps) {
this.props.onAfterMenuDismiss();
}
}

public componentWillUnmount() {

@dzearing dzearing Mar 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why?

super.componentWillUnmount();
}

public focus(): void {
if (this._isSplitButton && this._splitButtonContainer.value) {
this._splitButtonContainer.value.focus();
Expand Down Expand Up @@ -425,7 +436,6 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
disabled,
checked,
getSplitButtonClassNames,
onClick,
primaryDisabled
} = this.props;

Expand Down Expand Up @@ -460,7 +470,7 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
onKeyDown={ this._onSplitButtonContainerKeyDown }
ref={ this._splitButtonContainer }
data-is-focusable={ true }
onClick={ !disabled && !primaryDisabled ? onClick : undefined }
onClick={ !disabled && !primaryDisabled ? this._onSplitButtonClick : undefined }
tabIndex={ !disabled ? 0 : undefined }
>
<span
Expand Down Expand Up @@ -511,6 +521,14 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
return <BaseButton {...splitButtonProps} onMouseDown={ this._onMouseDown } tabIndex={ -1 } />;
}

private _onSplitButtonClick = (ev: React.MouseEvent<HTMLDivElement | HTMLAnchorElement>) => {

@joschect joschect Apr 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This method isn't used anywhere, how is it getting called?

@chang47 chang47 Apr 5, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, it's not this was old code that came from a merge conflict. I'll get rid of it.

if (!this._processingTouch && this.props.onClick) {
this.props.onClick(ev);
} else {
this._onMenuClick(ev);
}
}

private _onMouseDown = (ev: React.MouseEvent<BaseButton>) => {
if (this.props.onMouseDown) {
this.props.onMouseDown(ev);
Expand Down Expand Up @@ -554,6 +572,19 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
}
}

private _onPointerDown(ev: PointerEvent) {
if (ev.pointerType === 'touch') {
this._processingTouch = true;

ev.preventDefault();
ev.stopImmediatePropagation();

this._async.setTimeout(() => {
this._processingTouch = false;
}, 500);
}
}

/**
* Returns if the user hits a valid keyboard key to open the menu
* @param ev - the keyboard event
Expand All @@ -567,7 +598,7 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
}
}

private _onMenuClick = (ev: React.MouseEvent<HTMLAnchorElement>) => {
private _onMenuClick = (ev: React.MouseEvent<HTMLDivElement | HTMLAnchorElement>) => {
const { onMenuClick } = this.props;
if (onMenuClick) {
onMenuClick(ev, this);
Expand Down
98 changes: 58 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 @@ -129,6 +129,8 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {

private _scrollIdleTimeoutId: number | undefined;

private _processingTouch: boolean;

// Determines if we should be setting
// focus back to the input when the menu closes.
// The general rule of thumb is if the menu was launched
Expand All @@ -152,6 +154,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
const selectedKeys: (string | number)[] = this._getSelectedKeys(props.defaultSelectedKey, props.selectedKey);

this._isScrollIdle = true;
this._processingTouch = false;

const initialSelectedIndices: number[] = this._getSelectedIndices(props.options, selectedKeys);

Expand All @@ -170,6 +173,8 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
public componentDidMount() {
// hook up resolving the options if needed on focus
this._events.on(this._comboBoxWrapper.value, 'focus', this._onResolveOptions, true);

@jspurlin jspurlin Apr 4, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you mean to remove this line?


this._events.on(this._comboBoxWrapper.value, 'pointerdown', this._onPointerDown, true);
}

public componentWillReceiveProps(newProps: IComboBoxProps) {
Expand Down Expand Up @@ -346,8 +351,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 +364,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
/>
</div>

{isOpen && (
{ isOpen && (
(onRenderContainer as any)({
...this.props,
onRenderList,
Expand All @@ -368,13 +373,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 +486,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 +938,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 +957,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 +972,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 +1007,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 +1044,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={ this._selectedElement }>
{ 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 +1192,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 @@ -1650,12 +1655,25 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
*/
private _onAutofillClick = (): void => {
if (this.props.allowFreeform) {
this.focus(this.state.isOpen);
this.focus(this.state.isOpen || this._processingTouch);
} else {
this._onComboBoxClick();
}
}

private _onPointerDown = (ev: PointerEvent): void => {
if (ev.pointerType === 'touch') {
this._processingTouch = true;

ev.preventDefault();
ev.stopImmediatePropagation();

this._async.setTimeout(() => {
this._processingTouch = false;
}, 500);
}
}

/**
* Get the styles for the current option.
* @param item Item props for the current option
Expand Down
30 changes: 24 additions & 6 deletions packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
private _isScrollIdle: boolean;
private readonly _navigationIdleDelay: number = 250 /* ms */;

@dzearing dzearing Apr 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should just use const at the top of the file for constant numbers.

/** Comment explaining what its used for. */
const NavigationIdleDelay = 250;

private _scrollIdleTimeoutId: number | undefined;
private _processingTouch: boolean;

private _splitButtonContainers: Map<string, HTMLDivElement>;

Expand Down Expand Up @@ -149,6 +150,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext

// Invoked immediately before a component is unmounted from the DOM.
public componentWillUnmount() {
super.componentWillUnmount();

@dzearing dzearing Apr 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmmm. This feels unneeded?

if (this._isFocusingPreviousElement && this._previousActiveElement) {

// This slight delay is required so that we can unwind the stack, const react try to mess with focus, and then
Expand All @@ -160,9 +162,6 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
if (this.props.onMenuDismissed) {
this.props.onMenuDismissed(this.props);
}

this._events.dispose();
this._async.dispose();
}

public render(): JSX.Element | null {
Expand Down Expand Up @@ -592,8 +591,10 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext

return (
<div
ref={ (el: HTMLDivElement) =>
this._splitButtonContainers.set(item.key, el)
ref={ (el: HTMLDivElement) => {
this._splitButtonContainers.set(item.key, el);
el && this._events.on(el, 'pointerdown', this._onPointerDown, true);
}
}
role={ 'button' }
aria-labelledby={ item.ariaLabel }
Expand Down Expand Up @@ -705,6 +706,19 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
}
}

private _onPointerDown(ev: PointerEvent) {
if (ev.pointerType === 'touch') {
this._processingTouch = true;

ev.preventDefault();
ev.stopImmediatePropagation();

this._async.setTimeout(() => {
this._processingTouch = false;
}, 500);
}
}

/**
* Checks if the submenu should be closed
*/
Expand Down Expand Up @@ -849,7 +863,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
}
}

private _onItemClick(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>) {
private _onItemClick(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>) {
const items = getSubmenuItems(item);

if (!hasSubmenu(item) && (!items || !items.length)) { // This is an item without a menu. Click it.
Expand All @@ -875,6 +889,10 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
if (item.disabled || item.isDisabled) {
return;
}
if (this._processingTouch) {
return this._onItemClick(item, ev);

@dzearing dzearing Mar 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you add unit tests to validate the things you're changing?

@chang47 chang47 Mar 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do!

@chang47 chang47 Mar 30, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added unit tests, with some caveats if you look at another one of my comments.

}

let dismiss = false;
if (item.onClick) {
dismiss = !!item.onClick(ev, item);
Expand Down