ComboBox/SplitButton Expand on Touch #4353

chang47 · 2018-03-23T22:31:09Z

Problem:
Currently, we don't have correct behavior for touch events for ComboBoxes and Splitbuttons.

Specifically:
For ComboBoxes when clicking on the input, we would select the text instead of expanding on the menu.

For SplitButtons, the menu button is too small and is hard to click to expand the menu.

Solution:
Add touch support so that when we click on the input box on the ComboBox, we would open the menu.

For the Base button, we added support for click so now that if we were to click on the split button we will always open the menu. This is fine, because the split button primary action should always be an option in the menu.

Validation:
I used a laptop with a touch screen and an iPad to test that we can now:

expand the combo boxes when we touch the input box.
open split button menus when we touch the split button container.

in Firefox, IE, Edge, Chrome. Safari

…behave the same as other browsers (especially for comboBox)

chang47 · 2018-03-23T22:34:01Z

@jspurlin

jspurlin · 2018-03-23T23:09:36Z

@chang47 the bundle size needs to be updated

dzearing · 2018-03-24T01:10:20Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

      return;
    }
+    if (this._processingTouch) {
+      return this._onItemClick(item, ev);


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

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

joschect

It looks like pointer events are not yet supported by IOS. Maybe touch events should be used instead? https://caniuse.com/#search=pointer

Also since react currently doesn't support pointerevents it might make sense to go with touchstart as well.

jspurlin · 2018-03-26T17:55:39Z

If anything we should fallback to touch events, but pointer events are standardized (since 2015), are supported practically everywhere, and we shouldn't implement this based off of outdated technology/APIs (e.g. touch events).

I also don't think the fact that react doesn't support pointerevents natively is a valid reason for not using pointer events.

dzearing · 2018-03-26T20:37:17Z

packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx

    }
  }

+  public componentWillUnmount() {


dzearing · 2018-03-26T20:37:49Z

packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx


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


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

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 facebook/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.

dzearing · 2018-03-27T19:25:31Z

common/changes/office-ui-fabric-react/jspurlin-ComboBoxExpandOnTouch_2018-03-23-22-33.json

+  "changes": [
+    {
+      "packageName": "office-ui-fabric-react",
+      "comment": "Added ontouch support to expand menus for split buttons and combo boxes",


SplitButton/ComboBox: added onTouch support for menu expansion.

jspurlin · 2018-03-30T15:35:15Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

        aria-setsize={ totalItemCount }
        onKeyDown={ this._onSplitContainerItemKeyDown.bind(this, item) }
        onClick={ this._executeItemClick.bind(this, item) }
+        onTouchStart={ this._onTouchStart.bind(this) }


We shouldn't need to bind this, you can use autobind on the function.

Also, why are we always setting this when we might also be setting onpointerdown? Couldn't this just be set in an else above if we know we can't use pointerdown... or is this always set just for automation?

From what I know from merging all the changes, we moved away from using autobind and returned to using this, but I'm not sure of the reason. @joschect do you have any insights of why we moved away from this?

You are correct with why I had to set it here. It's for automation, we can't simulate the onTouchStart event any other way.

Ah, good call. I didn't realize fabric moved away from autobind, looking at the changelog it states that you should use arrow functions instead of autobind so you could just do this._onTouchStart here (like onTouchStart={ this._onTouchStart }) and then in the function definition use an arrow function like private _onTouchStart = (ev: TouchEvent<HTMLElement>) => {

@chang47 we've moved away from autobind because it adds general code bloat and increases the size of functions

chang47 · 2018-03-30T15:39:20Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

  }

+  private _onTouchStart = (ev: TouchEvent<HTMLInputElement>): void => {
+    if (this._comboBoxWrapper.value && !('onpointerdown' in this._comboBoxWrapper)) {


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

Got it.

I added other comments. let me know when you've resolved.

dzearing

minor nits

jspurlin · 2018-04-12T21:25:01Z

@chang47 you need to increase the bundle size you are .26kb over the current max

Dismissing outdated lock on this PR

dzearing · 2018-04-12T21:57:05Z

packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx

  }

+  public componentDidMount() {
+    if (this._isSplitButton && this._splitButtonContainer.value && 'onpointerdown' in this._splitButtonContainer.value) {


Perhaps leave a comment here on why! That way the person re-writing the button later can understand your motivation.

// For split buttons, touching anywhere in the button should drop the dropdown, which should contain the primary action. This gives more hit target space for touch environments.

dzearing · 2018-04-12T21:58:43Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

+    // Due to restraints caused by React 16 where we no can no longer
+    // call the ref values of children component with their own render
+    // function before componentDidMount we have to use this setTimeout
+    // hack to have access to ref. The correct approacj to solve this


dzearing · 2018-04-12T21:58:55Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

+    // call the ref values of children component with their own render
+    // function before componentDidMount we have to use this setTimeout
+    // hack to have access to ref. The correct approacj to solve this
+    // problem is to create a comonent for the split button and then


"component"

dzearing · 2018-04-12T21:59:53Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

+    // function before componentDidMount we have to use this setTimeout
+    // hack to have access to ref. The correct approacj to solve this
+    // problem is to create a comonent for the split button and then
+    // have it add it's own event listener. This is logged on Issue 4522


its

This is logged in issue #4522

dzearing · 2018-04-12T22:02:55Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

+    this._async.setTimeout(() => {
+      if (this._splitButtonContainers) {
+        this._splitButtonContainers.forEach((value: HTMLDivElement) => {
+          this._events.on(value, 'pointerdown', this._onPointerDown, true);


hmm. In code earlier in this PR, you went to great lengths to validate that onPointerDown was "in" the value, and that the value was not falsey. Why are you not doing that here?

Also this feels REALLY gross. I think the lesson learned here is that the button should have been part of the item, not part of the menu...

What if the menu items change? Is this going to execute again? Are you going to miss registering new items? E.g. the items are lazy loaded.

chang47 · 2018-04-20T17:16:18Z

Created a separate PR to make the split button to be its own component so we can have better state management controls for touch: #4618

jspurlin · 2018-05-01T17:33:13Z

@chang47 you need to bump the bundle size, you are .03KB over the current max size

chang47 · 2018-05-01T17:49:55Z

@jspurlin will do, just let me do some final validations.

jspurlin and others added 8 commits March 12, 2018 16:49

Add autoexpand on touch to align with the desired behavior

07b16f9

update to use pointer events for comboBox and splitButton

3502f3a

update snapshot

1deccdd

remove unused variable

4f36e14

Add preventDefault and stopImmediatePropagation so that IE/Edge will …

06806a1

…behave the same as other browsers (especially for comboBox)

test change

e6e0276

removed test push

78ee211

resolved merge conflict

aea2641

chang47 requested a review from joschect as a code owner March 23, 2018 22:31

added change file

020ed81

jspurlin approved these changes Mar 23, 2018

View reviewed changes

update bundle size

c60caca

chang47 requested a review from dzearing as a code owner March 23, 2018 23:19

dzearing reviewed Mar 24, 2018

View reviewed changes

joschect reviewed Mar 26, 2018

View reviewed changes

dzearing reviewed Mar 26, 2018

View reviewed changes

packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx Outdated

}

}

public componentWillUnmount() {

Copy link

Member

dzearing Mar 26, 2018

Choose a reason for hiding this comment

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

why?

dzearing reviewed Mar 26, 2018

View reviewed changes

temp combo box changes

8b888d8

dzearing reviewed Mar 27, 2018

View reviewed changes

added touch start event and added test cases

fbb8857

jspurlin reviewed Mar 30, 2018

View reviewed changes

chang47 commented Mar 30, 2018

View reviewed changes

dzearing previously requested changes Apr 2, 2018

View reviewed changes

REDMOND\chiechan added 4 commits April 2, 2018 17:55

updated tests with else case

4c5720e

resolved merge

90c6ac1

removed unneeded comment

b8b07c9

updated change list

fab1029

increased the size limit

a94b7c5

dzearing reviewed Apr 12, 2018

View reviewed changes

updated with better comments and grammatical fixes

71ade9a

chang47 mentioned this pull request Apr 20, 2018

Contextual Menu: Moving Split Button to be its own component #4618

Merged

chang47 added 8 commits April 29, 2018 20:13

pulled latest from fabric

b68bf62

fixed edge async menu close bug

16a88a7

update test

cec47d0

bumped bundle size

6d3810e

fixed gammar and moved some code around

1f9fd86

made better callback name

8ce9bc7

added pointer and touch event

9b45fef

fixed semicolon

fb63fb8

dzearing closed this May 1, 2018

dzearing reopened this May 1, 2018

dzearing approved these changes May 1, 2018

View reviewed changes

REDMOND\chiechan added 3 commits May 1, 2018 13:30

updated bundle size

e7d0229

resovled merge

9d247be

fixed bundle size to include keytip change

d3b1d13

chang47 merged commit f526bfe into microsoft:master May 2, 2018

aftab-hassan mentioned this pull request Mar 7, 2019

SplitButton in Touch doesn't work correctly #8230

Closed

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

ComboBox/SplitButton Expand on Touch #4353

ComboBox/SplitButton Expand on Touch #4353

Uh oh!

Conversation

chang47 commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chang47 commented Mar 23, 2018

Uh oh!

jspurlin commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chang47 Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joschect left a comment

Choose a reason for hiding this comment

Uh oh!

jspurlin commented Mar 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joschect Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspurlin Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzearing left a comment

Choose a reason for hiding this comment

Uh oh!

jspurlin commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzearing Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chang47 commented Apr 20, 2018

Uh oh!

jspurlin commented May 1, 2018

Uh oh!

chang47 commented May 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

chang47 commented Mar 23, 2018 •

edited

Loading

chang47 Mar 30, 2018 •

edited

Loading

joschect Mar 26, 2018 •

edited

Loading

jspurlin Mar 30, 2018 •

edited

Loading

dzearing Apr 12, 2018 •

edited

Loading