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.

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

Updating package-lock.json #5158

Merged
merged 22 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .vdiff.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"browsers": [
{
"name": "Chromium",
"version": 130
"version": 131
}
],
"system": {
Expand Down
8 changes: 7 additions & 1 deletion components/dropdown/dropdown-content-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ export const DropdownContentMixin = superclass => class extends LocalizeCoreElem

// DE44538: wait for dropdown content to fully render,
// otherwise this.getContentContainer() can return null.
await this.updateComplete;
await this.__waitForContentContainer();
Copy link
Member

Choose a reason for hiding this comment

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

@dbatiste @margaree FYI on the changes in here.

This first one wasn't causing vdiffs to fail, but it was causing a lot of errors in the console because this.getContentContainer() was returning null and the code below was assuming that it wasn't. Clearly based on this comment this was a problem in the past, but I'm guessing waiting for updateComplete is no longer enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why that element would not be rendered by the time updateComplete is fulfilled.


this.__previousFocusableAncestor =
newValue === true
Expand Down Expand Up @@ -849,6 +849,12 @@ export const DropdownContentMixin = superclass => class extends LocalizeCoreElem
this._topOverflow = this.__content.scrollTop !== 0;
}

async __waitForContentContainer() {
if (this.getContentContainer() !== null) return;
await new Promise(resolve => requestAnimationFrame(resolve));
return this.__waitForContentContainer();
}

_constrainSpaceAround(spaceAround, spaceRequired, targetRect) {
const constrained = { ...spaceAround };
if (this.boundary) {
Expand Down
22 changes: 14 additions & 8 deletions components/filter/filter.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ function addSpaceListener() {
if (spaceListenerAdded) return;
spaceListenerAdded = true;
document.addEventListener('keydown', e => {
if (e.keyCode !== 32) return;
if (e.key !== ' ') return;
Copy link
Member

Choose a reason for hiding this comment

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

My IDE was giving me warnings since keyCode is deprecated. This is functionally equivalent.

spacePressed = true;
});
document.addEventListener('keyup', e => {
if (e.keyCode !== 32) return;
if (e.key !== ' ') return;
spacePressed = false;
});
}
Expand Down Expand Up @@ -244,6 +244,7 @@ class Filter extends FocusMixin(LocalizeCoreElement(RtlMixin(LitElement))) {
onSubscribe: this._updateActiveFiltersSubscriber.bind(this),
updateSubscribers: this._updateActiveFiltersSubscribers.bind(this)
});
this._spacePressedDuringLastSelection = false;
}

static get focusElementSelector() {
Expand Down Expand Up @@ -617,7 +618,7 @@ class Filter extends FocusMixin(LocalizeCoreElement(RtlMixin(LitElement))) {
return html`
<d2l-list-item
id="${itemId}"
@d2l-list-item-selected="${ifDefined(item.additionalContent ? this._handleListItemSelelcted : undefined)}"
@d2l-list-item-selected="${item.additionalContent ? this._handleListItemSelected : undefined}"
Copy link
Member

Choose a reason for hiding this comment

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

Wrapping this in ifDefined does nothing since that's just for attributes. Also fixing typo in the handler name.

?selection-disabled="${item.disabled}"
?hidden="${item.hidden}"
key="${item.key}"
Expand Down Expand Up @@ -896,18 +897,23 @@ class Filter extends FocusMixin(LocalizeCoreElement(RtlMixin(LitElement))) {
}

async _handleExpandCollapse(e) {
const eventPromise = e.target.expanded ? e.detail.expandComplete : e.detail.collapseComplete;
const expanded = e.target.expanded;
const eventPromise = expanded ? e.detail.expandComplete : e.detail.collapseComplete;
const parentListItem = e.target.closest('d2l-list-item');
parentListItem.classList.add('expanding-content');

await eventPromise;
parentListItem.classList.remove('expanding-content');

if (expanded && !hasDisplayedKeyboardTooltip && this._spacePressedDuringLastSelection) {
await new Promise(resolve => requestAnimationFrame(resolve));
this._displayKeyboardTooltip = true;
hasDisplayedKeyboardTooltip = true;
}
}

_handleListItemSelelcted() {
if (hasDisplayedKeyboardTooltip || !spacePressed) return;
this._displayKeyboardTooltip = true;
hasDisplayedKeyboardTooltip = true;
Copy link
Member

Choose a reason for hiding this comment

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

We were getting really consistent failures (~90% of the time) where the tooltip which is tied to the list item would position itself before the expand/collapse area had finished opening. This captures whether spacebar was pressed (because we lose that information after) but moves the displaying of the tooltip up into the event handler for expand/collapse.

_handleListItemSelected() {
this._spacePressedDuringLastSelection = spacePressed;
}

_handleSearch(e) {
Expand Down
51 changes: 5 additions & 46 deletions components/filter/test/filter.vdiff.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,32 +210,11 @@ describe('filter', () => {
it('dates-custom-tooltip', async() => {
resetHasDisplayedKeyboardTooltip();
const elem = await fixture(createSingleDimDateCustomSimple());
focusElem(elem.shadowRoot.querySelector('d2l-list-item'));
await focusElem(elem.shadowRoot.querySelector('d2l-list-item'));
sendKeysElem(elem, 'press', 'Tab+Space');
await oneEvent(elem, 'd2l-tooltip-show');
await nextFrame();
await expect(document).to.be.golden();
});

it('dates-custom-tooltip-selected-default', async() => {
resetHasDisplayedKeyboardTooltip();
const elem = await fixture(createSingleDimDateCustomSimple(true));
const listItem = elem.shadowRoot.querySelector('d2l-list-item');
focusElem(listItem);
sendKeysElem(listItem, 'press', 'ArrowDown');
await aTimeout(200); // make sure tooltip does not appear
await expect(document).to.be.golden();
});

it('dates-custom-tooltip-selected-default-deselected-selected', async() => {
resetHasDisplayedKeyboardTooltip();
const elem = await fixture(createSingleDimDateCustomSimple(true));
const listItem = elem.shadowRoot.querySelector('d2l-list-item');
focusElem(listItem);
sendKeysElem(listItem, 'press', 'ArrowDown');
sendKeysElem(listItem, 'press', 'ArrowUp+Space');
await oneEvent(elem, 'd2l-tooltip-show');
await nextFrame();
await aTimeout(300);
await expect(document).to.be.golden();
});

Expand Down Expand Up @@ -268,12 +247,12 @@ describe('filter', () => {
await nextFrame();
await clickElem(elem.shadowRoot.querySelector('[text="Clear"]'));
await hoverAt(0, 0);
await aTimeout(500);
await expect(elem).to.be.golden();
});
});

[
{ name: 'press-clear', allSelected: true, selector: '[text="Clear"]' },
{ name: 'press-unselect-all', allSelected: true, selector: 'd2l-selection-select-all' },
{ name: 'press-select-all', allSelected: false, selector: 'd2l-selection-select-all' }
].forEach(({ name, allSelected, selector }) => {
Expand All @@ -290,6 +269,7 @@ describe('filter', () => {

await clickElem(elem.shadowRoot.querySelector(selector));
await hoverAt(0, 0);
await aTimeout(300);
await expect(elem).to.be.golden();
});
});
Expand All @@ -310,28 +290,7 @@ describe('filter', () => {

await clickElem(elem.shadowRoot.querySelector('[text="Clear"]'));
await hoverAt(0, 0);
await expect(elem).to.be.golden();
});

it('press-clear-dates-custom-date-selected', async() => {
const elem = await fixture(createSingleDimDateCustom({ customSelected: true, startValue: '2018-02-12T05:00:00.000Z', opened: true }));

await clickElem(elem.shadowRoot.querySelector('[text="Clear"]'));
await hoverAt(0, 0);
await expect(elem).to.be.golden();
});

it('select-other-option-then-custom-again', async() => {
const elem = await fixture(createSingleDimDateCustom({ customSelected: true, startValue: '2018-02-12T05:00:00.000Z', opened: true }));
await clickElem(elem.shadowRoot.querySelector('d2l-list-item'));
await clickElem(elem.shadowRoot.querySelector('d2l-list-item[label="Custom date range, expand to choose dates"]'));
await hoverAt(0, 0);
await expect(elem).to.be.golden();
});

it('open custom date input', async() => {
const elem = await fixture(createSingleDimDateCustom({ customSelected: true, startValue: '2018-02-12T05:00:00.000Z', opened: true }));
elem.shadowRoot.querySelector('d2l-list-item[label="Custom date range, expand to choose dates"]').querySelector('d2l-input-date-time-range').setAttribute('start-opened', 'start-opened');
await aTimeout(300);
await expect(elem).to.be.golden();
});

Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
10 changes: 9 additions & 1 deletion components/inputs/input-time.js
Original file line number Diff line number Diff line change
Expand Up @@ -461,9 +461,17 @@ class InputTime extends InputInlineHelpMixin(FocusMixin(LabelledMixin(SkeletonMi
// open and focus dropdown on down arrow or enter
if (e.keyCode === 40 || e.keyCode === 13) {
if (!this._dropdownFirstOpened) this._dropdownFirstOpened = true;
this.opened = true;
e.preventDefault();
await this._waitForItems();
Copy link
Member

Choose a reason for hiding this comment

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

I was seeing consistent vdiff results where it wasn't focusing on the first item in the menu when the time input was opened. I tracked this down to the menu bailing on its focus code because there weren't any items and it therefore couldn't determine where to move focus. That's because time inputs don't render their menu items until the menu is first opened as a performance thing.

So this change sets this._dropdownFirstOpened to true which will cause a re-render and the items to be rendered. It then waits for them to be present in the DOM, and only then does it set opened which will cause another re-render and the menu code will take over to place focus.

this.opened = true;
}
}

async _waitForItems() {
const items = this.shadowRoot.querySelectorAll('d2l-menu-item-radio');
if (items.length > 0) return;
await new Promise(resolve => requestAnimationFrame(resolve));
return this._waitForItems();
}
}
customElements.define('d2l-input-time', InputTime);
3 changes: 2 additions & 1 deletion components/inputs/test/input-date-time.vdiff.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,8 @@ describe('d2l-input-date-time', () => {
it('open time', async() => {
const elem = await fixture(basicFixture);
const textInput = elem.shadowRoot.querySelector('d2l-input-time').shadowRoot.querySelector('input');
await sendKeysElem(textInput, 'press', 'Enter');
sendKeysElem(textInput, 'press', 'Enter');
await oneEvent(elem, 'd2l-dropdown-open');
Copy link
Member

Choose a reason for hiding this comment

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

It needs to wait longer now.

await expect(elem).to.be.golden();
});
});
Expand Down
6 changes: 4 additions & 2 deletions components/inputs/test/input-time-range.vdiff.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,15 +173,17 @@ describe('d2l-input-time-range', () => {

it('open start', async() => {
const elem = await fixture(create({ endValue: time, startValue: laterTime }), { viewport });
await sendKeysElem(elem, 'press', 'Enter');
sendKeysElem(elem, 'press', 'Enter');
await oneEvent(elem, 'd2l-dropdown-open');
await expect(elem).to.be.golden();
});

it('open end', async() => {
const elem = await fixture(create({ endValue: time, startValue: laterTime }), { viewport });
await focusElem(elem);
await sendKeys('press', 'Tab');
await sendKeys('press', 'Enter');
sendKeys('press', 'Enter');
await oneEvent(elem, 'd2l-dropdown-open');
await expect(elem).to.be.golden();
});
});
Expand Down
6 changes: 4 additions & 2 deletions components/inputs/test/input-time.vdiff.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,15 @@ describe('d2l-input-time', () => {

it('dropdown open keydown top', async() => {
const elem = await fixture(create({ value: '00:15:00' }), { viewport });
await sendKeysElem(elem, 'press', 'Enter');
sendKeysElem(elem, 'press', 'Enter');
await oneEvent(elem, 'd2l-dropdown-open');
await expect(elem).to.be.golden();
});

it('dropdown open keydown selected', async() => {
const elem = await fixture(create({ value: '02:00:00' }), { viewport });
await sendKeysElem(elem, 'press', 'Enter');
sendKeysElem(elem, 'press', 'Enter');
await oneEvent(elem, 'd2l-dropdown-open');
await expect(elem).to.be.golden();
});

Expand Down
Loading
Loading