-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
Changes from 35 commits
07b16f9
3502f3a
1deccdd
4f36e14
06806a1
e6e0276
78ee211
aea2641
020ed81
c60caca
8b888d8
fbb8857
4c5720e
90c6ac1
b8b07c9
fab1029
0293d75
fa7671e
c284b76
d6be2f4
30a2933
1fe7db7
ad563fe
1702de4
f20265a
ec0bd53
51e730f
f8fea3b
dd096d8
acce342
dbaa322
e3ebdf7
e57e6e9
dd13382
089233c
a94b7c5
71ade9a
b68bf62
16a88a7
cec47d0
6d3810e
1f9fd86
8ce9bc7
9b45fef
fb63fb8
e7d0229
9d247be
d3b1d13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "changes": [ | ||
| { | ||
| "packageName": "office-ui-fabric-react", | ||
| "comment": "SplitButton/ComboBox: added onTouch support for menu expansion.", | ||
| "type": "minor" | ||
| } | ||
| ], | ||
| "packageName": "office-ui-fabric-react", | ||
| "email": "[email protected]" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,8 @@ export interface IBaseButtonState { | |
| menuProps?: IContextualMenuProps | null; | ||
| } | ||
|
|
||
| const TouchIdleDelay = 500; /* ms */ | ||
|
|
||
| export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState> implements IButton { | ||
|
|
||
| private get _isSplitButton(): boolean { | ||
|
|
@@ -49,6 +51,8 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
| private _descriptionId: string; | ||
| private _ariaDescriptionId: string; | ||
| private _classNames: IButtonClassNames; | ||
| private _processingTouch: boolean; | ||
| private _lastTouchTimeoutId: number | undefined; | ||
|
|
||
| constructor(props: IBaseButtonProps, rootClassName: string) { | ||
| super(props); | ||
|
|
@@ -192,6 +196,12 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
| return this._onRenderContent(tag, buttonProps); | ||
| } | ||
|
|
||
| public componentDidMount() { | ||
| if (this._isSplitButton && this._splitButtonContainer.value && 'onpointerdown' in this._splitButtonContainer.value) { | ||
| this._events.on(this._splitButtonContainer.value, 'pointerdown', this._onPointerDown, true); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
| } | ||
|
|
||
| public componentDidUpdate(prevProps: IBaseButtonProps, prevState: IBaseButtonState) { | ||
| // If Button's menu was closed, run onAfterMenuDismiss | ||
| if (this.props.onAfterMenuDismiss && prevState.menuProps && !this.state.menuProps) { | ||
|
|
@@ -425,7 +435,6 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
| disabled, | ||
| checked, | ||
| getSplitButtonClassNames, | ||
| onClick, | ||
| primaryDisabled | ||
| } = this.props; | ||
|
|
||
|
|
@@ -458,6 +467,7 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
| aria-describedby={ buttonProps.ariaDescription } | ||
| className={ classNames && classNames.splitButtonContainer } | ||
| onKeyDown={ this._onSplitButtonContainerKeyDown } | ||
| onTouchStart={ this._onTouchStart } | ||
| ref={ this._splitButtonContainer } | ||
| data-is-focusable={ true } | ||
| onClick={ !disabled && !primaryDisabled ? this._onSplitButtonPrimaryClick : undefined } | ||
|
|
@@ -478,8 +488,11 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
| if (this._isExpanded) { | ||
| this._dismissMenu(); | ||
| } | ||
| if (this.props.onClick) { | ||
|
|
||
| if (!this._processingTouch && this.props.onClick) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now if we are not
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're correct, I've changed it to an else if to only do a menu click if we're processingTouch |
||
| this.props.onClick(ev); | ||
| } else if (this._processingTouch) { | ||
| this._onMenuClick(ev); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -563,6 +576,36 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
| } | ||
| } | ||
|
|
||
| private _onTouchStart: () => void = () => { | ||
| if (this._isSplitButton && this._splitButtonContainer.value && !('onpointerdown' in this._splitButtonContainer.value)) { | ||
| this._handleTouchAndPointerEvent(); | ||
| } | ||
| } | ||
|
|
||
| private _onPointerDown(ev: PointerEvent) { | ||
| if (ev.pointerType === 'touch') { | ||
| this._handleTouchAndPointerEvent(); | ||
|
|
||
| ev.preventDefault(); | ||
| ev.stopImmediatePropagation(); | ||
| } | ||
| } | ||
|
|
||
| private _handleTouchAndPointerEvent() { | ||
| // If we already have an existing timeeout from a previous touch and pointer event | ||
| // cancel that timeout so we can set a nwe one. | ||
| if (this._lastTouchTimeoutId !== undefined) { | ||
| this._async.clearTimeout(this._lastTouchTimeoutId); | ||
| this._lastTouchTimeoutId = undefined; | ||
| } | ||
| this._processingTouch = true; | ||
|
|
||
| this._lastTouchTimeoutId = this._async.setTimeout(() => { | ||
| this._processingTouch = false; | ||
| this._lastTouchTimeoutId = undefined; | ||
| }, TouchIdleDelay); | ||
| } | ||
|
|
||
| /** | ||
| * Returns if the user hits a valid keyboard key to open the menu | ||
| * @param ev - the keyboard event | ||
|
|
@@ -576,7 +619,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); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,13 @@ enum HoverStatus { | |
| default = -1 | ||
| } | ||
|
|
||
| const ScrollIdleDelay = 250 /* ms */; | ||
| const TouchIdleDelay = 500; /* ms */ | ||
|
|
||
| // This is used to clear any pending autocomplete | ||
| // text (used when autocomplete is true and allowFreeform is false) | ||
| const ReadOnlyPendingAutoCompleteTimeout = 1000 /* ms */; | ||
|
|
||
| @customizable('ComboBox', ['theme']) | ||
| export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> { | ||
|
|
||
|
|
@@ -104,10 +111,6 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> { | |
| // The base id for the comboBox | ||
| private _id: string; | ||
|
|
||
| // This is used to clear any pending autocomplete | ||
| // text (used when autocomplete is true and allowFreeform is false) | ||
| private readonly _readOnlyPendingAutoCompleteTimeout: number = 1000 /* ms */; | ||
|
|
||
| // After a character is inserted when autocomplete is true and | ||
| // allowFreeform is false, remember the task that will clear | ||
| // the pending string of characters | ||
|
|
@@ -125,10 +128,12 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> { | |
|
|
||
| private _hasPendingValue: boolean; | ||
|
|
||
| private readonly _scrollIdleDelay: number = 250 /* ms */; | ||
|
|
||
| private _scrollIdleTimeoutId: number | undefined; | ||
|
|
||
| private _processingTouch: boolean; | ||
|
|
||
| private _lastTouchTimeoutId: number | undefined; | ||
|
|
||
| // 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 | ||
|
|
@@ -152,6 +157,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); | ||
|
|
||
|
|
@@ -168,8 +174,9 @@ 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); | ||
|
||
| if (this._comboBoxWrapper.value && 'onpointerdown' in this._comboBoxWrapper.value) { | ||
| this._events.on(this._comboBoxWrapper.value, 'pointerdown', this._onPointerDown, true); | ||
| } | ||
| } | ||
|
|
||
| public componentWillReceiveProps(newProps: IComboBoxProps) { | ||
|
|
@@ -326,6 +333,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> { | |
| onKeyDown={ this._onInputKeyDown } | ||
| onKeyUp={ this._onInputKeyUp } | ||
| onClick={ this._onAutofillClick } | ||
| onTouchStart={ this._onTouchStart } | ||
| onInputValueChange={ this._onInputChange } | ||
| aria-expanded={ isOpen } | ||
| aria-autocomplete={ this._getAriaAutoCompleteValue() } | ||
|
|
@@ -653,7 +661,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> { | |
| this._lastReadOnlyAutoCompleteChangeTimeoutId = | ||
| this._async.setTimeout( | ||
| () => { this._lastReadOnlyAutoCompleteChangeTimeoutId = undefined; }, | ||
| this._readOnlyPendingAutoCompleteTimeout | ||
| ReadOnlyPendingAutoCompleteTimeout | ||
| ); | ||
| return; | ||
| } | ||
|
|
@@ -1136,7 +1144,7 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> { | |
| this._isScrollIdle = false; | ||
| } | ||
|
|
||
| this._scrollIdleTimeoutId = this._async.setTimeout(() => { this._isScrollIdle = true; }, this._scrollIdleDelay); | ||
| this._scrollIdleTimeoutId = this._async.setTimeout(() => { this._isScrollIdle = true; }, ScrollIdleDelay); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1678,12 +1686,42 @@ 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 _onTouchStart: () => void = () => { | ||
| if (this._comboBoxWrapper.value && !('onpointerdown' in this._comboBoxWrapper)) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dzearing @joschect I had to add the onTouchStart event with an if check like this for split buttons, contextual menu split buttons, and combo boxes to support unit testing, because if I were to try and do something similar with addEventListener instead of adding this callback directly to a React component, the callback would never get called and it would not be unit testable. Specifically, what I'm doing is: if onPointerDown is supported, than we don't do anything with onTouchStart and later in the componentDidMount, we'll add the pointerdown event, otherwise, we'll support onTouchStart and not add the pointerdown event. This would be a temporary fix until React adds support for onPointerDown, but what are your thoughts on doing something like this? I was not able to think of a better solution. Thanks!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. I added other comments. let me know when you've resolved. |
||
| this._handleTouchAndPointerEvent(); | ||
| } | ||
| } | ||
|
|
||
| private _onPointerDown = (ev: PointerEvent): void => { | ||
| if (ev.pointerType === 'touch') { | ||
| this._handleTouchAndPointerEvent(); | ||
|
|
||
| ev.preventDefault(); | ||
| ev.stopImmediatePropagation(); | ||
| } | ||
| } | ||
|
|
||
| private _handleTouchAndPointerEvent() { | ||
| // If we already have an existing timeeout from a previous touch and pointer event | ||
| // cancel that timeout so we can set a nwe one. | ||
| if (this._lastTouchTimeoutId !== undefined) { | ||
| this._async.clearTimeout(this._lastTouchTimeoutId); | ||
| this._lastTouchTimeoutId = undefined; | ||
| } | ||
| this._processingTouch = true; | ||
|
|
||
| this._lastTouchTimeoutId = this._async.setTimeout(() => { | ||
| this._processingTouch = false; | ||
| this._lastTouchTimeoutId = undefined; | ||
| }, TouchIdleDelay); | ||
| } | ||
|
|
||
| /** | ||
| * Get the styles for the current option. | ||
| * @param item Item props for the current option | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps leave a comment here on why! That way the person re-writing the button later can understand your motivation.