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.

Sign up for GitHub

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

Jump to bottom

Autocomplete: duplicate list within iframe for non visual users #47907

Merged
merged 7 commits into from Feb 21, 2023
Merged

Autocomplete: duplicate list within iframe for non visual users #47907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ea2eed7
Autocomplete: duplicate list within iframe for non visual users
ellatrix Feb 9, 2023
c975302
Use visually hidden
ellatrix Feb 9, 2023
2c330f5
Use portal instead of slot/fil
ellatrix Feb 10, 2023
fc19831
Add e2e test
ellatrix Feb 13, 2023
d1f628c
Revert mistake
ellatrix Feb 13, 2023
d94b7b7
Address feedback
ellatrix Feb 15, 2023
9550337
Fix autocomplete test
ellatrix Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@

- `ToolsPanel`: fix type inconsistencies between types, docs and normal component usage ([47944](https://github.com/WordPress/gutenberg/pull/47944)).
- `SelectControl`: Fix styling when `multiple` prop is enabled ([#47893](https://github.com/WordPress/gutenberg/pull/43213)).
- `useAutocompleteProps`, `Autocomplete`: Make accessible when rendered in an iframe ([#47907](https://github.com/WordPress/gutenberg/pull/47907)).

### Enhancements

Expand Down
106 changes: 72 additions & 34 deletions packages/components/src/autocomplete/autocompleter-ui.js
View file
Open in desktop
Expand Up @@ -6,15 +6,23 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { useLayoutEffect, useRef, useEffect } from '@wordpress/element';
import {
useLayoutEffect,
useRef,
useEffect,
useState,
} from '@wordpress/element';
import { useAnchor } from '@wordpress/rich-text';
import { useMergeRefs, useRefEffect } from '@wordpress/compose';

/**
* Internal dependencies
*/
import getDefaultUseItems from './get-default-use-items';
import Button from '../button';
import Popover from '../popover';
import { VisuallyHidden } from '../visually-hidden';
import { createPortal } from 'react-dom';

export function getAutoCompleterUI( autocompleter ) {
const useItems = autocompleter.useItems
Expand All @@ -40,7 +48,25 @@ export function getAutoCompleterUI( autocompleter ) {
value,
} );

const [ needsA11yCompat, setNeedsA11yCompat ] = useState( false );
const popoverRef = useRef();
const popoverRefs = useMergeRefs( [
popoverRef,
useRefEffect(
( node ) => {
if ( ! contentRef.current ) return;

// If the popover is rendered in a different document than
// the content, we need to duplicate the options list in the
// content document so that it's available to the screen
// readers, which check the DOM ID based aira-* attributes.
setNeedsA11yCompat(
node.ownerDocument !== contentRef.current.ownerDocument
);
},
[ contentRef ]
),
] );

useOnClickOutside( popoverRef, reset );

Expand All @@ -55,41 +81,53 @@ export function getAutoCompleterUI( autocompleter ) {
return null;
}

return (
<Popover
focusOnMount={ false }
onClose={ onReset }
placement="top-start"
className="components-autocomplete__popover"
anchor={ popoverAnchor }
ref={ popoverRef }
const ListBox = ( { Component = 'div' } ) => (
<Component
id={ listBoxId }
role="listbox"
className="components-autocomplete__results"
>
<div
id={ listBoxId }
role="listbox"
className="components-autocomplete__results"
{ items.map( ( option, index ) => (
<Button
key={ option.key }
id={ `components-autocomplete-item-${ instanceId }-${ option.key }` }
role="option"
aria-selected={ index === selectedIndex }
disabled={ option.isDisabled }
className={ classnames(
'components-autocomplete__result',
className,
{
'is-selected': index === selectedIndex,
}
) }
onClick={ () => onSelect( option ) }
>
{ option.label }
</Button>
) ) }
</Component>
);

return (
<>
<Popover
focusOnMount={ false }
onClose={ onReset }
placement="top-start"
className="components-autocomplete__popover"
anchor={ popoverAnchor }
ref={ popoverRefs }
>
{ items.map( ( option, index ) => (
<Button
key={ option.key }
id={ `components-autocomplete-item-${ instanceId }-${ option.key }` }
role="option"
aria-selected={ index === selectedIndex }
disabled={ option.isDisabled }
className={ classnames(
'components-autocomplete__result',
className,
{
'is-selected': index === selectedIndex,
}
) }
onClick={ () => onSelect( option ) }
>
{ option.label }
</Button>
) ) }
</div>
</Popover>
<ListBox />
</Popover>
{ contentRef.current &&
needsA11yCompat &&
createPortal(
<ListBox Component={ VisuallyHidden } />,
contentRef.current.ownerDocument.body
) }
</>
);
}

Expand Down
67 changes: 54 additions & 13 deletions test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js
View file
Open in desktop
Expand Up @@ -57,12 +57,14 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {
} )
)
);
await requestUtils.activateTheme( 'emptytheme' );
await requestUtils.activatePlugin( 'gutenberg-test-autocompleter' );
} );

test.afterAll( async ( { requestUtils } ) => {
await requestUtils.deleteAllUsers();
await requestUtils.deactivatePlugin( 'gutenberg-test-autocompleter' );
await requestUtils.activateTheme( 'twentytwentyone' );
} );

test.beforeEach( async ( { admin } ) => {
Expand Down Expand Up @@ -98,11 +100,28 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {
<!-- /wp:paragraph -->`;
}

await page.click( 'role=button[name="Add default block"i]' );
await editor.canvas.click(
'role=button[name="Add default block"i]'
);
await page.keyboard.type( testData.triggerString );
await expect(
page.locator( `role=option[name="${ testData.optionText }"i]` )
).toBeVisible();
const ariaOwns = await editor.canvas.evaluate( () => {
return document.activeElement.getAttribute( 'aria-owns' );
} );
const ariaActiveDescendant = await editor.canvas.evaluate( () => {
return document.activeElement.getAttribute(
'aria-activedescendant'
);
} );
// 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 );
Comment on lines +118 to +124
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

@ciampo ciampo Feb 15, 2023 •
edited

Choose a reason for hiding this comment

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

Here's some more context to why Autocomplete has mostly e2e tests instead of unit tests :)

#42674 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

@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 😆 .

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

await page.keyboard.press( 'Enter' );
await page.keyboard.type( '.' );

Expand Down Expand Up @@ -131,7 +150,9 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {
<!-- /wp:paragraph -->`;
}

await page.click( 'role=button[name="Add default block"i]' );
await editor.canvas.click(
'role=button[name="Add default block"i]'
);
await page.keyboard.type( 'Stuck in the middle with you.' );
await pageUtils.pressKeyTimes( 'ArrowLeft', 'you.'.length );
await page.keyboard.type( testData.triggerString );
Expand Down Expand Up @@ -169,7 +190,9 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {
<!-- /wp:paragraph -->`;
}

await page.click( 'role=button[name="Add default block"i]' );
await editor.canvas.click(
'role=button[name="Add default block"i]'
);
await page.keyboard.type( testData.firstTriggerString );
await expect(
page.locator(
Expand Down Expand Up @@ -209,7 +232,9 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {
<!-- /wp:paragraph -->`;
}

await page.click( 'role=button[name="Add default block"i]' );
await editor.canvas.click(
'role=button[name="Add default block"i]'
);
await page.keyboard.type( testData.triggerString );
await expect(
page.locator( `role=option[name="${ testData.optionText }"i]` )
Expand Down Expand Up @@ -247,7 +272,9 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {
<!-- /wp:paragraph -->`;
}

await page.click( 'role=button[name="Add default block"i]' );
await editor.canvas.click(
'role=button[name="Add default block"i]'
);
await page.keyboard.type( testData.triggerString );
await expect(
page.locator( `role=option[name="${ testData.optionText }"i]` )
Expand Down Expand Up @@ -282,7 +309,9 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {
<!-- /wp:paragraph -->`;
}

await page.click( 'role=button[name="Add default block"i]' );
await editor.canvas.click(
'role=button[name="Add default block"i]'
);
await page.keyboard.type( testData.triggerString );
await expect(
page.locator( `role=option[name="${ testData.optionText }"i]` )
Expand All @@ -301,7 +330,9 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {
page,
editor,
} ) => {
await page.click( 'role=button[name="Add default block"i]' );
await editor.canvas.click(
'role=button[name="Add default block"i]'
);
// The 'Grapes' option is disabled in our test plugin, so it should not insert the grapes emoji
await page.keyboard.type( 'Sorry, we are all out of ~g' );
await expect(
Expand Down Expand Up @@ -367,7 +398,9 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {
<!-- /wp:paragraph -->`;
}

await page.click( 'role=button[name="Add default block"i]' );
await editor.canvas.click(
'role=button[name="Add default block"i]'
);

for ( let i = 0; i < 4; i++ ) {
await page.keyboard.type( testData.triggerString );
Expand All @@ -393,7 +426,7 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {
page,
editor,
} ) => {
await page.click( 'role=button[name="Add default block"i]' );
await editor.canvas.click( 'role=button[name="Add default block"i]' );
await page.keyboard.type( '@fr' );
await expect(
page.locator( 'role=option', { hasText: 'Frodo Baggins' } )
Expand All @@ -412,8 +445,9 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {

test( 'should hide UI when selection changes (by keyboard)', async ( {
page,
editor,
} ) => {
await page.click( 'role=button[name="Add default block"i]' );
await editor.canvas.click( 'role=button[name="Add default block"i]' );
await page.keyboard.type( '@fr' );
await expect(
page.locator( 'role=option', { hasText: 'Frodo Baggins' } )
Expand All @@ -426,13 +460,20 @@ test.describe( 'Autocomplete (@firefox, @webkit)', () => {

test( 'should hide UI when selection changes (by mouse)', async ( {
page,
editor,
pageUtils,
} ) => {
await page.click( 'role=button[name="Add default block"i]' );
await page.keyboard.type( '@fr' );
await editor.canvas.click( 'role=button[name="Add default block"i]' );
await page.keyboard.type( '@' );
await pageUtils.pressKeyWithModifier( 'primary', 'b' );
await page.keyboard.type( 'f' );
await pageUtils.pressKeyWithModifier( 'primary', 'b' );
await page.keyboard.type( 'r' );
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' );
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

await expect(
page.locator( 'role=option', { hasText: 'Frodo Baggins' } )
).not.toBeVisible();
Copy link
Member

Choose a reason for hiding this comment

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

Is this a related failure?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tests are now run in the iframe, so yes.

Expand Down