Skip to content

Fixing broken hierarchy for items list in Combobox#4185

Merged
RussKie merged 4 commits intodotnet:masterfrom
SergeySmirnov-Akvelon:Issue-3654_combobox_hierarchy_is_broken_for_list_with_items
Nov 26, 2020
Merged

Fixing broken hierarchy for items list in Combobox#4185
RussKie merged 4 commits intodotnet:masterfrom
SergeySmirnov-Akvelon:Issue-3654_combobox_hierarchy_is_broken_for_list_with_items

Conversation

@SergeySmirnov-Akvelon
Copy link
Contributor

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon commented Nov 2, 2020 •
edited
Loading

Fixes #3654

Proposed changes

  • Updated logic of "FragmentNavigate" method for Combobox accessible objects.
  • Updated logic for ComboBox.ComboBoxChildNativeWindow. Now we return our ComboBoxChildEditUiaProvider instead of the native.
  • Fixed unit tests

Customer Impact

Before:
Issue-3654-broken
After:
Issue-3654-fixed

Regression?

  • Yes

Risk

  • Minimal

Test methodology

  • Manually
  • CTI team

Accessibility testing

  • Inspect
  • Accessibility Insights
  • Narrator

Test environment(s)

  • Microsoft Windows [Version 10.0.19041.388]
  • .Net SDK 5.0.100-rc.2.20479.15
Microsoft Reviewers: Open in CodeFlow

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon requested a review from a team as a code owner November 2, 2020 11:19
@codecov
Copy link

codecov bot commented Nov 2, 2020 •
edited
Loading

Codecov Report

Merging #4185 (d550bb9) into master (b66809f) will decrease coverage by 0.16338%.
The diff coverage is 97.64310%.

@@                 Coverage Diff                 @@
##              master       #4185         +/-   ##
===================================================
- Coverage   98.12475%   97.96137%   -0.16339%     
===================================================
  Files            494         503          +9     
  Lines         258472      259292        +820     
  Branches        4489        4615        +126     
===================================================
+ Hits          253625      254006        +381     
- Misses          4089        4473        +384     
- Partials         758         813         +55
Flag Coverage Δ
Debug 97.96137% <97.64310%> (-0.16339%) ⬇️
production 100.00000% <ø> (?)
test 97.96137% <97.64310%> (-0.16339%) ⬇️

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

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon force-pushed the Issue-3654_combobox_hierarchy_is_broken_for_list_with_items branch from f1412cc to 48f1192 Compare November 9, 2020 11:41
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Nov 9, 2020
@SergeySmirnov-Akvelon
Fix Accessibility: The list item of comboBox incorrectly displayed in…
0f260e2 
… inspect tree when setting its DropDownStyle as DropDown or DropDownList. dotnet#3654

Issue is reproduced because ComboBoxChildListUiaProvider has no parent. Also "FragmentNavigate" method does not work clearly and sometimes we got an incorrect result.

- Updated logic of "FragmentNavigate" method for Combobox accessible objects. Updated logic for ComboBox.ComboBoxChildNativeWindow. Now we return our ComboBoxChildEditUiaProvider instead of the native.
- Fixed unit tests
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon force-pushed the Issue-3654_combobox_hierarchy_is_broken_for_list_with_items branch from 48f1192 to 0f260e2 Compare November 10, 2020 09:47
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon added waiting-review This item is waiting on review by one or more members of team and removed waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Nov 13, 2020
@SergeySmirnov-Akvelon
Copy link
Contributor Author

SergeySmirnov-Akvelon commented Nov 13, 2020

CTI approved

Tanya-Solyanik
Tanya-Solyanik reviewed Nov 19, 2020
View reviewed changes
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. I added some nits.

  • Please clarify, why we are not testing if Handle was created everywhere before creating providers or accessible object?
  • And please add more details about what controls were tested manually - toolstrip combo item, DGV combo column? Had you tested all combo boxes from the test app? Particularly the data-bound one? Is MSAA still good?

@vladimir-krestov - Had you reviewed this change?

vladimir-krestov
vladimir-krestov reviewed Nov 20, 2020
View reviewed changes
Copy link
Contributor

@vladimir-krestov vladimir-krestov left a comment

Choose a reason for hiding this comment

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

Good work! Added cosmetic review points

@SergeySmirnov-Akvelon
Copy link
Contributor Author

SergeySmirnov-Akvelon commented Nov 20, 2020 •
edited
Loading

Hi @Tanya-Solyanik. I verified manually ToolStripCombobox, DataGridViewComboBoxColumn and comboboxes from the test application including the data-bound combobox. We found no change in MSAA behavior because MSAA uses native controls and does not call the "FragmentNavigate" method. During the refactoring, we have made a lot of new changes, so I suggest to send a fix to CTI team once again after fixing review notes

vladimir-krestov
vladimir-krestov reviewed Nov 21, 2020
View reviewed changes
Copy link
Contributor

@vladimir-krestov vladimir-krestov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Little points. In the rest, looks good to me

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon force-pushed the Issue-3654_combobox_hierarchy_is_broken_for_list_with_items branch from b089a0f to d550bb9 Compare November 23, 2020 07:19
RussKie
RussKie approved these changes Nov 26, 2020
View reviewed changes
Copy link
Contributor

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

:shipit:

@RussKie RussKie merged commit 8e86ffb into dotnet:master Nov 26, 2020
@RussKie RussKie deleted the Issue-3654_combobox_hierarchy_is_broken_for_list_with_items branch November 26, 2020 04:53
@ghost ghost added this to the 6.0 Preview1 milestone Nov 26, 2020
@RussKie RussKie removed the waiting-review This item is waiting on review by one or more members of team label Nov 26, 2020
@Olina-Zhang Olina-Zhang mentioned this pull request Dec 24, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@Tanya-Solyanik Tanya-Solyanik Tanya-Solyanik left review comments

@vladimir-krestov vladimir-krestov vladimir-krestov left review comments

@RussKie RussKie RussKie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon

Labels

None yet

Projects

None yet

Milestone

6.0 Preview1

Development

Successfully merging this pull request may close these issues.

Accessibility: The list item of comboBox incorrectly displayed in inspect tree when setting its DropDownStyle as DropDown or DropDownList.

4 participants

@SergeySmirnov-Akvelon @RussKie @Tanya-Solyanik @vladimir-krestov