-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve VoiceOver experience when reordering menus #18155
Conversation
d4fc778
to
faed559
Compare
faed559
to
5c4e4be
Compare
You can test the changes in Jetpack from this Pull Request by:
.ipa file can also be downloaded directly here.If you need access to App Center, please ask a maintainer to add you. |
You can test the changes in WordPress from this Pull Request by:
.ipa file can also be downloaded directly here.If you need access to App Center, please ask a maintainer to add you. |
8a6463f
to
f7275d5
Compare
f7275d5
to
7cbfaf0
Compare
I'm going to bump this to the next release because we'll be code freezing 19.6 next week and this is still a draft. It doesn't seem the case, but if it's very important that this makes it into this release, let me know and we'll organize a new beta once ready. |
Thanks @mokagio, 19.7 works just fine. |
I think this should say "Item C", but I'm loving how neatly set out the test cases are. |
Thanks @guarani! This is now fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested using build pr18155-c41c600
and everything works as expected, nice work @twstokes!
- Test 1 - Move Item C to be a child of Item A, under Item B
- Test 2 - Move Item A to be a child of Item B
- Test 3 - Move Item B to become a sibling of its parent at the top level
- Test 4 - Move Item C to become a sibling of its parent
- Test 5 - Move Item A after Item B
- Test 6 - Move Item B above Item A
- Test 7 - Hover over the main button
- Test 8 - Hover over the drag handle
- Test 9 - Saving reordered menu items
let entityName = MenuItem.classNameWithoutNamespaces() | ||
let entity = NSEntityDescription.insertNewObject(forEntityName: entityName, into: context) | ||
|
||
let menuItem = entity as! MenuItem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick comment] XCTUnwrap(entity as? MenuItem)
could be used here, but I'm not sure if it fits this context since this is a helper method and if the force-cast failed, it would be due to a programming error rather than a test failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good thought @guarani! Since the type shouldn't change and this is in the context of a test, I'm considering letting it stay as is. I believe this could only fail if some really bad stuff happened under the hood with Core Data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Closes #17121
Description
This PR aims to improve the VoiceOver experience when reordering menu items:
Before.MP4
After.MP4
Testing
ℹ️ Note: It can be helpful to enable triple-clicking to toggle VoiceOver.
Prerequisites:
Preconditions for each test:
Test 1 - Move Item C to be a child of Item A, under Item B
Before:
After:
Expected VO (VoiceOver): "Child of [Item A], after [Child B]"
Test 2 - Move Item A to be a child of Item B
Before:
After:
Expected VO: "Child of [Item B]"
Note: If an Item C exists after Item B that Item A is inserted above, we still only expect "Child of [Item B]" and not "Child of [Item B] above [Item C]."
Test 3 - Move Item B to become a sibling of its parent at the top level
Before:
After:
Expected VO: "Top level. After [Item A]"
Test 4 - Move Item C to become a sibling of its parent
Before:
After:
Expected VO: "Child of [Item A]. After [Item B]"
Test 5 - Move Item A after Item B
Before:
After:
Expected VO: "After [Item B]"
Test 6 - Move Item B above Item A
Before:
After:
Expected VO: "Before [Item A]"
If Item A is moved quickly to be a child of Item B below existing children, VO will not announce the new parent. This is due to multiple VO events firing in quick succession and it's using the latest VO string (which is equivalent to Test 5).
Example:
Before:
After:
Expected VO: "After [Item D]"
The same applies in the opposite direction:
If Item A is moved quickly to be a child of Item B above existing children, VO will not announce the new parent. This is because multiple VO events fired due to the quick succession and it's using the latest VO string which is equivalent to Test 6.
Test 7 - Hover over the main button
If a child of a menu item:
Expected VO: "[Name of item] child of [parent menu item]. Button. Edits this menu item."
If the top level (no parent):
Expected VO: "[Name of item] top level. Button. Edits this menu item."
Test 8 - Hover over the drag handle
Move the VoiceOver focus to the drag handle on the right of a menu item.
Expected VO: "Move [name of item]. Button. Double-tap to move this menu item up or down. Move horizontally to change hierarchy."
VO no longer announces "Dimmed" as the button state. More context.
Test 9 - Saving reordered menu items
Test cases:
Regression Notes
Functionality to the Menus view should be unchanged when VoiceOver is off.
What I did to test those areas of impact (or what existing automated tests I relied on)
I manually tested all tests above.
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txt
if necessary.