diff --git a/.changeset/poor-swans-hope.md b/.changeset/poor-swans-hope.md new file mode 100644 index 000000000..804216473 --- /dev/null +++ b/.changeset/poor-swans-hope.md @@ -0,0 +1,5 @@ +--- +'@lion/listbox': patch +--- + +Always scrollIntoView and let the scrollIntoView method handle redundancy instead of implementing a "naive" isInView method. diff --git a/packages/listbox/src/ListboxMixin.js b/packages/listbox/src/ListboxMixin.js index f3f9c96c9..1e12d051d 100644 --- a/packages/listbox/src/ListboxMixin.js +++ b/packages/listbox/src/ListboxMixin.js @@ -57,31 +57,6 @@ function uuid() { return Math.random().toString(36).substr(2, 10); } -/** - * @param {HTMLElement} container - * @param {HTMLElement} element - * @param {Boolean} [partial] - */ -function isInView(container, element, partial = false) { - const cTop = container.scrollTop; - const cBottom = cTop + container.clientHeight; - const eTop = element.offsetTop; - const eBottom = eTop + element.clientHeight; - const isTotal = eTop >= cTop && eBottom <= cBottom; - let isPartial; - - if (partial === true) { - isPartial = (eTop < cTop && eBottom > cTop) || (eBottom > cBottom && eTop < cBottom); - } else if (typeof partial === 'number') { - if (eTop < cTop && eBottom > cTop) { - isPartial = ((eBottom - cTop) * 100) / element.clientHeight > partial; - } else if (eBottom > cBottom && eTop < cBottom) { - isPartial = ((cBottom - eTop) * 100) / element.clientHeight > partial; - } - } - return isTotal || isPartial; -} - /** * @type {ListboxMixin} * @param {import('@open-wc/dedupe-mixin').Constructor} superclass @@ -714,6 +689,17 @@ const ListboxMixinImplementation = superclass => this._listboxNode.focus(); } + /** + * @param {HTMLElement} el element + * @param {HTMLElement} [scrollTargetEl] container + * @protected + * @description allow Subclassers to adjust the animation: like non smooth behavior, different timing etc. + */ + // eslint-disable-next-line class-methods-use-this, no-unused-vars + _scrollIntoView(el, scrollTargetEl) { + el.scrollIntoView({ behavior: 'smooth', block: 'nearest' }); + } + /** @private */ __setupEventListeners() { this._listboxNode.addEventListener( @@ -752,9 +738,8 @@ const ListboxMixinImplementation = superclass => return; } this._activeDescendantOwnerNode.setAttribute('aria-activedescendant', el.id); - if (!isInView(this._scrollTargetNode, el)) { - el.scrollIntoView({ behavior: 'smooth', block: 'nearest' }); - } + + this._scrollIntoView(el, this._scrollTargetNode); } /** diff --git a/packages/listbox/test-suites/ListboxMixin.suite.js b/packages/listbox/test-suites/ListboxMixin.suite.js index 47ea375b1..653c6b99e 100644 --- a/packages/listbox/test-suites/ListboxMixin.suite.js +++ b/packages/listbox/test-suites/ListboxMixin.suite.js @@ -3,7 +3,7 @@ import { repeat, LitElement } from '@lion/core'; import { Required } from '@lion/form-core'; import { LionOptions } from '@lion/listbox'; import '@lion/listbox/define'; -import { expect, fixture as _fixture, defineCE } from '@open-wc/testing'; +import { expect, aTimeout, nextFrame, fixture as _fixture, defineCE } from '@open-wc/testing'; import { html, unsafeStatic } from 'lit/static-html.js'; import sinon from 'sinon'; @@ -319,6 +319,60 @@ export function runListboxMixinSuite(customConfig = {}) { expect(el.checkedIndex).to.equal(1); expect(el.serializedValue).to.equal('hotpink'); }); + + it('scrolls active element into view when necessary, takes into account sticky/fixed elements', async () => { + const el = await fixture(html` +
+
Header 1
+
+
Header 2
+ <${tag} id="color" name="color" has-no-default-selected> + <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'red'}>Red + <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'hotpink'}>Hotpink + <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'teal'}>Teal + <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'1'}>1 + <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'2'}>2 + <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'3'}>3 + <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'4'}>4 + <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'5'}>5 + <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'5'}>6 + <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'6'}>7 + +
+
+ `); + const listboxEl = /** @type {LionListbox} */ (el.querySelector('#color')); + + // Skip test if listbox cannot receive focus, e.g. in combobox + // Skip test if the listbox is controlled by overlay system, + // since these overlays "overlay" any fixed/sticky items, meaning + // the active item will not be hidden by them. + // @ts-ignore allow protected members in tests + if (listboxEl._listboxReceivesNoFocus || listboxEl._overlayCtrl) { + return; + } + + Object.defineProperty(listboxEl, '_scrollTargetNode', { + get: () => el, + }); + + const thirdOption = /** @type {LionOption} */ (listboxEl.formElements[2]); + const lastOption = /** @type {LionOption} */ ( + listboxEl.formElements[listboxEl.formElements.length - 1] + ); + await listboxEl.updateComplete; + await nextFrame(); + + // Scroll to last option and wait for browser scroll animation (works) + lastOption.active = true; + await aTimeout(1000); + + thirdOption.active = true; + await aTimeout(1000); + + // top should be offset 2x40px (sticky header elems) instead of 0px + expect(el.scrollTop).to.equal(116); + }); }); describe('Accessibility', () => { diff --git a/packages/listbox/types/ListboxMixinTypes.d.ts b/packages/listbox/types/ListboxMixinTypes.d.ts index 568135daa..700929be4 100644 --- a/packages/listbox/types/ListboxMixinTypes.d.ts +++ b/packages/listbox/types/ListboxMixinTypes.d.ts @@ -63,6 +63,8 @@ export declare class ListboxHost { protected _listboxOnKeyUp(ev: KeyboardEvent): void; + protected _scrollIntoView(el: HTMLElement, scrollTargetEl: HTMLElement | undefined): void; + protected _setupListboxNode(): void; protected _teardownListboxNode(): void;