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.

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

Comment Detail: Fix content cell elements not resizing in AX size #17464

Merged
merged 2 commits into from
Nov 14, 2021

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Nov 12, 2021

Refs #17087

As titled, this PR now ensures that:

  • The content cell header properly displays the text under increased font size.
  • The Reply and Like buttons' sizes are adjusted along with the device's font size.

⚠️ Auto-merge is enabled. ⚠️


To note: I've tried to fix an issue with VoiceOver but to no avail. The screen reader somehow jumps to the top of the screen again after reading the contents of the web view. It looks like WKWebView is buggy (just my luck), and after loading the HTML content, somehow the accessibility parent of the web view got reassigned. I believe this is what caused the screen reader to "jump".

Screen Shot 2021-11-12 at 23 48 14

The highlighted element is the table view container. As you can see, there are five elements. The first one is the comment header, the second one is the content cell, and so on. Next is the hierarchy for the content cell:

Screen Shot 2021-11-12 at 23 49 01

Above is the content cell element unwrapped. You can see all the accessible elements there in the content cell1, and the one that I highlighted is a group for the web view. Next, I'll go deeper into the web view and this is what I got:

Screen Shot 2021-11-12 at 23 49 21

"Hey there." is the comment content. As you can see, it's somehow nested right under SimulatorKit.SimDisplayRenderableView, which is almost at the very top of the hierarchy. This is why the screen reader won't continue after reading the contents of the web view. Sigh.

I have tried a lot of things to fix this and to the point of accessing the private WKContentView class of WKWebView to see if I could apply some hack or something, but nope. This gives me more reason to create a standalone, reproducible project and submit a radar. 😅

To test

  • Increase device/simulator font size.
  • Enable newCommentDetail feature flag.
  • Go to My Site > Comments and select any comment.
  • 🔍 Verify that the content cell is displayed correctly, adjusting the contents' sizes with the device's current font size.

Regression Notes

  1. Potential unintended areas of impact
    n/a. Feature is unreleased.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    n/a. Feature is unreleased.

  3. What automated tests I added (or what prevented me from doing so)
    n/a. Feature is unreleased.

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.

Footnotes

  1. There's also a strange ordering issue here. Somehow the web view element is placed after the Reply and Like button. 🤔

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 12, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 12, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@ScoutHarris
Copy link
Contributor

ScoutHarris commented Nov 12, 2021

With large text (in this case, Accessibility Medium) it looks like the label stack view isn't growing properly. Sometimes the name is cut off, sometimes the date is.

Screen Shot 2021-11-12 at 12 11 09 PM

Screen Shot 2021-11-12 at 12 10 49 PM

I'll go ahead and approve (disabling auto-merge) so it can be fixed and merged to make the release. Or if you like, it can fixed separately and this can just be merged.

@ScoutHarris ScoutHarris disabled auto-merge November 12, 2021 19:20
@dvdchr
Copy link
Contributor Author

dvdchr commented Nov 14, 2021

With large text (in this case, Accessibility Medium) it looks like the label stack view isn't growing properly.

Thank you for noticing this @ScoutHarris !! I swear I've had this fixed yesterday 😅. Oh well. This should now be addressed in c3d47ec. Here are some screenshots:

Normal AX Medium AX Large
header_normal header_axmedium header_axlarge

@dvdchr dvdchr enabled auto-merge November 14, 2021 16:31
@dvdchr dvdchr merged commit ade92ad into develop Nov 14, 2021
@dvdchr dvdchr deleted the issue/17087-content-cell-a11y-size branch November 14, 2021 16:53
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.

2 participants