Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adds keyboard shortcuts to PM UI Preview Window, License Acceptance and File Conflict dialog #3967

Merged
merged 8 commits into from
Sep 30, 2021
Merged

Adds keyboard shortcuts to PM UI Preview Window, License Acceptance and File Conflict dialog #3967

merged 8 commits into from
Sep 30, 2021

Conversation

dominoFire
Copy link
Contributor

@dominoFire dominoFire commented Mar 25, 2021 •
edited
Loading

Bug

Fixes: NuGet/Home#10691

Regression? Last working version:

Description

Accessibility improvement:

  • Adds keyboard access keys to File Conflict Dialog, Preview Changes Window Dialog and License Acceptance Window by adding an underscore (_) to button texts. This improves usability with keyboard.
  • Added support to access keys in Checkbox, disabled in CheckBox style before this change.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • [ ] Automated tests added
    • [ ] Test exception
    • N/A: UI change, manually validated

  • Documentation

    • [ ] Documentation PR or issue filled
    • N/A: Does not change main scenario features

  • Validation

Manual validation: it shows the keyboard shortcuts when pressing 'Alt' key.

File Conflict Dialog

After:
image

Accessibility Insights test:
image

Preview Window Dialog

After:
image

Accessibility Insights test:
image

License Acceptance Window Dialog

Affter:
image

Accessibility Insights test:
image

@dominoFire dominoFire requested a review from a team as a code owner March 25, 2021 02:34
donnie-msft
donnie-msft reviewed Mar 29, 2021
View reviewed changes
Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

Did you run Accessibility Insights before/after these changes? I don't think it's a hard violation, but there may have been a warning that your PR resolves.

Also, I think it's just good to make sure AI reflects the new change.

src/NuGet.Clients/NuGet.PackageManagement.UI/Converters/AcceleratorKeyConverter.cs Outdated Show resolved Hide resolved
src/NuGet.Clients/NuGet.PackageManagement.UI/Resources.resx Outdated
@@ -265,16 +265,20 @@
<value>File Conflict</value>
</data>
<data name="Button_No" xml:space="preserve">
<value>No</value>
<value>_No</value>
<comment>Button_Yes, Button_No, ButtonYesToAll, Button_NoToAll should have underscore in different letters. See https://docs.microsoft.com/en-us/windows/uwp/design/input/access-keys#choose-access-keys</comment>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest checking with localization team to make sure they understand this, if you haven't already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will follow up with them once this PR is merged.

@donnie-msft
Copy link
Contributor

I briefly played with a few options, and I'm pretty confused as to how WPF is handling these.
Setting the Automation Property of AccessKey or AcceleratorKey did nothing by itself. Only if I set button's text with Back_ground does it actually affect the behavior :D
image

My observation:

  • "_" in content

    • makes the shortcut key work
    • sets the AccessKey property in AI

  • "AutomationProperty.AccessKey"

    • property does not reflect in AI
    • does not enable shortcut
    • if set with "_" content, enables shortcut BUT breaks NVDA/Narrator announcement

  • "AutomationProperty.AcceleratorKey"

    • property does reflect in AI
    • no behavior
    • no NVDA/Narrator announcement
    • if set with "_" content
      • NVDA announces the key twice (assuming content + AcceleratorKey property)
      • Narrator as expected
      • AI reflects both!

image

@dominoFire
Copy link
Contributor Author

Need to verify keyboard accelerators in another languages. For example, in Russsian language, it does not show keyboard accelerators.

image

@ghost ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jun 13, 2021
@ghost
Copy link

ghost commented Jun 13, 2021

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@ghost ghost closed this Jun 20, 2021
@dominoFire
Copy link
Contributor Author

Reopening to finish work.

@dominoFire dominoFire reopened this Sep 29, 2021
@ghost ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 29, 2021
@dominoFire dominoFire force-pushed the dev-dominofire-fileconflictdialog-shortcuts-fix branch from f91058e to cd9cc5f Compare September 29, 2021 04:07
@donnie-msft donnie-msft dismissed jebriede’s stale review September 29, 2021 16:21

Code changed a lot, and new Approval rules

@dominoFire dominoFire changed the title Adds keyboard shorcuts to PM UI File Conflict dialog Adds keyboard shorcuts to PM UI Preview Window, License Acceptance and File Conflict dialog Sep 29, 2021
@dominoFire dominoFire changed the title Adds keyboard shorcuts to PM UI Preview Window, License Acceptance and File Conflict dialog Adds keyboard shortcuts to PM UI Preview Window, License Acceptance and File Conflict dialog Sep 29, 2021
@dominoFire dominoFire requested a review from a team September 29, 2021 19:56
@dominoFire dominoFire requested a review from a team September 29, 2021 20:05
donnie-msft
donnie-msft reviewed Sep 29, 2021
View reviewed changes
src/NuGet.Clients/NuGet.PackageManagement.UI/Resources/Resources.xaml
@@ -213,6 +213,7 @@
Grid.Column="1"
Focusable="False"
Margin="{TemplateBinding Padding}"
RecognizesAccessKey="True"
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to support access keys in Checkbox, disabled in this style before this change.

@dominoFire dominoFire force-pushed the dev-dominofire-fileconflictdialog-shortcuts-fix branch from 46e1146 to a2250f8 Compare September 30, 2021 00:11
@dominoFire dominoFire merged commit 1a9ae7a into dev Sep 30, 2021
@dominoFire dominoFire deleted the dev-dominofire-fileconflictdialog-shortcuts-fix branch September 30, 2021 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@donnie-msft donnie-msft donnie-msft approved these changes

@erdembayar erdembayar erdembayar approved these changes

@jeffkl jeffkl jeffkl approved these changes

@jebriede jebriede Awaiting requested review from jebriede

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

File Conflict dialog does not have access keys
5 participants
@dominoFire @donnie-msft @erdembayar @jebriede @jeffkl