Skip to content

Adding Toggle pattern support for ListView items with checkboxes #4039

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
Merged
merged 1 commit into from
Oct 2, 2020
Merged

Adding Toggle pattern support for ListView items with checkboxes #4039

merged 1 commit into from
Oct 2, 2020

Conversation

vladimir-krestov
Copy link
Contributor

@vladimir-krestov vladimir-krestov commented Sep 30, 2020 •
edited by RussKie
Loading

Proposed changes

  • Implement Toggle pattern for ListViewItemAccessibleObject if CheckBoxes is true
  • Add on/off Narrator announcement for ListView items if CheckBoxes is true

Customer Impact

  • Now a user can hear if a ListView item is checked(on) or unchecked(off) using Narrator

Regression?

  • Yes

Risk

  • Minimal

Screenshots

Before

image

After

image

image

Test methodology

  • Unit tests
  • Manual testing

Accessibility testing

  • using Inspect and Narrator

Test environment(s)

  • .Net Version: 5.0.0-rc.2.20472.5
  • Microsoft Windows [Version 10.0.19041.508]
Microsoft Reviewers: Open in CodeFlow

@vladimir-krestov vladimir-krestov requested a review from a team as a code owner September 30, 2020 22:31
@vladimir-krestov vladimir-krestov added this to the 5.0 GA milestone Sep 30, 2020
@vladimir-krestov vladimir-krestov added the waiting-review This item is waiting on review by one or more members of team label Sep 30, 2020
@vladimir-krestov vladimir-krestov force-pushed the Issue_4029_Implement_ListViewTogglePattern branch from c49fdc8 to 85d5f44 Compare September 30, 2020 22:33
@vladimir-krestov
Copy link
Contributor Author

The keyboard on/off announcement works well but if to use mouse there is a little issue:

  • Narrator announces "off" state but not "on" state because the mouse selection announcement overlaps "on" state announcement.
    image

Narrator announcement for mouse actions is not a common scenario so it will be fixed as .Net 6.0 fix.

@RussKie RussKie added servicing-consider .NET Shiproom label indicating a PR seeks to enter into a branch under Tell-Mode criteria and removed waiting-review This item is waiting on review by one or more members of team labels Oct 1, 2020
@RussKie RussKie added the waiting-author-feedback The team requires more information from the author label Oct 1, 2020
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Oct 1, 2020
@merriemcgaw merriemcgaw added the a11y-MAS High Priority - Accessibility violation of Microsoft Accessibility Standards label Oct 1, 2020
@vladimir-krestov
Copy link
Contributor Author

@merriemcgaw, I asked Sergey to made testing for the fix dlls.
Everything works well except the described issue above. ✔️
The PR is ready to merge. Waiting for approval

Tanya-Solyanik
Tanya-Solyanik previously approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

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

Looks good, I have only minor comments

@Tanya-Solyanik
Copy link
Contributor

Tanya-Solyanik commented Oct 1, 2020 •
edited
Loading

Narrator announcement for mouse actions is not a common scenario so it will be fixed as .Net 6.0 fix.

Did you open a 6.0 issue?

Narrator announces "off" state but not "on" state because the mouse selection announcement overlaps "on" state announcement.

How do you plan to fix that?

@merriemcgaw
Copy link
Member

Per offline email, @Tanya-Solyanik will run some extra tests and when @SergeySmirnov-Akvelon updates per her comments we will send ask mail and get this merged.

@Tanya-Solyanik
Copy link
Contributor

I ran some tests, it seems the issue with mouse happens only when the 1st subitem(cell) is selected, as opposed to the whole item (row). This makes sense because checked state applies only to items.
@SergeySmirnov-Akvelon - we don't need another bug, this is by-design

@vladimir-krestov
Copy link
Contributor Author

Thank you, @Tanya-Solyanik, for the review. You are very attentive 😊

@SergeySmirnov-Akvelon @vladimir-krestov
Add Toggle pattern support for ListView items
b88d926 
Related to PR dotnet#3963 and PR dotnet#3224
Fix keyboard Narrator announcing the check/uncheck state of ListView item when enabling ListView CheckBoxes property
Fixes Issue dotnet#4029
@vladimir-krestov vladimir-krestov force-pushed the Issue_4029_Implement_ListViewTogglePattern branch from 85d5f44 to b88d926 Compare October 2, 2020 09:47
@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #4039 into release/5.0 will decrease coverage by 30.95662%.
The diff coverage is 80.00000%.

@@                  Coverage Diff                   @@
##           release/5.0       #4039          +/-   ##
======================================================
- Coverage     68.02385%   37.06723%   -30.95662%     
======================================================
  Files             1416         924         -492     
  Lines           509461      250998      -258463     
  Branches         41437       36990        -4447     
======================================================
- Hits            346555       93038      -253517     
+ Misses          156686      152499        -4187     
+ Partials          6220        5461         -759
Flag Coverage Δ
#Debug 37.06723% <80.00000%> (-30.95662%) ⬇️
#production 37.06723% <80.00000%> (+0.00350%) ⬆️
#test ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@Tanya-Solyanik
Copy link
Contributor

@Pilchie - we would like to take this fix in Tell Mode for 5.0 GA. Could you please help me with the next steps? As far as I understand, it's

  1. merge into release/5.0
  2. send a heads up e-mail to tactics

@Pilchie
Copy link
Member

Pilchie commented Oct 2, 2020

We are in tell mode only for test only or infrastructure changes. Any product changes require Tactics approval. Please add the servicing-consider label, and the shiproom template questions.

@Tanya-Solyanik
Copy link
Contributor

Steve had approved in the e-mail

@Tanya-Solyanik Tanya-Solyanik added servicing-approved .NET Shiproom approved the PR for merge and removed servicing-consider .NET Shiproom label indicating a PR seeks to enter into a branch under Tell-Mode criteria labels Oct 2, 2020
@Tanya-Solyanik Tanya-Solyanik changed the title Adding Toggle pattern support for ListView items Adding Toggle pattern support for ListView items with checkboxes Oct 2, 2020
@Tanya-Solyanik Tanya-Solyanik merged commit 98120ed into dotnet:release/5.0 Oct 2, 2020
@vladimir-krestov vladimir-krestov deleted the Issue_4029_Implement_ListViewTogglePattern branch November 4, 2020 07:35
@ghost ghost locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@RussKie RussKie RussKie left review comments

@Tanya-Solyanik Tanya-Solyanik Tanya-Solyanik approved these changes

Assignees

@vladimir-krestov vladimir-krestov

Labels
a11y-MAS High Priority - Accessibility violation of Microsoft Accessibility Standards servicing-approved .NET Shiproom approved the PR for merge
Projects
None yet
Milestone
5.0 GA
Development

Successfully merging this pull request may close these issues.
6 participants
@vladimir-krestov @Tanya-Solyanik @merriemcgaw @Pilchie @RussKie @SergeySmirnov-Akvelon