ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes #4594

chang47 · 2018-04-18T01:02:35Z

Problem:
When submitting a value in a combo box, depending on what input is used to submit the value, from an accessibility standpoint, we want to take different actions that would accurately reflect the input that was used whenever we submit our value for callbacks.

For example:
Pressing enter, we would submit the current value in the combo box, however pressing tab would also submit the current value, but it would also move focus to the next element.

Currently, we have no way of differentiating what inputs were used on the component so we can't deal with what happens with a combo box correctly after its input has been changed.
Solution:
We exposed the keycode that was pressed (if any) to the callback so that the user can do whatever they deem necessary to create an accessible experienece using a combo box

Validation:
Seeing as the only change done was adding an extra parameter to the callback, nothing should be broken, however I have played with with all the combo boxes submitting my values by hitting: enter, tab, escape, and click and ensured that the submitted code gets called, there's no errors, and that the behavior remained the same as before.

chang47 · 2018-04-18T01:02:49Z

@jspurlin

jspurlin · 2018-04-18T13:12:43Z

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

   * @param searchDirection - the direction to search along the options from the given index
   */
-  private _setSelectedIndex(index: number, searchDirection: SearchDirection = SearchDirection.none) {
+  private _setSelectedIndex(index: number, searchDirection: SearchDirection = SearchDirection.none, submittedInput?: KeyCodes) {


Consider using a better name for the new parameter, submittedInput isn't very clear

Also, should this be a keyCode or the whole event? Can't a value be submitted by changing the value in the input and clicking away? What happens currently in that case?

@jspurlin Yes a value can be submitted by changing the value. I wanted to abstract as much information as necessary from the user so I used keycodes, but you do bring a valid point, maybe just passing the whole event will be better for the user to make an more informed decision. What do you think?

For our specific case, we currently only check for tabs so focus would go to wherever we clicked on.

jspurlin · 2018-04-18T20:04:34Z

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

   * Submit a pending value if there is one
   */
-  private _submitPendingValue(submittedInput?: KeyCodes) {
+  private _submitPendingValue(submitPendingValueEvent: React.KeyboardEvent<HTMLElement | Autofill> | React.FocusEvent<HTMLInputElement>) {


Consider wrapping this to multiple lines

Traditionally events that are passed to callbacks in fabric use type any to get around having to specify all of this

Thank goodness, I was debating about type safety vs having a complex parameter, and chose type safety. But looking around now, you're right, there are instances where we just use any.

jspurlin · 2018-04-18T20:07:12Z

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

-  private _onItemClick(index: number | undefined): () => void {
-    return (): void => {
-      this._setSelectedIndex(index as number);
+  private _onItemClick(index: number | undefined): (ev: React.MouseEvent<any> | React.FormEvent<any>) => void {


same comment here, this could just be type any

jspurlin · 2018-04-18T20:07:45Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.types.ts

   *    (and hence only value would be true, the other parameter would be null in this case)
   */
-  onChanged?: (option?: IComboBoxOption, index?: number, value?: string, submittedInput?: KeyCodes) => void;
+  onChanged?: (option?: IComboBoxOption, index?: number, value?: string, submitPendingValueEvent?: React.KeyboardEvent<any> | React.FocusEvent<any> | React.MouseEvent<any> | React.FormEvent<any>) => void;


same comment here

jspurlin · 2018-04-18T20:54:27Z

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.types.ts

 import { IStyle, ITheme } from '../../Styling';
 import { IButtonStyles } from '../../Button';
-import { IRenderFunction } from '../../Utilities';
+import { IRenderFunction, KeyCodes } from '../../Utilities';


You no longer need KeyCodes

…an/newComboboxCallback

jspurlin · 2018-04-18T21:02:55Z

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

   * @param searchDirection - the direction to search along the options from the given index
   */
-  private _setSelectedIndex(index: number, searchDirection: SearchDirection = SearchDirection.none) {
+  private _setSelectedIndex(index: number, searchDirection: SearchDirection = SearchDirection.none, submitPendingValueEvent?: any) {


Did you look at making this new parameter the second parameter? Doing that might make it to where you have to pass searchDirection in the cases where you don't need

jspurlin · 2018-04-18T21:03:44Z

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

-      this._setSelectedIndex(index as number);
+  private _onItemClick(index: number | undefined): (ev: any) => void {
+    return (ev: React.MouseEvent<any> | React.FormEvent<any>): void => {
+      this._setSelectedIndex(index as number, undefined, ev);


Should this be searchDirection.none instead of undefined?

jspurlin · 2018-04-18T21:04:06Z

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

-    return (): void => {
-      this._setSelectedIndex(index as number);
+  private _onItemClick(index: number | undefined): (ev: any) => void {
+    return (ev: React.MouseEvent<any> | React.FormEvent<any>): void => {


this can be updated to any

jspurlin · 2018-04-18T23:59:13Z

common/changes/office-ui-fabric-react/chiechan-newComboboxCallback_2018-04-18-00-34.json

+  "changes": [
+    {
+      "packageName": "office-ui-fabric-react",
+      "comment": "ComboBox: Added KeyCode pressed as additional paramater to onChanged callback",


This (and the title to the PR) need to be updated

* master: Applying package updates. ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes (microsoft#4594) Remove usage of Number.NaN (microsoft#4615) Update createRef to match the new React 16.3 api (microsoft#4598) Update Breadcrumb.base.tsx Fix minor typos (microsoft#4607) Remove unused variables and enable no-unused-variable (microsoft#4608) Add optional overflowIndex prop to Breadcrumb (microsoft#4609) Applying package updates. Reenable bundlesize in yaml (microsoft#4590) Fix more index imports (microsoft#4604)

* master: DetailsRow: Flexshrink fix (microsoft#4622) [TextField] Implemented masking (microsoft#3783) Applying package updates. ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes (microsoft#4594) Remove usage of Number.NaN (microsoft#4615)

REDMOND\chiechan added 3 commits April 17, 2018 17:30

added new paramater to callback

46fdc33

added change file

d853f6b

updated the combobox type import

b3ae60d

jspurlin reviewed Apr 18, 2018

View reviewed changes

changed callback to include all events instead of just keycode

564fdad

jspurlin reviewed Apr 18, 2018

View reviewed changes

REDMOND\chiechan added 2 commits April 18, 2018 13:36

made events more generic so user can parse it themselves

d5f18be

made event optional

fc51d4b

jspurlin reviewed Apr 18, 2018

View reviewed changes

Merge https://github.com/OfficeDev/office-ui-fabric-react into chiech…

baaf37c

…an/newComboboxCallback

jspurlin reviewed Apr 18, 2018

View reviewed changes

REDMOND\chiechan added 2 commits April 18, 2018 15:01

re-arranged function call and added more any events

d6d02e9

removed unneeded parameter added

836b00a

jspurlin reviewed Apr 18, 2018

View reviewed changes

updated change file

b3551c7

chang47 changed the title ~~ComboBox: Added KeyCode pressed as additional paramater to onChanged callback for accessibility~~ ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes Apr 19, 2018

fixed type

f322efa

jspurlin approved these changes Apr 19, 2018

View reviewed changes

changed event to any to prevent crashes

55b0465

jspurlin merged commit 02b098f into microsoft:master Apr 19, 2018

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

ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes #4594

ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes #4594

Uh oh!

Conversation

chang47 commented Apr 18, 2018

Uh oh!

chang47 commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chang47 Apr 18, 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!

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 Apr 18, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chang47 Apr 18, 2018 •

edited

Loading

jspurlin Apr 18, 2018 •

edited

Loading