[RNMobile] Search Block: Add UI Tests and make available to production! #30783
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Had to take the search block out of devOnly mode for this to work.
AmandaRiu
added
[Block] Search
2 tasks
|
Size Change: +25.2 kB (+2%) Total Size: 1.49 MB
ℹ️ View Unchanged
|
- Re-evaluate during orientation changes as well
…earch-block-ui-tests
AmandaRiu
changed the title
AmandaRiu
force-pushed
the
rnmobile/search-block-ui-tests
branch
from
cbaba3f
to
fe44004
Compare
dcalhoun
reviewed
Apr 20, 2021
packages/react-native-editor/__device-tests__/pages/editor-page.js
Outdated
Show resolved
Hide resolved
Co-authored-by: David Calhoun <[email protected]>
|
Ready for another review! |
|
Special Note: Since the last few commits made changes to the language of the |
dcalhoun
reviewed
Apr 21, 2021
There was a problem hiding this comment.
Special Note: Since the last few commits made changes to the language of the
accessibilityLabelwhich would effect how a screenreader may work, I tested VoiceOver and Talkback on physical devices to ensure no functionality was negatively affected.
I also tested this round of changes on an iPhone SE and Samsung Galaxy S20. Both performed as expected, aside from one issue that is related to an issue we discussed once before — the auto-focus of the search input. I wanted to at least showcase the issue I encountered incase we want to capture it in separate issue. The fact that the erroneous search input auto-focus breaks the focus of the bottom sheet makes this a bigger issue from my perspective.
Steps to Reproduce
- Launch block editor.
- Add Search block.
- Enable VoiceOver.
- Open block settings.
Expected Outcome: The bottom sheet is focused, and focus is trapped within the bottom sheet modal until it is dismissed.
Actual Outcome: The search input is focused. A user would have to known the bottom sheet is open and tap it to move focus to its elements, which may be difficult to do if one is visual impaired.
Screenshot of issue
RPReplay_Final1619037970.MP4
packages/block-library/src/search/test/__snapshots__/edit.native.js.snap
Outdated
Show resolved
Hide resolved
packages/react-native-editor/__device-tests__/pages/editor-page.js
Outdated
Show resolved
Hide resolved
|
Thanks for the additional review. I've answered all the code comments inline and made some changes in response. Regarding:
I tested this as well, but I do not believe this is an issue with the Search block directly. Opening any block's settings while a screenreader is active does not place the focus in the bottom sheet. The user is forced to swipe to navigate to it. I think this is likely a larger problem with maybe the way the bottom sheet is displayed and beyond the scope of this PR. Ready for another round, thank you! |
dcalhoun
approved these changes
Apr 23, 2021
There was a problem hiding this comment.
This looks good! 
I tested this as well, but I do not believe this is an issue with the Search block directly. Opening any block's settings while a screenreader is active does not place the focus in the bottom sheet. The user is forced to swipe to navigate to it. I think this is likely a larger problem with maybe the way the bottom sheet is displayed and beyond the scope of this PR.
Ah, thank you for verifying this. Definitely seems unrelated to this work.
Since the search block fields are already empty, accept whatever the default value for `clear` which is `true` for Android and `false` for iOS. Also add a sleep when changing animated settings options on Android to avoid flakiness.
empty before adding text.
|
Made some additional improvements to the UI tests after finding they were failing on iOS only when run from CircleCI. For some reason the test wasn't getting properly replaced so the expect() statements were failing. To get around this odd flakiness that doesn't happen when run locally for anyone who has tested this PR, I've changed the set of tests around modifying the text in elements of the Search block. For those tests I now manually insert a Search block with all text elements blank. This way it doesn't matter if the existing text is replaced properly or not because there is no existing text. I also hardened the tests so if one of the text-tests fail it doesn't fail them all. I've verified these tests now run successfully not only locally, but also on CircleCI. Ready for a final round @dcalhoun ! |
dcalhoun
approved these changes
Apr 26, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#3210
Description
This PR adds automated UI tests for the Search block and is the final PR in a series to port the Search block to mobile. Includes the following:
devOnlymode.testIDproperties to the Search block label, input and button elements so they can be accessed reliably by Appium when running on iOS.testID's exist so they are available later for UI testing. Also updated the snapshots to include these new properties.How has this been tested?
Test this PR by first running the unit test:
Then run the UI tests for this block:
IOS
Android
Screenshots
N/A
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).