Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Report keyboard shortcut when entering a list #15826
Report keyboard shortcut when entering a list #15826
Changes from 6 commits
aadac80
57df466
667c0ca
2a1d18b
43c500e
acaeeee
9a05a44
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should speak shortcuts for any object that focus enters. I cannot remember specifically why we never originally did. If you can make an argument for lists, I think you can for any other container. Most likely this was simply that early on it was only done for focus and not focusEntered, and then when later abstracted, it was deliberately removed from focusEntered to match existing behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelDCurran, this has just been discussed with @LeonarddeR in #15826 (comment), #15826 (comment) and #15826 (comment).
The group container can have a shortcut key defined but actually not working. To avoid to get other such not working case, it seems safer to restrict this PR to the only known, frequent and useful case of lists. If in the future we can find other containers for which the shortcut key should be announced, we may include their role too.
Or should I instead exclude explicitly the case of grouping which is not working?
A third possibility can be to remove this filtering and consider that putting a shortcut key in grouping where it does not work should not be done in first place by the developer of the application.
@michaelDCurran, let me know which solution you would prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether authors place a shortcut on groupings, or whether these shortcuts actually work on groupings in applications really is separate to this pr I feel. However, your current solution solves your immediate problem, and does not introduce other known problems. Therefore, could you please at least add a comment above this line stating why we only do it for list at this stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Done in 43c500e.