Skip to content

Conversation

@staskus
Copy link
Contributor

@staskus staskus commented Dec 13, 2022

Fixes #19727

Description

Not reproducible:

  • On the Views & Visitors card, the “Weeks >” button is inaccessible by VoiceOver
  • After the app returns to the Insights tab after a card is added, VoiceOver reads the “Add Stats Card” button instead of reading the card that was added (or not reading anything).

Testing instructions

See #19727

Regression Notes

  1. Potential unintended areas of impact

None

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

None

  1. What automated tests I added (or what prevented me from doing so)

None

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Images & Videos

No results view action button visible for voiceover

accessibility.for.stats.card.mov

Week button visible for voiceover (issue that wasn't reproduced)

week.button.views.and.visitors.card.mov

Reading appropriate hint depending on a row belonging in an inactive or active section

stat.already.displayed.in.the.stats.mov

Reselecting row for voiceover when a card is moved from active to inactive section

https://user-images.githubusercontent.com/4062343/207818131-1e9006f2-c232-4a15-999d-794c17f5491a.mov
https://user-images.githubusercontent.com/4062343/207818177-580cb690-442d-4ab6-9ab7-0722c35ad950.mp4

Coming back to an empty Insights view - back button selected (issue that wasn't reproduced)

back.button.selected.mov

After we remove a row from section, iOS automatically selects another row that is reused for voiceover and automatically reads it. Using UIAccessibility.post API to manually select a row that was moved to another section.
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

When no cards are shown on the Insights tab, the “Add stats cards” button is shown but isn’t selectable via VoiceOver.

This looks good now, I can select this button using VoiceOver 👍

On the Views & Visitors card, the “Weeks >” button is inaccessible by VoiceOver.

Like you, I couldn't reproduce this either, I tested on trunk.

On the Manage Stats Cards screen, when adding a card to the ACTIVE CARDS list, VoiceOver jumps seemingly randomly to focus on other cards in the list.

This still happens for me. When moving cards from one section to another, VoiceOver focus switches to a seemingly random card before landing on the correct one. I see this in your "Reselecting row for voiceover when a card is moved from active to inactive section" screen recording. When removing Total Likes, the focus temporarily switches to "All-Time" before landing on Total Likes.

After the app returns to the Insights tab after a card is added, VoiceOver reads the “Add Stats Card” button instead of reading the card that was added (or not reading anything).

Again, like you, I couldn't reproduce this either, I tested on trunk.

@staskus
Copy link
Contributor Author

staskus commented Dec 14, 2022

Thanks for testing, @guarani!

This still happens for me. When moving cards from one section to another, VoiceOver focus switches to a seemingly random card before landing on the correct one.

Yes, that is the correct observation. My previous fix would only refocus on the correct row after the switch. The issue happens because we call tableView.reloadData on the whole table, and voiceover refocuses on one of the reused cells. It does not know that we moved that particular cell to a different section. To fix this issue properly, we need to manually move/delete/insert rows, which allows voiceover to understand what to focus on.

Check dae28bf commit and new videos under Reselecting row for voiceover when a card is moved from active to inactive section section.

Bonus: since we are moving the rows, we get a nice animation.

@staskus staskus requested a review from guarani December 14, 2022 12:23
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 14, 2022

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19773-dc79613 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 14, 2022

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19773-dc79613 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@sla8c
Copy link
Contributor

sla8c commented Dec 14, 2022

@staskus I've tested this and I've found an issue - after moving a row from the active cards section to the inactive section the accessibility hint is not updated and the voiceover still says "Stat is already displayed in Insights".

This is also an issue when moving from inactive section to active section (voiceover says "Select to add this Stat to insights")

I've made a recording:

AccessiblityHintNotUpdated.mp4

@staskus
Copy link
Contributor Author

staskus commented Dec 14, 2022

@staskus I've tested this and I've found an issue - after moving a row from the active cards section to the inactive section the accessibility hint is not updated and the voiceover still says "Stat is already displayed in Insights".

Great catch! iOS doesn't reload data when using .moveRow and therefore the hint is not updated. I'll make a fix, it may require not using .moveRow (and animation that comes with it) if I don't find a solution.

@guarani
Copy link
Contributor

guarani commented Dec 15, 2022

Thanks for the update @staskus!

I'll make a fix, it may require not using .moveRow (and animation that comes with it) if I don't find a solution.

I saw you re-requested a review from me and then @sla8c reviewed and found an issue. Would you prefer I go ahead with reviewing or hold off until you say this is ready?

@staskus
Copy link
Contributor Author

staskus commented Dec 15, 2022

Would you prefer I go ahead with reviewing or hold off until you say this is ready?

@guarani I'll re-request once it's ready.

@staskus staskus removed request for guarani and sla8c December 15, 2022 07:25
@staskus
Copy link
Contributor Author

staskus commented Dec 15, 2022

@staskus I've tested this and I've found an issue - after moving a row from the active cards section to the inactive section the accessibility hint is not updated and the voiceover still says "Stat is already displayed in Insights"

I made a fix, reloading the data of the moved row so the accessibility data would update.

@staskus staskus requested review from guarani and sla8c December 15, 2022 09:07
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

This is working great! I verified the following:

  • the issue that @sla8c caught earlier is fixed
  • the rows read their state correctly when tapped
  • rows that move between active/inactive sections retain their VoiceOver selection rectangle
  • the rows in the inactive section retain their ordering
  • placeholder rows behave as expected

I like the new table view updates which look smooth and play nicely with VoiceOver as you mentioned above.

@staskus staskus merged commit a37dd79 into trunk Dec 15, 2022
@staskus staskus deleted the fix/19727-stats-revamp-voiceover-issues branch December 15, 2022 14:12
@guarani guarani modified the milestones: 21.5, 21.4 Dec 15, 2022
@staskus staskus mentioned this pull request Apr 4, 2024
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[iOS] Stats: VoiceOver issues

5 participants