fix(listbox): always scroll active item into view

ing-bank · Jun 23, 2021 · dd1655e · dd1655e
1 parent 8a32283
commit dd1655e
Show file tree

Hide file tree

Showing 5 changed files with 90 additions and 30 deletions.
diff --git a/.changeset/poor-swans-hope.md 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
@@ -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<import('@lion/core').LitElement>} 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
@@ -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`
+          <div id="scrolling-element" style="position: relative;  overflow-y: scroll; height: 570px;">
+            <div style="position: sticky; top: 0px; width: 100%; height: 100px; background-color: purple; z-index: 1;">Header 1</div>
+            <div style="position: relative">
+              <div style="position: sticky; top: 100px; width: 100%; height: 50px; background-color: beige; z-index: 1;">Header 2</div>
+              <${tag} id="color" name="color" has-no-default-selected>
+                <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'red'}>Red</${optionTag}>
+                <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'hotpink'}>Hotpink</${optionTag}>
+                <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'teal'}>Teal</${optionTag}>
+                <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'1'}>1</${optionTag}>
+                <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'2'}>2</${optionTag}>
+                <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'3'}>3</${optionTag}>
+                <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'4'}>4</${optionTag}>
+                <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'5'}>5</${optionTag}>
+                <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'5'}>6</${optionTag}>
+                <${optionTag} style="height: 50px; scroll-margin-top: 150px;" .choiceValue=${'6'}>7</${optionTag}>
+              </${tag}>
+            </div>
+          </div>
+        `);
+        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
@@ -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;

diff --git a/yarn.lock b/yarn.lock
@@ -1701,7 +1701,7 @@
     "@open-wc/dedupe-mixin" "^1.3.0"
     lit-html "^1.0.0"
 
-"@open-wc/scoped-elements@^2.0.0", "@open-wc/scoped-elements@^2.0.0-next.0":
+"@open-wc/scoped-elements@^2.0.0-next.0":
   version "2.0.0-next.3"
   resolved "https://registry.yarnpkg.com/@open-wc/scoped-elements/-/scoped-elements-2.0.0-next.3.tgz#adbd9d6fddc67158fd11ffe78c5e11aefdaaf8af"
   integrity sha512-9dT+0ea/RKO3s2m5H+U8gwG7m1jE89JhgWKI6FnkG4pE9xMx8KACoLZZcUfogVjb6/vKaIeoCj6Mqm+2HiqCeQ==
@@ -1710,6 +1710,15 @@
     "@open-wc/dedupe-mixin" "^1.3.0"
     "@webcomponents/scoped-custom-element-registry" "0.0.1"
 
+"@open-wc/scoped-elements@^2.0.0-next.3":
+  version "2.0.0-next.4"
+  resolved "https://registry.yarnpkg.com/@open-wc/scoped-elements/-/scoped-elements-2.0.0-next.4.tgz#d8294358e3e8ad2ba44200ab805549fde49245f6"
+  integrity sha512-BMd5n5BHLi3FBhwhPbBuN7pZdi8I1CIQn10aKLZtg9aplVhN2BG1rwr0ANebXJ6fdq8m1PE1wQAaCXYCcEBTEQ==
+  dependencies:
+    "@lit/reactive-element" "^1.0.0-rc.1"
+    "@open-wc/dedupe-mixin" "^1.3.0"
+    "@webcomponents/scoped-custom-element-registry" "0.0.2"
+
 "@open-wc/semantic-dom-diff@^0.13.16":
   version "0.13.21"
   resolved "https://registry.yarnpkg.com/@open-wc/semantic-dom-diff/-/semantic-dom-diff-0.13.21.tgz#718b9ec5f9a98935fc775e577ad094ae8d8b7dea"
@@ -2790,6 +2799,11 @@
   resolved "https://registry.yarnpkg.com/@webcomponents/scoped-custom-element-registry/-/scoped-custom-element-registry-0.0.1.tgz#196365260a019f87bddbded154ab09faf0e666fc"
   integrity sha512-ef5/v4U2vCxrnSMpo41LSWTjBOXCQ4JOt4+Y6PaSd8ympYioPhOP6E1tKmIk2ppwLSjCKbTyYf7ocHvwDat7bA==
 
+"@webcomponents/scoped-custom-element-registry@0.0.2":
+  version "0.0.2"
+  resolved "https://registry.yarnpkg.com/@webcomponents/scoped-custom-element-registry/-/scoped-custom-element-registry-0.0.2.tgz#c863d163cb39c60063808e5ae23e06a1766fbe5f"
+  integrity sha512-lKCoZfKoE3FHvmmj2ytaLBB8Grxp4HaxfSzaGlIZN6xXnOILfpCO0PFJkAxanefLGJWMho4kRY5PhgxWFhmSOw==
+
 "@webcomponents/shadycss@^1.10.2":
   version "1.10.2"
   resolved "https://registry.yarnpkg.com/@webcomponents/shadycss/-/shadycss-1.10.2.tgz#40e03cab6dc5e12f199949ba2b79e02f183d1e7b"