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

Fix WindowCommands tab stop bug #2858

Merged
merged 3 commits into from Feb 20, 2017

Conversation

Projects
None yet
3 participants
@neilt6
Contributor

neilt6 commented Feb 16, 2017

What changed?

Prevents WindowCommandsItem from erroneously responding to the tab key.

Fix WindowCommands tab stop bug
Prevents WindowCommandsItems from erroneously responding to the tab key.
@punker76

This comment has been minimized.

Show comment
Hide comment
@punker76

punker76 Feb 17, 2017

Member

@thoemmi What do you think?

Member

punker76 commented Feb 17, 2017

@thoemmi What do you think?

@punker76 punker76 requested a review from thoemmi Feb 17, 2017

@thoemmi

I don't get why WindowCommandsItem shouldn't get the focus? With the proposed change, you're not able to access the windows Commands with keyboard only. What issue is the PR trying to solve?

@neilt6

This comment has been minimized.

Show comment
Hide comment
@neilt6

neilt6 Feb 17, 2017

Contributor

WindowCommandItem is acting as a tab stop, but doesn't pass keyboard input to the Button it contains, making it useless behavior that can't be disabled from user code. Even if it did work, I find it counter-intuitive since the usual window commands (minimize, maximize, close, or even back in the case of UWP) aren't normally tab stops.

Contributor

neilt6 commented Feb 17, 2017

WindowCommandItem is acting as a tab stop, but doesn't pass keyboard input to the Button it contains, making it useless behavior that can't be disabled from user code. Even if it did work, I find it counter-intuitive since the usual window commands (minimize, maximize, close, or even back in the case of UWP) aren't normally tab stops.

@thoemmi

This comment has been minimized.

Show comment
Hide comment
@thoemmi

thoemmi Feb 17, 2017

Collaborator

The general window commands Minimize, Maximize, and Close don't get the focus with MahApps.Metro too. Those are generic Windows commands and not focusable in any Windows application (as far as I'm aware of.)

However, the additional Windows commands provided by MahApps.Metro are specific to the application, and IMHO should be accessible via keyboard.

If the buttons don't work, I'd think that this is the issue that must be fixed, and not by preventing them from getting the focus at all.

Collaborator

thoemmi commented Feb 17, 2017

The general window commands Minimize, Maximize, and Close don't get the focus with MahApps.Metro too. Those are generic Windows commands and not focusable in any Windows application (as far as I'm aware of.)

However, the additional Windows commands provided by MahApps.Metro are specific to the application, and IMHO should be accessible via keyboard.

If the buttons don't work, I'd think that this is the issue that must be fixed, and not by preventing them from getting the focus at all.

@neilt6

This comment has been minimized.

Show comment
Hide comment
@neilt6

neilt6 Feb 17, 2017

Contributor

I assumed this was a bug since WindowCommands.IsTabStop is set to false in the default style, and the buttons aren't actually interactive. In addition to fixing the tab stop behavior on the buttons, we should also make WindowCommands.IsTabStop enable/disable all of the tab stops like you would expect.

Contributor

neilt6 commented Feb 17, 2017

I assumed this was a bug since WindowCommands.IsTabStop is set to false in the default style, and the buttons aren't actually interactive. In addition to fixing the tab stop behavior on the buttons, we should also make WindowCommands.IsTabStop enable/disable all of the tab stops like you would expect.

@punker76 punker76 merged commit 3bb0c7f into MahApps:develop Feb 20, 2017

1 check passed

continuous-integration/teamcity Finished TeamCity Build MahApps.Metro PullRequest :: MahApps.Metro PullRequests : Tests passed: 62
Details
@punker76

This comment has been minimized.

Show comment
Hide comment
@punker76

punker76 Feb 20, 2017

Member

@neilt6 The inner buttons of the WindowCommandsItem are now focusable and interact with user key input.
/cc @thoemmi can you check the result of my last changes? thx

Member

punker76 commented Feb 20, 2017

@neilt6 The inner buttons of the WindowCommandsItem are now focusable and interact with user key input.
/cc @thoemmi can you check the result of my last changes? thx

@punker76 punker76 added this to the 1.5.0 milestone Feb 20, 2017

@punker76

This comment has been minimized.

Show comment
Hide comment
@punker76

punker76 Feb 20, 2017

Member

@neilt6 -> 1.5.0-alpha002

Member

punker76 commented Feb 20, 2017

@neilt6 -> 1.5.0-alpha002

@thoemmi

This comment has been minimized.

Show comment
Hide comment
@thoemmi

thoemmi Feb 21, 2017

Collaborator

@punker76 Looks good to me

Collaborator

thoemmi commented Feb 21, 2017

@punker76 Looks good to me

@punker76

This comment has been minimized.

Show comment
Hide comment
@punker76

punker76 Feb 21, 2017

Member

@thoemmi thx for watching 👍

Member

punker76 commented Feb 21, 2017

@thoemmi thx for watching 👍

@thoemmi

This comment has been minimized.

Show comment
Hide comment
@thoemmi

thoemmi Feb 21, 2017

Collaborator

Wow, it took you 30 seconds to reply to my comment 😉

Collaborator

thoemmi commented Feb 21, 2017

Wow, it took you 30 seconds to reply to my comment 😉

@neilt6 neilt6 deleted the neilt6:patch-1 branch Apr 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment