Skip to content

Add focusDefaultOption prop to prevent focusing first option #4080

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
+48 −5
Open

Add focusDefaultOption prop to prevent focusing first option #4080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e5b7fc2
focusDefaultOption prop added to enable to disable default focus on t…
Aug 5, 2019
45f4d54
Fixed lint error
Aug 5, 2019
0e42b3a
Updated docks advanced section
Francismb Oct 2, 2019
581beaa
Added example
Francismb Oct 2, 2019
b3491db
Merge branch 'master' into disable-auto-option-focus
Francismb Oct 2, 2019
6c1b768
Removed extra space
Francismb Oct 2, 2019
1b6c972
Merge branch 'disable-auto-option-focus' of github.com:frazboyz/react…
Francismb Oct 2, 2019
626f1c4
Merge branch 'disable-auto-option-focus' of https://github.com/Franci…
bladey Jun 10, 2020
261ad14
Update index.js
bladey Jun 10, 2020
5467d70
Filename tweaks.
bladey Jun 10, 2020
5d1cf66
Merge branch 'master' into Francismb-disable-auto-option-focus
bladey Nov 23, 2020
ee35360
Merge branch 'master' into Francismb-disable-auto-option-focus
bladey Dec 2, 2020
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
13 changes: 13 additions & 0 deletions docs/examples/DisableDefaultFocus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React from 'react';
import Select from 'react-select';

import { colourOptions } from '../data';

const DisableDefaultFocus = () => (
<Select
options={colourOptions}
focusDefaultOption={false}
/>
);

export default DisableDefaultFocus;
1 change: 1 addition & 0 deletions docs/examples/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,4 @@ export { default as MenuBuffer } from './MenuBuffer';
export { default as MenuPortal } from './MenuPortal';
export { default as Theme } from './Theme';
export { default as StyleCompositionExample } from './StyleCompositionExample';
export { default as DisableDefaultFocus } from './DisableDefaultFocus';
14 changes: 14 additions & 0 deletions docs/pages/advanced/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
Popout,
MenuBuffer,
MenuPortal,
DisableDefaultFocus,
MultiSelectSort,
} from '../../examples';

Expand Down Expand Up @@ -200,6 +201,19 @@ export default function Advanced() {
</ExampleWrapper>
)}

## Focus Default Option
Prevent React-Select from selecting the first option in the dropdown menu.

${(
<ExampleWrapper
label="Example of disabling default dropdown focus"
urlPath="docs/examples/DisableDefaultFocus.js"
raw={require('!!raw-loader!../../examples/DisableDefaultFocus.js')}
>
<DisableDefaultFocus />
</ExampleWrapper>
)}

## Accessing Internals
${(
<ExampleWrapper
Expand Down
25 changes: 20 additions & 5 deletions packages/react-select/src/Select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ export type Props = {
formatGroupLabel: typeof formatGroupLabel,
/* Formats option labels in the menu and control as React components */
formatOptionLabel?: (OptionType, FormatOptionLabelMeta) => Node,
/* Applies focus to the first option in the menu when opened */
focusDefaultOption: boolean,
Comment on lines +141 to +142
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit and bike shedding: I prefer autoFocusOption or autoFocusFirstOption as the prop name, but that's just a personal preference.

  1. autoFocus vs focus: in my mind reinforces that this is something that happens once when the menu opens, but that the focus can change with user input.
  2. option or firstOption vs defaultOption: defaultOption makes it sound like to me that the the default option is something that is customizable when it's not. Using option still leaves room for future props that could change which option is focused by default, but that's not the case at the moment.

But I'm fine with focusDefaultOption if that's easier.

Copy link

Choose a reason for hiding this comment

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

Hmmm I thought autoFocus would be like input#autofocus where the field snatches a user's focus.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, there's a little bit of overloading with the word "focus" which makes it hard to differentiate, since we have to talk about both focus of the Select component and the focus of individual options when the menu is open.

I think maybe the most descriptive (but probably too long) prop name would be autoFocusFirstOptionOnMenuOpen where the onMenuOpen disambiguates when the auto-focusing would happen.

Copy link

Choose a reason for hiding this comment

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

Knowing the full context, I prefer your autoFocusOption; however, could we not change the type signature to allow people to choose a different item to receive first focus? Who's to say maybe somebody would not want to focus the last option, or some middle one 🤷 . Regardless, I like that naming best. If it will be constrained to just boolean, I like your longer variable name: autoFocusFirstOptionOnMenuOpen - very crystal clear.

Copy link
Collaborator

@Methuselah96 Methuselah96 Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, it's hard to know where to stop as far as customization. Our options ordered from most customizable to least customizable are:

  1. Provide a getDefaultFocusedOption prop which is a function where the user is passed information and they can return the option they want to be focused by default.
  2. Make autoFocusOption a prop that can be 'first' | 'last' | false (can be expanded later, but I'm not sure what else you would need).
  3. Make autoFocusOption a prop that can be true or false.

Since react-select is already very complex and hard to keep straight with all its edge cases, I think we want to be conservative and avoid complexities.

That's why I suggested autoFocusOption instead of autoFocusFirstOption as my top choice (if we go with true | false) because it leaves open the possibility of adding 'first' | 'last' options later on.

But you make a good point. Maybe autoFocusOptionOnMenuOpen is the best of both worlds because it avoids using first so that we can expand it later and is very clear.

/* Resolves option data to a string to be displayed as the label by components */
getOptionLabel: typeof getOptionLabel,
/* Resolves option data to a string to compare options and specify value attributes */
Expand Down Expand Up @@ -258,6 +260,7 @@ export const defaultProps = {
escapeClearsValue: false,
filterOption: createFilter(),
formatGroupLabel: formatGroupLabel,
focusDefaultOption: true,
getOptionLabel: getOptionLabel,
getOptionValue: getOptionValue,
isDisabled: false,
Expand Down Expand Up @@ -415,9 +418,21 @@ export default class Select extends Component<Props, State> {
const menuOptions = nextProps.menuIsOpen
? this.buildMenuOptions(nextProps, selectValue)
: { render: [], focusable: [] };
const focusedValue = this.getNextFocusedValue(selectValue);
const focusedOption = this.getNextFocusedOption(menuOptions.focusable);
this.setState({ menuOptions, selectValue, focusedOption, focusedValue });

let focusedValue = null;
let focusedOption = null;

if (nextProps.focusDefaultOption) {
focusedValue = this.getNextFocusedValue(selectValue);
focusedOption = this.getNextFocusedOption(menuOptions.focusable);
}
Comment on lines +425 to +428
Copy link
Collaborator

@Methuselah96 Methuselah96 Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that this prop should affect focusedValue, I think it should only affect focusedOption. focusedValue refers to the focused value in a multi-select (if you hover over a selected value or expand the menu of a multi-select and press the left arrow) and his little relation to the focusedOption. By default focusedValue is set to null (see implementation) unless you perform one of the above actions, so it doesn't have the same problem as focusedOption that this PR is trying to solve.

Comment on lines +422 to +428
Copy link
Collaborator

@Methuselah96 Methuselah96 Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

A cleaner way to do this would be to check focusDefaultOption in this.getNextFocusedOption instead of in this method (componentWillReceiveProps) since the logic can be confined within getNextFocusedOption.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And actually, now that I think it about, this logic is incorrect if we don't move it into this.getNextFocusedOption.

The situation where this would make a difference is if the menu is open and the options prop changes. What this.getNextFocusedOption does is it checks to see if the currently selected option is still a focusable option, and if it is, then it keeps it as the focused option (which is still what we want even if focusDefaultOption is false because there is already a selected option).

So instead we should change this.getNextFocusedOption from:

getNextFocusedOption(options: OptionsType) {
  const { focusedOption: lastFocusedOption } = this.state;
  return lastFocusedOption && options.indexOf(lastFocusedOption) > -1
    ? lastFocusedOption
    : options[0];
}

to:

getNextFocusedOption(options: OptionsType, focusDefaultOption: boolean) {
  const { focusedOption: lastFocusedOption } = this.state;
  if (lastFocusedOption && options.indexOf(lastFocusedOption) > -1) {
    return lastFocusedOption;
  }
  return focusDefaultOption ? options[0] : null;
}


this.setState({
menuOptions,
selectValue,
focusedOption,
focusedValue,
});
}
// some updates should toggle the state of the input visibility
if (this.inputIsHiddenAfterUpdate != null) {
Expand Down Expand Up @@ -498,7 +513,7 @@ export default class Select extends Component<Props, State> {
openMenu(focusOption: 'first' | 'last') {
const { selectValue, isFocused } = this.state;
const menuOptions = this.buildMenuOptions(this.props, selectValue);
const { isMulti } = this.props;
const { isMulti, focusDefaultOption } = this.props;
let openAtIndex =
focusOption === 'first' ? 0 : menuOptions.focusable.length - 1;

Expand All @@ -516,7 +531,7 @@ export default class Select extends Component<Props, State> {
this.setState({
menuOptions,
focusedValue: null,
focusedOption: menuOptions.focusable[openAtIndex],
focusedOption: focusDefaultOption ? menuOptions.focusable[openAtIndex] : null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might have the interesting effect of calling this.announceAriaLiveContext({ event: 'menu' }); twice if the user uses the keyboard to navigate the Select component. The first time will be in the callback of this call to setState (see a few lines below this one). The second time will be in focusOption where it calls this.announceAriaLiveContext({ event: 'menu' }); again if there is no selected option.

I believe we can remove the second case because there is not a time where we call focusOption when the menu is closed (#3427 removed the last case of this situation) and should do that as part of this PR in order to avoid the above scenario.

}, () => {
this.onMenuOpen();
this.announceAriaLiveContext({ event: 'menu' });
Expand Down