Skip to content

Conversation

scottgonzalez
Copy link
Member

Fixes #10639

It's kind of strange that text selection is preserved when interacting with a native select, but we should try to match native behavior as well as possible. I think this is the only way to accomplish this.

It'd be good to have someone else verify this is working properly since I had to try lots of variations of this code to get it working everywhere and it's possible I overlooked something by the end. There are a few scenarios to test:

  • Interacting with the button and menu don't throw errors when there is no text selection.
  • Clicking the button should always focus the button, any existing text selection should be preserved.
  • Click a menu item should focus the button, any existing text selection should be preserved.
  • You should always be able to use the arrow keys on an open menu (because focus should be on the button).

@tjvantoll
Copy link
Member

I verified this fixes #10639 and that it doesn't introduce any errors. One thing I noticed is that this still doesn't completely mimic the native behavior though. If you 1) Select some text in a contenteditable element, 2) Click on the selectmenu button, 3) Start typing options... in the native select you will autocomplete options, but in the selectmenu you'll overwrite text in the contenteditable. This is a small thing that is probably ok, but I thought I'd mention it.

I tested in a few browsers and this looks good to me. I'd like to see is a test to verify that clicks on the button give the button focus, as that was the underlying problem of #10639. I don't think a test for the text selection is necessary, but it wouldn't hurt.

@scottgonzalez
Copy link
Member Author

Testing the text selection shouldn't be too bad, but I don't think we can write a reliable test for focus after click without using Web Driver.

@scottgonzalez scottgonzalez deleted the selectmenu-selection-focus branch October 16, 2014 12:14
scottgonzalez added a commit that referenced this pull request Nov 7, 2014
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.

2 participants