Skip to content

Improve popup box keyboard control#865

Closed
miketoon wants to merge 1 commit intojuce-framework:masterfrom
WeAreROLI:miketoon/bugfix/popupmenu-improve-kayboard-control
Closed

Improve popup box keyboard control#865
miketoon wants to merge 1 commit intojuce-framework:masterfrom
WeAreROLI:miketoon/bugfix/popupmenu-improve-kayboard-control

Conversation

@miketoon
Copy link
Copy Markdown
Contributor

@miketoon miketoon commented Mar 3, 2021

Set ticked/selected item as highlighted on menu construction and
prevent fake mouse events from resetting this.

Previously when a menu was opened with the enter key a user would have to press escape key to close the menu. This was inconsistent if the selected/ticked item was highlighted using the arrow keys and then enter pressed.

Also previosuly keyboard navigation would start at the top/bottom of the list depending on whether up/down was pressed but this change uses the ticked/selected item as the start point.

@reuk
Copy link
Copy Markdown
Member

reuk commented Mar 9, 2021

I think this feature makes sense, but I'm not sure about the implementation. It's valid for a popup menu to have multiple ticked items (for example if it's created as a window menu, rather than a combobox dropdown) so I don't think it makes sense to use the 'ticked' state to set the initial selection. To see what I'm talking about, apply this patch and run the 'Menus Demo' in the DemoRunner. In this case, we probably don't want the ticked items to be selected as soon as the menu opens.

My preference would be to add an 'initially selected item' member to PopupMenu::Options, and to have the ComboBox automatically set this option to the currently selected item when showing the menu. This would also allow users to retain the existing menu behaviour, in the case that they don't wish to have any initial selection.

Do you think a solution like that would work for you?

@miketoon miketoon force-pushed the miketoon/bugfix/popupmenu-improve-kayboard-control branch from 8b96e91 to afd4a9c Compare March 10, 2021 08:17
@miketoon
Copy link
Copy Markdown
Contributor Author

yep that works. i've pushed a change to this branch which implements that.

@reuk
Copy link
Copy Markdown
Member

reuk commented Mar 10, 2021

I've added this in d62d3aa.

Hopefully this works for you, please let me know if there are any issues.

@reuk reuk closed this Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants