Skip to content
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

Prevent autocompleter from letting users insert two More blocks #7166

Merged
merged 1 commit into from
Jun 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 10 additions & 1 deletion components/autocomplete/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,14 @@ A function that returns the label for a given option. A label may be a string or
A function that returns the keywords for the specified option.

- Type: `Function`
- Required: Yes
- Required: No

#### isOptionDisabled

A function that returns whether or not the specified option should be disabled. Disabled options cannot be selected.

- Type: `Function`
- Required: No

#### getOptionCompletion

Expand Down Expand Up @@ -120,6 +127,8 @@ const fruitCompleter = {
],
// Declares that options should be matched by their name
getOptionKeywords: option => [ option.name ],
// Declares that the Grapes option is disabled
isOptionDisabled: option => option.name === 'Grapes',
// Declares completions should be inserted as abbreviations
getOptionCompletion: option => (
<abbr title={ option.name }>{ option.visual }</abbr>
Expand Down
14 changes: 14 additions & 0 deletions components/autocomplete/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ const { ENTER, ESCAPE, UP, DOWN, LEFT, RIGHT, SPACE } = keycodes;
* @returns {string[]} list of key words to search.
*/

/**
* @callback FnIsOptionDisabled
* @param {CompleterOption} option a completer option.
*
* @returns {string[]} whether or not the given option is disabled.
*/

/**
* @callback FnGetOptionLabel
* @param {CompleterOption} option a completer option.
Expand Down Expand Up @@ -92,6 +99,7 @@ const { ENTER, ESCAPE, UP, DOWN, LEFT, RIGHT, SPACE } = keycodes;
* @property {String} triggerPrefix the prefix that will display the menu.
* @property {(CompleterOption[]|FnGetOptions)} options the completer options or a function to get them.
* @property {?FnGetOptionKeywords} getOptionKeywords get the keywords for a given option.
* @property {?FnIsOptionDisabled} isOptionDisabled get whether or not the given option is disabled.
* @property {FnGetOptionLabel} getOptionLabel get the label for a given option.
* @property {?FnAllowNode} allowNode filter the allowed text nodes in the autocomplete.
* @property {?FnAllowContext} allowContext filter the context under which the autocomplete activates.
Expand Down Expand Up @@ -242,6 +250,10 @@ export class Autocomplete extends Component {
const { open, range, query } = this.state;
const { getOptionCompletion } = open || {};

if ( option.isDisabled ) {
Copy link
Member

Choose a reason for hiding this comment

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

I wondered why this was necessary due to the button actually being disabled. Now I see it is due to the fact that our keyboard listener calls select with indiscriminately when ENTER is pressed. I wonder if it would be clearer to conditionally call select in handleKeyDown. Just sharing my experience in case you concur.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we re-use select() in both the keyDown and onClick handlers. I think it's OK to leave as is.

return;
}

this.reset();

if ( getOptionCompletion ) {
Expand Down Expand Up @@ -345,6 +357,7 @@ export class Autocomplete extends Component {
value: optionData,
label: completer.getOptionLabel( optionData ),
keywords: completer.getOptionKeywords ? completer.getOptionKeywords( optionData ) : [],
isDisabled: completer.isOptionDisabled ? completer.isOptionDisabled( optionData ) : false,
} ) );

const filteredOptions = filterOptions( this.state.search, keyedOptions );
Copy link
Member

@brandonpayton brandonpayton Jun 7, 2018

Choose a reason for hiding this comment

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

It may be a pathological case, but since we show a limited number of options at a time, it is technically possible for all shown options to be disabled. Maybe we should update filterOptions to sort disabled options to the end before filtering.

Copy link
Member Author

Choose a reason for hiding this comment

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

Really like this idea, but let's punt on it until disabled items are a more common occurrence. Right now it's really only the More block case that we have to support.

Expand Down Expand Up @@ -604,6 +617,7 @@ export class Autocomplete extends Component {
id={ `components-autocomplete-item-${ instanceId }-${ option.key }` }
role="option"
aria-selected={ index === selectedIndex }
disabled={ option.isDisabled }
className={ classnames( 'components-autocomplete__result', className, {
'is-selected': index === selectedIndex,
} ) }
Expand Down
90 changes: 90 additions & 0 deletions components/autocomplete/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ describe( 'Autocomplete', () => {
options,
getOptionLabel: ( option ) => option.label,
getOptionKeywords: ( option ) => option.keywords,
isOptionDisabled: ( option ) => option.isDisabled,
};

const slashCompleter = {
Expand Down Expand Up @@ -435,6 +436,43 @@ describe( 'Autocomplete', () => {
} );
} );

it( 'set the disabled attribute on results', ( done ) => {
const wrapper = makeAutocompleter( [
{
...slashCompleter,
options: [
{
id: 1,
label: 'Bananas',
keywords: [ 'fruit' ],
isDisabled: true,
},
{
id: 2,
label: 'Apple',
keywords: [ 'fruit' ],
isDisabled: false,
},
],
},
] );
expectInitialState( wrapper );
// simulate typing '/'
simulateInput( wrapper, [ par( tx( '/' ) ) ] );
// wait for async popover display
process.nextTick( () => {
wrapper.update();

const firstItem = wrapper.find( 'button.components-autocomplete__result' ).at( 0 ).getDOMNode();
expect( firstItem.hasAttribute( 'disabled' ) ).toBe( true );

const secondItem = wrapper.find( 'button.components-autocomplete__result' ).at( 1 ).getDOMNode();
expect( secondItem.hasAttribute( 'disabled' ) ).toBe( false );

done();
} );
} );

it( 'navigates options by arrow keys', ( done ) => {
const wrapper = makeAutocompleter( [ slashCompleter ] );
// listen to keydown events on the editor to see if it gets them
Expand Down Expand Up @@ -591,6 +629,58 @@ describe( 'Autocomplete', () => {
} );
} );

it( 'does not select when option is disabled', ( done ) => {
const getOptionCompletion = jest.fn();
const testOptions = [
{
id: 1,
label: 'Bananas',
keywords: [ 'fruit' ],
isDisabled: true,
},
{
id: 2,
label: 'Apple',
keywords: [ 'fruit' ],
isDisabled: false,
},
];
const wrapper = makeAutocompleter( [ { ...slashCompleter, getOptionCompletion, options: testOptions } ] );
// listen to keydown events on the editor to see if it gets them
const editorKeydown = jest.fn();
const fakeEditor = wrapper.getDOMNode().querySelector( '.fake-editor' );
fakeEditor.addEventListener( 'keydown', editorKeydown, false );
expectInitialState( wrapper );
// the menu is not open so press enter and see if the editor gets it
expect( editorKeydown ).not.toHaveBeenCalled();
simulateKeydown( wrapper, ENTER );
expect( editorKeydown ).toHaveBeenCalledTimes( 1 );
// clear the call count
editorKeydown.mockClear();
// simulate typing '/'
simulateInput( wrapper, [ par( tx( '/' ) ) ] );
// wait for async popover display
process.nextTick( () => {
wrapper.update();
// menu should be open with all options
expect( wrapper.state( 'open' ) ).toBeDefined();
expect( wrapper.state( 'selectedIndex' ) ).toBe( 0 );
expect( wrapper.state( 'query' ) ).toEqual( '' );
expect( wrapper.state( 'search' ) ).toEqual( /(?:\b|\s|^)/i );
expect( wrapper.state( 'filteredOptions' ) ).toEqual( [
{ key: '0-0', value: testOptions[ 0 ], label: 'Bananas', keywords: [ 'fruit' ], isDisabled: true },
{ key: '0-1', value: testOptions[ 1 ], label: 'Apple', keywords: [ 'fruit' ], isDisabled: false },
] );
// pressing enter should NOT reset and NOT call getOptionCompletion
simulateKeydown( wrapper, ENTER );
expect( wrapper.state( 'open' ) ).toBeDefined();
expect( getOptionCompletion ).not.toHaveBeenCalled();
// the editor should not have gotten the event
expect( editorKeydown ).not.toHaveBeenCalled();
done();
} );
} );

it( 'doesn\'t otherwise interfere with keydown behavior', ( done ) => {
const wrapper = makeAutocompleter( [ slashCompleter ] );
// listen to keydown events on the editor to see if it gets them
Expand Down
3 changes: 3 additions & 0 deletions editor/components/autocompleters/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ export function createBlockCompleter( {
value: createBlock( name, initialAttributes ),
};
},
isOptionDisabled( inserterItem ) {
return inserterItem.isDisabled;
},
};
}

Expand Down
18 changes: 18 additions & 0 deletions editor/components/autocompleters/test/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,22 @@ describe( 'block', () => {
expect( labelComponents.at( 0 ).prop( 'icon' ) ).toBe( 'expected-icon' );
expect( labelComponents.at( 1 ).text() ).toBe( 'expected-text' );
} );

it( 'should derive isOptionDisabled from the item\'s isDisabled', () => {
const disabledInserterItem = {
name: 'core/foo',
title: 'foo',
keywords: [ 'foo-keyword-1', 'foo-keyword-2' ],
isDisabled: true,
};
const enabledInserterItem = {
name: 'core/bar',
title: 'bar',
keywords: [],
isDisabled: false,
};

expect( blockCompleter.isOptionDisabled( disabledInserterItem ) ).toBe( true );
expect( blockCompleter.isOptionDisabled( enabledInserterItem ) ).toBe( false );
} );
} );