Skip to content

Adding AccessibleObject for ListViewItem's image#7672

Merged
Tanya-Solyanik merged 2 commits intodotnet:mainfrom
NikitaSemenovAkvelon:Issue_4690-Added_ListViewItem_ListViewItemImageAccessibleObject
Sep 26, 2022
Merged

Adding AccessibleObject for ListViewItem's image#7672
Tanya-Solyanik merged 2 commits intodotnet:mainfrom
NikitaSemenovAkvelon:Issue_4690-Added_ListViewItem_ListViewItemImageAccessibleObject

Conversation

@NikitaSemenovAkvelon
Copy link
Copy Markdown
Contributor

@NikitaSemenovAkvelon NikitaSemenovAkvelon commented Aug 26, 2022

Fixes #4690 Details.Case 2

Proposed changes

  • Added ListViewItemImageAccessibleObject
  • Used AO for image inside ListViewItemDetailsAccessibleObject
  • Added unit tests

Customer Impact

  • User can see information about ListViewItem's image via accessibility tools

Regression?

  • No

Risk

  • Minimal

Screenshots

Before

image

After

image

Test methodology

  • Manually
  • Unit tests
  • AI

Accessibility testing

  • AI

Test environment(s)

  • 7.0.100-preview.7.22377.5
Microsoft Reviewers: Open in CodeFlow

@NikitaSemenovAkvelon NikitaSemenovAkvelon requested a review from a team as a code owner August 26, 2022 05:39
@NikitaSemenovAkvelon NikitaSemenovAkvelon changed the title Adding AccessibleObject for ListViewItem Adding AccessibleObject for ListViewItem's image Aug 26, 2022
Copy link
Copy Markdown
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, please see my comments.

@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_4690-Added_ListViewItem_ListViewItemImageAccessibleObject branch from 60fb54a to d83b172 Compare August 29, 2022 06:30
Copy link
Copy Markdown
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.

It looks really complex. I would consider not using any index for the image AO at all. It will allow to keep the code more clear, we will just insert one object. But I will check if it work

@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_4690-Added_ListViewItem_ListViewItemImageAccessibleObject branch from d83b172 to ad165f0 Compare August 30, 2022 04:24
<value>Gets or sets the parameter that is passed to the Command property's object on execution or on execution context request.</value>
</data>
<data name="ListViewItemImageAccessibleObjectDescription" xml:space="preserve">
<value>Image of the ListViewItem.</value>
Copy link
Copy Markdown
Contributor

@Tanya-Solyanik Tanya-Solyanik Aug 30, 2022

Choose a reason for hiding this comment

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

Does WPF have anything similar? I don't think that the current description is useful. Can we use "Image of <Item AccessibleName>" instead? Or even leave it empty?

@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_4690-Added_ListViewItem_ListViewItemImageAccessibleObject branch 3 times, most recently from 5be2f78 to 7a91efe Compare August 30, 2022 05:44
@RussKie
Copy link
Copy Markdown
Contributor

RussKie commented Aug 30, 2022

The resources are out of sync. Please rebase on top of the latest main.

@RussKie RussKie added the waiting-author-feedback The team requires more information from the author label Aug 30, 2022
@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_4690-Added_ListViewItem_ListViewItemImageAccessibleObject branch from 7a91efe to e90041a Compare August 30, 2022 06:24
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Aug 30, 2022
@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_4690-Added_ListViewItem_ListViewItemImageAccessibleObject branch from e90041a to d8e423e Compare August 30, 2022 06:32
Copy link
Copy Markdown
Contributor

@dmitrii-drobotov dmitrii-drobotov left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Tanya-Solyanik
Copy link
Copy Markdown
Contributor

It looks really complex.

I agree with Vladimir, maybe we can unify to having only one kind of index in the code and convert it right before it's returned to the accessibility tools. I'm not sure if conversion methods are much help ☹️

@vladimir-krestov
Copy link
Copy Markdown
Contributor

vladimir-krestov commented Aug 30, 2022

I tried to rework it, but GetChild should return the image AO and subItems AO. So subItems start from 0, and there is only one way - to use -1 index for image AO child, but it doesn't looks correct. So we have to implement this converting algorithm. It will help us to insert checkBox AO if we will want.
One more point, @Tanya-Solyanik, may we name the Image AO as "sub item" and put inside GetDetailsSubItemOrFakeInternal? It is not a subitem and should not be here, in my opinion. But I didn't find a fast way to rework this implementation much better.

@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_4690-Added_ListViewItem_ListViewItemImageAccessibleObject branch from d8e423e to de392c2 Compare August 31, 2022 07:57
@Tanya-Solyanik
Copy link
Copy Markdown
Contributor

Tanya-Solyanik commented Aug 31, 2022

may we name the Image AO as "sub item" and put inside GetDetailsSubItemOrFakeInternal?

If you think this change makes the code more readable, then sure. I'm concerned that when this code is maintained by somebody else, they would not be able to distinguish one index from the other.

Copy link
Copy Markdown
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 added some suggestions for naming.

@Tanya-Solyanik
Copy link
Copy Markdown
Contributor

@dmitrii-drobotov - did you have a chance to re-review the latest?

@Tanya-Solyanik Tanya-Solyanik added this to the 8.0 Preview1 milestone Sep 17, 2022
Used AO for image inside ListViewItemImageAccessibleObject
Added unit tests
@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_4690-Added_ListViewItem_ListViewItemImageAccessibleObject branch from de392c2 to dea0733 Compare September 19, 2022 06:00
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Sep 19, 2022
Tanya-Solyanik
Tanya-Solyanik previously approved these changes Sep 20, 2022
@Tanya-Solyanik
Copy link
Copy Markdown
Contributor

@dmitrii-drobotov - please re-review this change.

@Tanya-Solyanik Tanya-Solyanik 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 Sep 20, 2022
@Tanya-Solyanik
Copy link
Copy Markdown
Contributor

@NikitaSemenovAkvelon - please send this change to testing.

Copy link
Copy Markdown
Contributor

@dmitrii-drobotov dmitrii-drobotov 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!

@NikitaSemenovAkvelon
Copy link
Copy Markdown
Contributor Author

CTI detected an error that image bounds don't draw on mouse navigation in AI/Inspect. I've added a fix in a separate commit to clarity. After reviewing I'll squash it with the previous one.

Tanya-Solyanik
Tanya-Solyanik previously approved these changes Sep 21, 2022
Copy link
Copy Markdown
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.

Please fix the tests.

@Tanya-Solyanik Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Sep 21, 2022
@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_4690-Added_ListViewItem_ListViewItemImageAccessibleObject branch from f556107 to 7888516 Compare September 22, 2022 05:00
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Sep 22, 2022
@Olina-Zhang Olina-Zhang removed the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Sep 26, 2022
@Olina-Zhang
Copy link
Copy Markdown
Member

Tested this PR fixing with no new issue found finally.

@Tanya-Solyanik Tanya-Solyanik merged commit 39bb168 into dotnet:main Sep 26, 2022
v-elnovikova pushed a commit to v-elnovikova/winforms that referenced this pull request Oct 18, 2022
* Added ListViewItemImageAccessibleObject
Used AO for image inside ListViewItemImageAccessibleObject
Added unit tests
@ghost ghost locked as resolved and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Epic - ListViewSubItems in the ListView have incorrect behavior

6 participants