Skip to content

Conversation

robotdan
Copy link
Contributor

@robotdan robotdan commented Jan 8, 2014

Autocomplete: Ignore ENTER and TAB events for items with 'ui-state-disabled'. Fixes #9695 - autocomplete: should not allow to select a disabled item of its menu with the ENTER key

When ‘ui-state-disabled’ has been added to the

  • elements of the
    menu, ignore ENTER and TAB keydown events.

    For TAB, also call event.preventDefault() so that the menu is not
    dismissed when the user presses TAB on a disabled item. If this is not
    called, while select() is not called, the item will appear to have been
    selected.

  • …sabled'. Fixes #9695 - autocomplete: should not allow to select a disabled item of its menu with the ENTER key
    
    When ‘ui-state-disabled’ has been added to the <li> elements of the
    menu, ignore ENTER and TAB keydown events.
    
    For TAB, also call event.preventDefault() so that the menu is not
    dismissed when the user presses TAB on a disabled item.  If this is not
    called, while select() is not called, the item will appear to have been
    selected.
    @robotdan
    Copy link
    Contributor Author

    robotdan commented Jan 8, 2014

    Test case with patch from this branch.
    http://jsfiddle.net/robotdan/j93LM/

    @tjvantoll mentioned in Ticket #9596 that the plugin could be changed to allow this behavior to be modified by the user in an extension point. Would that strategy be preferred to the proposed change here?

    setTimeout(function() {
    ok ( !itemSelected, "Item has not been selected" );
    start();
    }, 200 );
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Maybe someone can comment on this value, I looked around at some other unit tests, and perhaps 200 ms is overkill and will only add extra time to the unit tests?

    Copy link
    Member

    Choose a reason for hiding this comment

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

    The test just needs to be deferred until after the menu is shown asynchronously. Our standard is to omit the second argument altogether in this situation.

    @tjvantoll
    Copy link
    Member

    @tjvantoll mentioned in Ticket #9596 that the plugin could be changed to allow this behavior to be modified by the user in an extension point. Would that strategy be preferred to the proposed change here?

    I personally like this approach better. I think that we should at least expose an extension point, but if we can take care of the problem itself (without too much work), we should do it.

    For the tests you should add another item to the menu that is not disabled. Then verify that the item is selected for the enabled item, and is not selected for the disabled item.

    It would be nice if you could test this without relying on overriding the instance's _renderItem(), but I can't think of a good way around it myself.

    this.menu.select( event );
    } else {
    event.preventDefault();
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Why are you preventing the user from leaving the field?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    I wasn't sure if this was the right thing to do or not. If I omit this, while the select: handler isn't called, the menu is dismissed and the value is set in the input field. If I call .preventDefault() it behaves the same as ENTER.

    So apart from the actual handler not being called, it appears that the value was selected, is that the expected behavior?

    Copy link
    Member

    Choose a reason for hiding this comment

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

    Well, I still personally think this is a strange situation to be in. But if you can't select the item, then the item really shouldn't have any effect on the widget at all.

    Copy link
    Member

    Choose a reason for hiding this comment

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

    I agree that we shouldn't prevent the user from tabbing. I didn't realize that was happening here.

    To do this right, the widget should prevent the <input>'s value from being updated as disabled menu items are focused; that would make this Tab situation a non-issue. That being said, I'm concerned that implementing that would add too much complexity for an edge case.

    Copy link
    Member

    Choose a reason for hiding this comment

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

    I have a feeling this whole thing is relatively easy to implement in third party code. I'll put together a demo this afternoon to verify.

    Copy link
    Member

    Choose a reason for hiding this comment

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

    Cool. I marked the ticket as valid because I think there needs to be a trivial way to do this. If you can do it easily in an extension we don't need it in the actual widget.

    @robotdan robotdan deleted the bug_9695 branch January 22, 2014 05:11
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants