Autocomplete: duplicate list within iframe for non visual users #47907
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
ellatrix
commented
Feb 9, 2023
|
Size Change: +93 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 9550337. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4232249635
|
|
@alexstine Sorry, could you test again? 😅 I removed the duplication when we're not rendering in an iframe otherwise we have duplicate IDs. There's a tiny delay now cause of the check, so I'm curious if it still works. If not there's other ways to fix it. I also made e2e test modifications: the autocomplete tests are now run in a block based theme and I checked if ariaOwns and ariaActiveDecsendant are correct. |
|
And with that the PR is ready for review. |
kevin940726
reviewed
Feb 13, 2023
Comment on lines
+118
to
+124
| // Ensure `aria-owns` is part of the same document and ensure the | ||
| // selected option is equal to the active descendant. | ||
| await expect( | ||
| await editor.canvas | ||
| .locator( `#${ ariaOwns } [aria-selected="true"]` ) | ||
| .getAttribute( 'id' ) | ||
| ).toBe( ariaActiveDescendant ); |
There was a problem hiding this comment.
This is probably a better fit in a unit test than an e2e test. Could it be done in a unit test environment giving the current architecture?
There was a problem hiding this comment.
Why should it be a unit test? We're checking if it's correct for an iframe because we need to duplicate the markup inside the iframe.
There was a problem hiding this comment.
It's just my preference since unit tests are faster and more reliable for this kind of task. I don't think we need anything from a real browser to perform this test. That said, it can still be tested in e2e tests if you prefer it though 🙂 . Feel free to ignore!
There was a problem hiding this comment.
Here's some more context to why Autocomplete has mostly e2e tests instead of unit tests :)
There was a problem hiding this comment.
Thanks for the context! I would probably still prefer an integration test (rendering both AutoComplete and RichText without mocking), but nvm if it's too much trouble to set up.
There was a problem hiding this comment.
Agreed — we just went for what we thought was the best trade-off between required efforts and results.
Out of curiosity, where would those tests live in the repo?
There was a problem hiding this comment.
@glendaviesnz had some ideas about adding more integration tests to the repo. I haven't put many thoughts into it yet 😅 , so I'll just shamelessly defer this question to him 😆 .
There was a problem hiding this comment.
It is still on my to-do list to look at ways that we can more easily provide for integration level testing for blocks, etc. rather than having to rely on e2e, eg. some way of testing the interaction between block inspector/toolbar/edit components without having to fire up a full WP instance. I am hoping to take more of a look at this once 6.2 is sorted.
|
I'd like to take a look at this PR but I didn't find the time today (and tomorrow I'll be AFK). I'll take a look on Wednesday, if it's not an issue |
alexstine
approved these changes
Feb 14, 2023
ciampo
reviewed
Feb 15, 2023
There was a problem hiding this comment.
Great work here, @ellatrix ! And thank you for waiting for my review :D I think this solution works well! Also, the fact that it doesn't change the public APIs of the component is also a plus!
Regarding the failing unit test, they are happening because the unit test code passes an empty contentRef prop to AutocompleterUI. As much as we should improve the unit test to resemble the actual usage of the component, in the component itself we should also check that the contentRef.current is not empty before using it as a container for the portal.
Applying these checks should make the component more resilient, and should also make the unit tests pass 🤞
diff --git a/packages/components/src/autocomplete/autocompleter-ui.js b/packages/components/src/autocomplete/autocompleter-ui.js
index d8875060f9..833ebe44ab 100644
--- a/packages/components/src/autocomplete/autocompleter-ui.js
+++ b/packages/components/src/autocomplete/autocompleter-ui.js
@@ -53,7 +53,10 @@ export function getAutoCompleterUI( autocompleter ) {
const popoverRefs = useMergeRefs( [
popoverRef,
useRefEffect( ( node ) => {
- if ( node.ownerDocument !== contentRef.current.ownerDocument ) {
+ if (
+ !! contentRef.current?.ownerDocument &&
+ node.ownerDocument !== contentRef.current?.ownerDocument
+ ) {
setNeedsA11yCompat( true );
}
} ),
@@ -115,7 +118,7 @@ export function getAutoCompleterUI( autocompleter ) {
{ needsA11yCompat &&
createPortal(
<ListBox Component={ VisuallyHidden } />,
- contentRef.current.ownerDocument.body
+ contentRef.current?.ownerDocument.body
) }
</>
);Finally, could you also rebase and add a CHANGELOG entry ?
Thank you 🙏
ciampo
reviewed
Feb 15, 2023
| const popoverRef = useRef(); | ||
| const popoverRefs = useMergeRefs( [ | ||
| popoverRef, | ||
| useRefEffect( ( node ) => { |
There was a problem hiding this comment.
Why is there a need for useRefEffect ? Can't we just use a "vanilla" ref callback function?
There was a problem hiding this comment.
We're using useRefEffect because it's easier to understand, it works similarly to useEffect so you don't need to handle null.
ciampo
reviewed
Feb 15, 2023
|
Thanks, I address your feedback. |
ellatrix
force-pushed
the
fix/autocomplete-iframe-a11y
branch
from
a628969
to
ba94657
Compare
ciampo
approved these changes
Feb 15, 2023
kevin940726
reviewed
Feb 16, 2023
| await page.keyboard.type( '@fr' ); | ||
| await expect( | ||
| page.locator( 'role=option', { hasText: 'Frodo Baggins' } ) | ||
| ).toBeVisible(); | ||
| await page.click( '[data-type="core/paragraph"]' ); | ||
| await editor.canvas.click( '[data-type="core/paragraph"]' ); | ||
| await expect( | ||
| page.locator( 'role=option', { hasText: 'Frodo Baggins' } ) | ||
| ).not.toBeVisible(); |
There was a problem hiding this comment.
Tests are now run in the iframe, so yes.
ellatrix
force-pushed
the
fix/autocomplete-iframe-a11y
branch
from
ba94657
to
721fbfc
Compare
ellatrix
force-pushed
the
fix/autocomplete-iframe-a11y
branch
from
721fbfc
to
9550337
Compare
ellatrix
commented
Feb 21, 2023
| await expect( | ||
| page.locator( 'role=option', { hasText: 'Frodo Baggins' } ) | ||
| ).toBeVisible(); | ||
| await page.click( '[data-type="core/paragraph"]' ); | ||
| // Use the strong tag to move the selection by mouse within the mention. | ||
| await editor.canvas.click( '[data-type="core/paragraph"] strong' ); |
There was a problem hiding this comment.
This test was actually passing for the wrong reason. The autocomplete component has an on click outside handler that hides the popover, which doesn't work in the iframe when clicking in a block.
In the test we should perform a click that changes the selection but still within the mention.
|
I'm afraid this fix doesn't work with Safari and VoiceOver. I'll try a more in-depth technical debugging later but I'd say duplicating the Listbox seems to be very far from ideal in the first place. On WP 6.1 everything works as expected:
On latest trunk, nothing is announced:
|
|
Okay, sounds like we'll need to revert this PR and go back to the drawing board. Sorry about the lack of testing on Mac from me. I tested this well for Windows. |
DaisyOlsen
added
the
[Focus] Accessibility (a11y)
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.
What?
The iframing of the editor broke the
aria-relationship between the input field (inside the iframe) and the list box (outside the iframe). Since this API is ID based, we have no other choice than to create a duplicate list within the iframe. We must also leave a list outside the iframe so it will be styled as admin UI and not content.Why?
Fixes #47767.
How?
I used slot/fill for now but I may be able to create a cleaner solution without passing additional props: check if the contentRef is in a different document and then create a portal mounting at the body.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast