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

[Gutenberg] Add AccessibilityLabel and hint to AztecViewManager #2368

Merged
merged 3 commits into from
Jul 27, 2020

Conversation

enejb
Copy link
Contributor

@enejb enejb commented Jun 10, 2020

Fixes #2364

Related GB PR: WordPress/gutenberg#23057

To test:

  1. Download WordPress-iOS.
  2. Make sure that he WordPress-iOS repo is on the same level as this directory. (gutenberg-mobile)
  1. Run the following command on the WordPress-iOS repo
$ export LOCAL_GUTENBERG=1
bundle exec pod update
  1. Load this PR.
  2. Build the iOS app using xcode and send it open it on your device.
  3. Navigate to the Pages screen.
  4. Enable VoiceOver using Siri.
  5. Navigate to a new post.

Before:
See https://cloudup.com/cNlMs6Fbu_F

After: You should get this experience
https://cloudup.com/cjd-RHZpyQM

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 10, 2020

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@enejb enejb self-assigned this Jun 10, 2020
@enejb enejb requested review from chipsnyder and pinarol June 12, 2020 07:26
@enejb enejb changed the title Add AccessibilityLable and hing to ActecViewManager Add AccessibilityLable and hing to AztecViewManager Jun 12, 2020
@enejb enejb changed the title Add AccessibilityLable and hing to AztecViewManager Add AccessibilityLabel and hing to AztecViewManager Jun 12, 2020
@enejb enejb changed the title Add AccessibilityLabel and hing to AztecViewManager Add AccessibilityLabel and hint to AztecViewManager Jun 12, 2020
@pinarol pinarol requested review from etoledom and removed request for pinarol June 12, 2020 09:07
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Adding some comments about adding extra labels and hints to Aztec's UITextView.
The main PR review is here: WordPress/gutenberg#23057 (review)

Comment on lines 19 to 21
RCT_EXPORT_VIEW_PROPERTY(accessibilityLabel, NSString)
RCT_EXPORT_VIEW_PROPERTY(accessibilityHint, NSString)

Copy link
Contributor

Choose a reason for hiding this comment

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

For this to work we should use RCT_REMAP_VIEW_PROPERTY instead.
But my question is, do we want to add extra labels and hints to the Aztec TextView?

Personally, I think it's better for the system to handle accessibility on UITextViews since it's quite complex.

As an example, adding the current label and hint, the VoiceOver will read on Aztec's empty post title:

Label: "Title, Empty. TextField, is editing. 0 width space, character mode, insertion point at end.
Hint: "Update the title. Use the rotor to access misspelled words."

Maybe we are complicating the already complex TextView labels and hint, it might get in the way of the users instead of helping.

Having this label and hint in the wrapper view should be enough to give the user the info needed, then they can just start editing in the TextView after performing the action.

What do you think? @enejb

Copy link
Contributor

@etoledom etoledom Jun 12, 2020

Choose a reason for hiding this comment

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

Ok, now it makes sense to me on a new post where the title is automatically focused, not having this is confusing. 🤔

In this case probably just the label is enough for the TextView?

And for some reason, even without this change is still working for me. (maybe that's why RCT_EXPORT_VIEW_PROPERTY also works).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I am not sure what the best approach is here. To me it also sounds like lot of text.

I just wanted to make sure that the user know that they are editing a the post title when they get dropped in creating a new post or page.

Copy link
Contributor

@etoledom etoledom Jun 12, 2020

Choose a reason for hiding this comment

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

For this to work we should use RCT_REMAP_VIEW_PROPERTY instead.

So, this was a mistake on my part:

RCT_EXPORT_VIEW_PROPERTY will work just fine, and the property will be set to the view's accessibilityLabel or accessibilityHint property as expected.

RCT_REMAP_VIEW_PROPERTY is used when the JS name doesn't match the View's property name. This gives an opportunity to re-name the property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remove the RCT_EXPORT_VIEW_PROPERTY since it wasn't doing anything.

@enejb enejb changed the title Add AccessibilityLabel and hint to AztecViewManager [Gutenberg] Add AccessibilityLabel and hint to AztecViewManager Jun 12, 2020
@enejb enejb force-pushed the add/accessibility-label-to-rich-text branch from ec83b3f to 4775a9c Compare July 3, 2020 13:36
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

@enejb enejb force-pushed the add/accessibility-label-to-rich-text branch from 4775a9c to 540a419 Compare July 27, 2020 12:22
@enejb enejb merged commit 04a39c0 into develop Jul 27, 2020
@enejb enejb deleted the add/accessibility-label-to-rich-text branch July 27, 2020 14:02
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.

When the page/post title is selected tell the user about that they are edditing the post title
2 participants