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

refactor(dropdown): extend updateComplete timing for overlay oven/close #708

Merged
merged 1 commit into from
Jun 1, 2020
Merged

refactor(dropdown): extend updateComplete timing for overlay oven/close #708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 39 additions & 17 deletions packages/dropdown/src/dropdown.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ import {
MenuItem,
MenuItemQueryRoleEventDetail,
} from '@spectrum-web-components/menu-item';
import '@spectrum-web-components/popover';
import { Overlay, Placement } from '@spectrum-web-components/overlay';
import { Placement } from '@spectrum-web-components/overlay';

/**
* @slot label - The placeholder content for the dropdown
Expand Down Expand Up @@ -227,15 +226,17 @@ export class DropdownBase extends Focusable {
delete this.placeholder;
}

private openMenu(): void {
private async openMenu(): Promise<void> {
/* istanbul ignore if */
if (
!this.popover ||
!this.button ||
!this.optionsMenu ||
this.optionsMenu.children.length === 0
)
) {
this.menuStateResolver();
return;
}

this.placeholder = document.createComment(
'placeholder for optionsMenu'
Expand All @@ -256,16 +257,39 @@ export class DropdownBase extends Focusable {
if (menuWidth) {
this.popover.style.setProperty('width', menuWidth);
}
Copy link
Collaborator

@andrewhatran andrewhatran Jun 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I understand that we're lazy loading Overlay so that we can prevent unused code from being downloaded.

However, since every dropdown contains an overlay, wouldn't Overlay be imported every time a dropdown is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100%, Overlay is a hard requirement of using the functionality that is delivered by an sp-dropdown element. However, lazy loading isn't only about preventing unused code; that's more directly a responsibility of tree shaking and side effect management (see #679 that looks to enhance out approach to that).

In this case, Overlay isn't a requirement of displaying the dropdown element when closed, and if you don't need to interact with the dropdown, this would allow the page never to download that JS. As an example, checkout the documentation site: https://opensource.adobe.com/spectrum-web-components/ unless you specifically interact with the theme customization UI, or are looking to review the dropdown documentation, there is no need to load the Overlay scripts.

In the case that you do need the Overlay scripts, it is highly unlikely that you'd interact with the dropdown immediately (e.g. load the page with the dropdown open by default, an,. if needed, that can be managed into a single bundle externally). This change looks to ensure that we give download/parse/run priority to JS that is required as the page is rendered by moving this Overlay into the dynamic dependency graph as opposed to the static dependency graph.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, that checks out. Giving priority to JS when the dependency is required rather than at construction makes a lot of sense.

It makes even more sense for disabled dropdowns.

const Overlay = await Promise.all([
import('@spectrum-web-components/overlay'),
import('@spectrum-web-components/popover'),
]).then(
([module]) =>
(module as typeof import('@spectrum-web-components/overlay'))
.Overlay
);
this.closeOverlay = Overlay.open(this.button, 'click', this.popover, {
placement: this.placement,
});
requestAnimationFrame(() => {
/* istanbul ignore else */
if (this.optionsMenu) {
/* Trick :focus-visible polyfill into thinking keyboard based focus */
this.dispatchEvent(
new KeyboardEvent('keydown', {
code: 'Tab',
})
);
this.optionsMenu.focus();
}
this.menuStateResolver();
});
}

private closeMenu(): void {
if (this.closeOverlay) {
this.closeOverlay();
delete this.closeOverlay;
}

this.menuStateResolver();
}

protected get buttonContent(): TemplateResult[] {
Expand Down Expand Up @@ -352,27 +376,25 @@ export class DropdownBase extends Focusable {
this.open = false;
}
if (changedProperties.has('open')) {
this.menuStatePromise = new Promise(
(res) => (this.menuStateResolver = res)
);
if (this.open) {
this.openMenu();
requestAnimationFrame(() => {
/* istanbul ignore if */
if (!this.optionsMenu) {
return;
}
/* Trick :focus-visible polyfill into thinking keyboard based focus */
this.dispatchEvent(
new KeyboardEvent('keydown', {
code: 'Tab',
})
);
this.optionsMenu.focus();
});
} else {
this.closeMenu();
}
}
}

private menuStatePromise = Promise.resolve();
private menuStateResolver!: () => void;

protected async _getUpdateComplete(): Promise<void> {
await super._getUpdateComplete();
await this.menuStatePromise;
}

public disconnectedCallback(): void {
this.open = false;

Expand Down
28 changes: 7 additions & 21 deletions packages/dropdown/test/dropdown.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,11 @@ import { Dropdown } from '../';
import '../../menu';
import '../../menu-item';
import { MenuItem } from '../../menu-item';
import { Popover } from '../../popover';
import {
fixture,
elementUpdated,
html,
expect,
nextFrame,
waitUntil,
} from '@open-wc/testing';
import { waitForPredicate } from '../../../test/testing-helpers';
Expand All @@ -38,12 +36,8 @@ const keyboardEvent = (code: string): KeyboardEvent =>
const arrowDownEvent = keyboardEvent('ArrowDown');
const arrowUpEvent = keyboardEvent('ArrowUp');

type testableDropdown = {
popover: Popover;
};

const dropdownFixture = async (): Promise<Dropdown> =>
await fixture<Dropdown>(
const dropdownFixture = async (): Promise<Dropdown> => {
const el = await fixture<Dropdown>(
html`
<sp-dropdown
label="Select a Country with a very long label, too long in fact"
Expand Down Expand Up @@ -72,11 +66,13 @@ const dropdownFixture = async (): Promise<Dropdown> =>
</sp-dropdown>
`
);
await waitForPredicate(() => !!window.applyFocusVisiblePolyfill);
return el;
};

describe('Dropdown', () => {
it('loads', async () => {
const el = await dropdownFixture();
await waitForPredicate(() => !!window.applyFocusVisiblePolyfill);

await elementUpdated(el);
expect(el).to.not.be.undefined;
Expand All @@ -85,7 +81,6 @@ describe('Dropdown', () => {
});
it('renders invalid', async () => {
const el = await dropdownFixture();
await waitForPredicate(() => !!window.applyFocusVisiblePolyfill);

await elementUpdated(el);

Expand All @@ -98,7 +93,6 @@ describe('Dropdown', () => {
});
it('closes when becoming disabled', async () => {
const el = await dropdownFixture();
await waitForPredicate(() => !!window.applyFocusVisiblePolyfill);

await elementUpdated(el);

Expand All @@ -114,7 +108,6 @@ describe('Dropdown', () => {
});
it('selects', async () => {
const el = await dropdownFixture();
await waitForPredicate(() => !!window.applyFocusVisiblePolyfill);

await elementUpdated(el);

Expand All @@ -139,7 +132,6 @@ describe('Dropdown', () => {
});
it('re-selects', async () => {
const el = await dropdownFixture();
await waitForPredicate(() => !!window.applyFocusVisiblePolyfill);

await elementUpdated(el);

Expand Down Expand Up @@ -181,7 +173,6 @@ describe('Dropdown', () => {
});
it('can have selection prevented', async () => {
const el = await dropdownFixture();
await waitForPredicate(() => !!window.applyFocusVisiblePolyfill);

await elementUpdated(el);

Expand Down Expand Up @@ -210,7 +201,6 @@ describe('Dropdown', () => {
});
it('opens on ArrowDown', async () => {
const el = await dropdownFixture();
await waitForPredicate(() => !!window.applyFocusVisiblePolyfill);

await elementUpdated(el);

Expand Down Expand Up @@ -245,7 +235,6 @@ describe('Dropdown', () => {
});
it('loads', async () => {
const el = await dropdownFixture();
await waitForPredicate(() => !!window.applyFocusVisiblePolyfill);

await elementUpdated(el);
expect(el).to.not.be.undefined;
Expand All @@ -254,7 +243,6 @@ describe('Dropdown', () => {
});
it('refocuses on list when open', async () => {
const el = await dropdownFixture();
await waitForPredicate(() => !!window.applyFocusVisiblePolyfill);

await elementUpdated(el);
const firstItem = el.querySelector('sp-menu-item') as MenuItem;
Expand Down Expand Up @@ -328,7 +316,6 @@ describe('Dropdown', () => {
button.click();

await elementUpdated(el);
await nextFrame();

expect(focusFirstSpy.called, 'do not focus first element').to.be.false;
expect(focusSelectedSpy.calledOnce, 'focus selected element').to.be
Expand Down Expand Up @@ -397,13 +384,12 @@ describe('Dropdown', () => {
await elementUpdated(el);
await waitUntil(() => el.value === '');

const hoverEl = el.querySelector('sp-menu-item') as MenuItem;

el.open = true;
await elementUpdated(el);

expect(el.open).to.be.true;
const hoverEl = ((el as unknown) as testableDropdown).popover.querySelector(
'sp-menu-item'
) as MenuItem;
hoverEl.dispatchEvent(new MouseEvent('mouseenter'));
await elementUpdated(el);

Expand Down