Skip to content

Commit

Permalink
fix(carousel): delay instantiation of IntersectionObserver (#10971)
Browse files Browse the repository at this point in the history
### Related Ticket(s)

Closes #10929 
[ADCMS-3414](https://jsw.ibm.com/browse/ADCMS-3414)

### Description

Delays the instantiation of the `IntersectionObserver` in the `<dds-carousel>` component until the root element is available, following the patter for how the resize observer gets setup. This should avoid potential race conditions and more reliably get the carousel setup with the correct `inert` attributes on the carousel items.

The intent with the `IntersectionObserver` in question here, was always to measure intersection with the carousel's viewport. Unforunately, the way the `IntersectionObserver` has been setup as a class field initializer, it happens too early and the referenced `this._contentsNode` was not yet set, that only happens in the [`update()` lifecycle stage](https://codepen.io/m4olivei/pen/JjwLMZx). Further the element referenced by `this._contentsNode` is a flexbox node, set to nowrap that contains all the carousel items, which overflows the container far off to the right of the screen. It's unclear what the bounding box of that element is, but I was seeing issues with using that element as the root node for the `IntersectionObserver`, so I switched to the parent element instead, which has a defined width and is set to `overflow: hidden`. This gives us a good element to use for the carousel viewport as the `IntersectionObserver` root.

### Changelog

**Changed**

- delay instantiation of the `IntersectionObserver` in the `<dds-carousel>` component.
- changed the root element of the `IntersectionObserver` to `.dds--carousel__scroll-container`
- clean up `IntersectionObserver` on ` disconnectedCallback`

### Testing instructions

- [ ] Load the "Carousel > Default" story in Canary. In Dev tools, run `document.querySelector('dds-carousel')._intersectionObserver.root`. Note that it's `null`. Run the same in this PR's Storybook instance. It should now be populated with `div.dds--carousel__scroll-container`
- [ ] There should be no regressions with the Carousel behavior
- [ ] With dev tools open and the `<dds-carousel>` element expanded to show the `<dds-card>` child nodes, verify the correct combinaton of `aria-hidden` and `inert` attributes. Items that are showing to the user should have `aria-hidden="false"` and should not have an `inert` attribute. Items that are hidden to the user should have `aria-hidden="true"` and should have an `inert` attribute. This should hold true as you page through the carousel. NOTE: I've added an e2e test to cover this.
  • Loading branch information
m4olivei authored Sep 29, 2023
1 parent 2cc309d commit 4dc0f91
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 31 deletions.
79 changes: 49 additions & 30 deletions packages/web-components/src/components/carousel/carousel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ const minIntersectionRatio = 0.75;
*/
@customElement(`${ddsPrefix}-carousel`)
class DDSCarousel extends HostListenerMixin(StableSelectorMixin(LitElement)) {
/**
* The scrolling container node.
*/
@query(`.${prefix}--carousel__scroll-container`)
private _containerNode?: HTMLElement;

/**
* The scrolling contents node.
*/
Expand Down Expand Up @@ -115,18 +121,10 @@ class DDSCarousel extends HostListenerMixin(StableSelectorMixin(LitElement)) {

/**
* IntersectionObserver to watch carousel contents.
* As items cross the minIntersectionRatio `inert` and `aria-hidden` are toggled.
*/
private _intersectionObserver = new IntersectionObserver(
this._onIntersect.bind(this),
{
root: this._contentsNode,
threshold: [
0.5 + this._intersectionThresholdDifference,
0.5 - this._intersectionThresholdDifference,
],
}
);
*
* @see connectedCallback()
*/
private _intersectionObserver: IntersectionObserver | null = null;

private _intersectionTimeout?;

Expand Down Expand Up @@ -244,6 +242,40 @@ class DDSCarousel extends HostListenerMixin(StableSelectorMixin(LitElement)) {
}
}

/**
* Cleans-up and creates the intersection observer for the scrolling container.
*
* @param [options] The options.
* @param [options.create] `true` to create the new intersection observer.
*/
private _cleanAndCreateObserverIntersection({
create,
}: { create?: boolean } = {}) {
const { _containerNode: containerNode } = this;
// Avoid creating the intersection observer prematurely by checking that
// this._containerNode has been set.
if (containerNode) {
if (this._intersectionObserver) {
this._intersectionObserver.disconnect();
this._intersectionObserver = null;
}
if (create) {
// As items cross the minIntersectionRatio `inert` and `aria-hidden` are
// toggled.
this._intersectionObserver = new IntersectionObserver(
this._onIntersect.bind(this),
{
root: containerNode,
threshold: [
0.5 + this._intersectionThresholdDifference,
0.5 - this._intersectionThresholdDifference,
],
}
);
}
}
}

/**
* Stops the container from scrolling when focusing on a card outside the viewport.
*
Expand Down Expand Up @@ -373,10 +405,10 @@ class DDSCarousel extends HostListenerMixin(StableSelectorMixin(LitElement)) {

this._childItems = (event.target as HTMLSlotElement).assignedElements();

this._intersectionObserver.disconnect();
this._intersectionObserver?.disconnect();

this._childItems.forEach((item) => {
this._intersectionObserver.observe(item);
this._intersectionObserver?.observe(item);
});

// retrieve item heading, eyebrows, and footers to set same height
Expand Down Expand Up @@ -436,7 +468,6 @@ class DDSCarousel extends HostListenerMixin(StableSelectorMixin(LitElement)) {
);
});
this._observeResizeRoot();
this.markHiddenAsInert();
}
}

Expand Down Expand Up @@ -614,6 +645,7 @@ class DDSCarousel extends HostListenerMixin(StableSelectorMixin(LitElement)) {
connectedCallback() {
super.connectedCallback();
this._cleanAndCreateObserverResize({ create: true });
this._cleanAndCreateObserverIntersection({ create: true });

const containingModal = this.closest(
`${ddsPrefix}-expressive-modal`
Expand All @@ -629,28 +661,15 @@ class DDSCarousel extends HostListenerMixin(StableSelectorMixin(LitElement)) {
}
}

markHiddenAsInert() {
const { _childItems: childItems, start, pageSize } = this;

childItems.forEach((item) => {
const index = childItems.indexOf(item);
item.inert = index < start || index > start + pageSize - 1;
});
}

disconnectedCallback() {
this._cleanAndCreateObserverResize();
this._cleanAndCreateObserverIntersection();
super.disconnectedCallback();
}

firstUpdated() {
this._cleanAndCreateObserverResize({ create: true });
}

protected updated(changedProperties) {
if (changedProperties.has('start')) {
this.markHiddenAsInert();
}
this._cleanAndCreateObserverIntersection({ create: true });
}

render() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* Copyright IBM Corp. 2022
* Copyright IBM Corp. 2022, 2023
*
* This source code is licensed under the Apache-2.0 license found in the
* LICENSE file in the root directory of this source tree.
Expand Down Expand Up @@ -203,6 +203,64 @@ const _tests = {
});
});
},
checkInertAriaHidden: () => {
it('should check visible and hidden cards for expected aria-hidden and inert attributes', () => {
cy.get(_selectorBase).then($carousel => {
// Take note of the page size, for later comparison.
const pageSize = $carousel[0]?.pageSize;

cy.wrap($carousel)
.children(':not([slot="title"])')
.then($carouselItems => {
// Verify that the carousel items have the expected aria-hidden
// and inert attributes.
cy.wrap($carouselItems)
.filter(`[aria-hidden="false"]`)
.should('have.length', pageSize);
cy.wrap($carouselItems)
.filter(':not([inert])')
.should('have.length', pageSize);

// Verify that the first carousel items has the correct
// aria-hidden and inert attributes, and that those attributes
// change accordingly after we advance the slider.
cy.wrap($carouselItems)
.first()
.then($firstChild => {
cy.wrap($firstChild)
.should('have.attr', 'aria-hidden')
.and('equal', 'false');
cy.wrap($firstChild).should('not.have.attr', 'inert');

// Scroll carousel forward.
cy.get(_selectors.buttonNext)
.click()
// Wait a second for the carousel to finish moving
.wait(1000);

// Verify that the aria-hidden and inert attributes of the
// first item toggled as expected. We just check the first,
// so that we don't have to consider the current viewport
// size. Checking the first should suffice, given this
// behavior is triggered via IntersectionObserver, and the
// first item being exposed is representative of any arbitrary
// item being exposed.
cy.wrap($firstChild)
.should('have.attr', 'aria-hidden')
.and('equal', 'true');
cy.wrap($firstChild).should('have.attr', 'inert');

// Scroll carousel backward to set it back to its initial
// position.
cy.get(_selectors.buttonPrevious)
.click()
// Wait a second for the carousel to finish moving
.wait(1000);
});
});
});
});
},
checkScroll: () => {
it('should scroll forward when Next button is clicked and back when the Previous button is clicked', () => {
cy.get(_selectors.buttonNext)
Expand Down Expand Up @@ -232,6 +290,7 @@ describe('dds-carousel | default (desktop)', () => {
_tests.checkTextRenders();
_tests.checkSameHeight();
_tests.checkClickableCard();
_tests.checkInertAriaHidden();
_tests.checkScroll();
});

Expand All @@ -246,6 +305,7 @@ describe('dds-carousel | default (mobile)', () => {
_tests.screenshotThemes();
_tests.checkTextRenders();
_tests.checkClickableCard();
_tests.checkInertAriaHidden();
_tests.checkScroll();
});

Expand Down Expand Up @@ -277,6 +337,7 @@ describe('dds-carousel | with images (mobile)', () => {
_tests.checkTextRenders();
_tests.checkImageRenders();
_tests.checkClickableCard();
_tests.checkInertAriaHidden();
_tests.checkScroll();
});

Expand All @@ -294,6 +355,7 @@ describe('dds-carousel | with videos (desktop)', () => {
_tests.checkVideoDurationText();
_tests.checkSameHeight();
_tests.checkClickableCard();
_tests.checkInertAriaHidden();
_tests.checkScroll();
});

Expand All @@ -310,6 +372,7 @@ describe('dds-carousel | with videos (mobile)', () => {
_tests.checkVideoRenders();
_tests.checkVideoDurationText();
_tests.checkClickableCard();
_tests.checkInertAriaHidden();
_tests.checkScroll();
});

Expand All @@ -328,6 +391,7 @@ describe('dds-carousel | with media (desktop)', () => {
_tests.checkVideoDurationText();
_tests.checkSameHeight();
_tests.checkClickableCard();
_tests.checkInertAriaHidden();
_tests.checkScroll();
});

Expand All @@ -345,5 +409,6 @@ describe('dds-carousel | with media (mobile)', () => {
_tests.checkVideoRenders();
_tests.checkVideoDurationText();
_tests.checkClickableCard();
_tests.checkInertAriaHidden();
_tests.checkScroll();
});

0 comments on commit 4dc0f91

Please sign in to comment.