From 46442eddf70eee46f65ae7a5a947e71376f9a6f7 Mon Sep 17 00:00:00 2001
From: Benny Powers <web@bennypowers.com>
Date: Thu, 15 Aug 2024 12:50:47 +0300
Subject: [PATCH] refactor(core): don't bind this in controller options

---
 .../activedescendant-controller.ts            |  6 +-
 .../controllers/combobox-controller.ts        | 64 +++++++++----------
 .../controllers/listbox-controller.ts         | 36 +++++------
 .../test/combobox-controller.spec.ts          |  8 +--
 elements/pf-select/pf-select.ts               | 20 ++----
 5 files changed, 60 insertions(+), 74 deletions(-)

diff --git a/core/pfe-core/controllers/activedescendant-controller.ts b/core/pfe-core/controllers/activedescendant-controller.ts
index 79f9f84ea4..a5d1e9a47b 100644
--- a/core/pfe-core/controllers/activedescendant-controller.ts
+++ b/core/pfe-core/controllers/activedescendant-controller.ts
@@ -21,13 +21,13 @@ export interface ActivedescendantControllerOptions<
    * If you provide this callback, ActivedescendantController will call it on your item with the
    * active state. You may use this to set active styles.
    */
-  setItemActive?(this: Item, active: boolean): void;
+  setItemActive?(item: Item, active: boolean): void;
   /**
    * Optional callback to retrieve the value from an option element.
    * By default, retrieves the `value` attribute, or the text content.
    * @see https://developer.mozilla.org/en-US/docs/Web/API/HTMLOptionElement
    */
-  getItemValue?(this: Item): string;
+  getItemValue?(item: Item): string;
 }
 
 /**
@@ -134,7 +134,7 @@ export class ActivedescendantController<
     super.atFocusedItemIndex = index;
     const item = this._items.at(this.atFocusedItemIndex);
     for (const _item of this.items) {
-      this.options.setItemActive?.call(_item, _item === item);
+      this.options.setItemActive?.(_item, _item === item);
     }
     const container = this.options.getActiveDescendantContainer();
     if (!ActivedescendantController.supportsCrossRootActiveDescendant) {
diff --git a/core/pfe-core/controllers/combobox-controller.ts b/core/pfe-core/controllers/combobox-controller.ts
index b639249ff9..9c0eb7ac1a 100644
--- a/core/pfe-core/controllers/combobox-controller.ts
+++ b/core/pfe-core/controllers/combobox-controller.ts
@@ -18,14 +18,6 @@ type AllOptions<Item extends HTMLElement> =
 
 type Lang = typeof ComboboxController['langs'][number];
 
-function getItemValue<Item extends HTMLElement>(this: Item): string {
-  if ('value' in this && typeof this.value === 'string') {
-    return this.value;
-  } else {
-    return '';
-  }
-}
-
 function deepClosest(element: Element | null, selector: string) {
   let closest = element?.closest(selector);
   let root = element?.getRootNode();
@@ -45,31 +37,39 @@ function deepClosest(element: Element | null, selector: string) {
   return closest;
 }
 
-function isItemFiltered<Item extends HTMLElement>(this: Item, value: string): boolean {
-  return !getItemValue.call(this)
+function getItemValue<Item extends HTMLElement>(item: Item): string {
+  if ('value' in item && typeof item.value === 'string') {
+    return item.value;
+  } else {
+    return '';
+  }
+}
+
+function isItemFiltered<Item extends HTMLElement>(item: Item, value: string): boolean {
+  return !getItemValue(item)
       .toLowerCase()
       .startsWith(value.toLowerCase());
 }
 
-function setItemHidden(this: HTMLElement, hidden: boolean) {
-  this.hidden = hidden;
+function setItemHidden(item: HTMLElement, hidden: boolean) {
+  item.hidden = hidden;
 }
 
-function setComboboxValue(this: HTMLElement, value: string): void {
-  if (!('value' in this)) {
+function setComboboxValue(item: HTMLElement, value: string): void {
+  if (!('value' in item)) {
     // eslint-disable-next-line no-console
-    return console.warn(`Cannot set value on combobox element ${this.localName}`);
+    return console.warn(`Cannot set value on combobox element ${item.localName}`);
   } else {
-    this.value = value;
+    item.value = value;
   }
 }
 
-function getComboboxValue(this: HTMLElement): string {
-  if ('value' in this && typeof this.value === 'string') {
-    return this.value;
+function getComboboxValue(combobox: HTMLElement): string {
+  if ('value' in combobox && typeof combobox.value === 'string') {
+    return combobox.value;
   } else {
     // eslint-disable-next-line no-console
-    return console.warn(`Cannot get value from combobox element ${this.localName}`), '';
+    return console.warn(`Cannot get value from combobox element ${combobox.localName}`), '';
   }
 }
 
@@ -118,28 +118,28 @@ export interface ComboboxControllerOptions<Item extends HTMLElement> extends
    * of the item, as if it implemented the `<option>` element's interface.
    * @see https://developer.mozilla.org/en-US/docs/Web/API/HTMLOptionElement
    */
-  getItemValue?(this: Item): string;
+  getItemValue?(item: Item): string;
   /**
    * Optional callback, called on the combobox input element to set its value.
    * by default, returns the element's `value` DOM property.
    */
-  getComboboxValue?(this: HTMLElement): string;
+  getComboboxValue?(combobox: HTMLElement): string;
   /**
    * Optional callback, called on the combobox input element to set its value.
    * by default, sets the element's `value` DOM property.
    */
-  setComboboxValue?(this: HTMLElement, value: string): void;
+  setComboboxValue?(item: HTMLElement, value: string): void;
   /**
    * Called on each item, with the combobox input, to determine if the item should be shown in the
    * listbox or filtered out. Return false to hide the item. By default, checks whether the item's
    * value starts with the input value (when both are lowercased).
    */
-  isItemFiltered?(this: Item, value: string): boolean;
+  isItemFiltered?(item: Item, value: string): boolean;
   /**
    * Called on each item when the filter changes.
    * By default, toggles the `hidden` attribute on the item
    */
-  setItemHidden?(this: Item, hidden: boolean): void;
+  setItemHidden?(item: Item, hidden: boolean): void;
 }
 
 /**
@@ -499,14 +499,14 @@ export class ComboboxController<
 
   // TODO(bennypowers): perhaps move this to ActivedescendantController
   #announce(item: Item) {
-    const value = this.options.getItemValue.call(item);
+    const value = this.options.getItemValue(item);
     ComboboxController.#alert?.remove();
     const fragment = ComboboxController.#alertTemplate.content.cloneNode(true) as DocumentFragment;
     ComboboxController.#alert = fragment.firstElementChild as HTMLElement;
     let text = value;
     const lang = deepClosest(this.#listbox, '[lang]')?.getAttribute('lang') ?? 'en';
     const langKey = lang?.match(ComboboxController.langsRE)?.at(0) as Lang ?? 'en';
-    if (this.options.isItemDisabled.call(item)) {
+    if (this.options.isItemDisabled(item)) {
       text += ` (${this.#translate('dimmed', langKey)})`;
     }
     if (this.#lb.isSelected(item)) {
@@ -530,10 +530,10 @@ export class ComboboxController<
       for (const item of this.items) {
         const hidden =
           !!this.options.isExpanded()
-            && !!(value = this.options.getComboboxValue.call(this.#input))
-            && this.options.isItemFiltered?.call(item, value)
+            && !!(value = this.options.getComboboxValue(this.#input))
+            && this.options.isItemFiltered?.(item, value)
             || false;
-        this.options.setItemHidden.call(item, hidden);
+        this.options.setItemHidden(item, hidden);
       }
     }
   }
@@ -606,7 +606,7 @@ export class ComboboxController<
         break;
       case 'Escape':
         if (!this.options.isExpanded()) {
-          this.options.setComboboxValue.call(this.#input, '');
+          this.options.setComboboxValue(this.#input, '');
           this.host.requestUpdate();
         }
         this.#hide();
@@ -688,7 +688,7 @@ export class ComboboxController<
           if (eventItem
               && !this.multi
               && this.options.isExpanded()
-              && !this.options.isItemDisabled.call(eventItem)
+              && !this.options.isItemDisabled(eventItem)
           ) {
             this.#hide();
             this.#button?.focus();
diff --git a/core/pfe-core/controllers/listbox-controller.ts b/core/pfe-core/controllers/listbox-controller.ts
index 79fa711027..22c4123c05 100644
--- a/core/pfe-core/controllers/listbox-controller.ts
+++ b/core/pfe-core/controllers/listbox-controller.ts
@@ -18,7 +18,7 @@ export interface ListboxControllerOptions<Item extends HTMLElement> {
    * element with the selected state.
    * Callers **must** ensure that the correct ARIA state is set.
    */
-  setItemSelected?(this: Item, selected: boolean): void;
+  setItemSelected?(item: Item, selected: boolean): void;
   /**
    * Optional predicate to ascertain whether a custom element item is disabled or not
    * By default, if the item matches any of these conditions, it is considered disabled:
@@ -27,7 +27,7 @@ export interface ListboxControllerOptions<Item extends HTMLElement> {
    * 2. it has the `disabled` attribute present
    * 3. it matches the `:disabled` pseudo selector
    */
-  isItemDisabled?(this: Item): boolean;
+  isItemDisabled?(item: Item): boolean;
   /**
    * Predicate which determines if a given element is in fact an item
    * instead of e.g a presentational divider. By default, elements must meet the following criteria
@@ -63,13 +63,14 @@ export interface ListboxControllerOptions<Item extends HTMLElement> {
 
 /**
  * This is the default method for setting the selected state on an item element
+ * @param item the item
  * @param selected is this item selected
  */
-function setItemSelected<Item extends HTMLElement>(this: Item, selected: boolean) {
+function setItemSelected<Item extends HTMLElement>(item: Item, selected: boolean) {
   if (selected) {
-    this.setAttribute('aria-selected', 'true');
+    item.setAttribute('aria-selected', 'true');
   } else {
-    this.removeAttribute('aria-selected');
+    item.removeAttribute('aria-selected');
   }
 }
 
@@ -92,12 +93,12 @@ export function isItem<Item extends HTMLElement>(item: EventTarget | null): item
  * @param item possibly disabled item
  * @package do not import this outside of `@patternfly/pfe-core`, it is subject to change at any time
  */
-export function isItemDisabled<Item extends HTMLElement>(this: Item): boolean {
-  return ('disabled' in this && typeof this.disabled === 'boolean' && this.disabled)
-      || this.getAttribute('aria-disabled') === 'true'
-      || this.hasAttribute('disabled')
-      || this.hasAttribute('inert')
-      || this.matches(':disabled');
+export function isItemDisabled<Item extends HTMLElement>(item: Item): boolean {
+  return ('disabled' in item && typeof item.disabled === 'boolean' && item.disabled)
+      || item.getAttribute('aria-disabled') === 'true'
+      || item.hasAttribute('disabled')
+      || item.hasAttribute('inert')
+      || item.matches(':disabled');
 }
 
 let constructingAllowed = false;
@@ -211,7 +212,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
     if (!arraysAreEquivalent(selected, Array.from(this.#selectedItems))) {
       this.#selectedItems = new Set(selected);
       for (const item of this.items) {
-        this.#options.setItemSelected.call(item, this.#selectedItems.has(item));
+        this.#options.setItemSelected(item, this.#selectedItems.has(item));
       }
       this.host.requestUpdate();
     }
@@ -417,7 +418,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
   #onClick = (event: MouseEvent) => {
     const item = this.#getItemFromEvent(event);
     this.#shiftStartingItem ??= this.#getItemFromEvent(event);
-    if (item && !this.#options.isItemDisabled.call(item)) {
+    if (item && !this.#options.isItemDisabled(item)) {
       // Case: single select?
       //       just reset the selected list.
       if (!this.multi) {
@@ -489,8 +490,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
         if (event.ctrlKey
             && (event.target === this.container
                 || this.#options.isItem(event.target))) {
-          const selectableItems = this.items.filter(item =>
-            !this.#options.isItemDisabled.call(item));
+          const selectableItems = this.items.filter(item => !this.#options.isItemDisabled(item));
           if (arraysAreEquivalent(this.selected, selectableItems)) {
             this.selected = [];
           } else {
@@ -514,7 +514,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
           this.selected = this.items.filter((x, i) =>
             this.#selectedItems.has(x)
             || i === this.items.indexOf(item) - 1)
-              .filter(x => !this.#options.isItemDisabled.call(x));
+              .filter(x => !this.#options.isItemDisabled(x));
         }
         break;
       case 'ArrowDown':
@@ -523,7 +523,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
           this.selected = this.items.filter((x, i) =>
             this.#selectedItems.has(x)
             || i === this.items.indexOf(item) + 1)
-              .filter(x => !this.#options.isItemDisabled.call(x));
+              .filter(x => !this.#options.isItemDisabled(x));
         }
         break;
       case ' ':
@@ -544,7 +544,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
   };
 
   #selectItem(item: Item, shiftDown = false) {
-    if (this.#options.isItemDisabled.call(item)) {
+    if (this.#options.isItemDisabled(item)) {
       return;
     } else if (this.multi && shiftDown) {
       // update starting item for other multiselect
diff --git a/core/pfe-core/controllers/test/combobox-controller.spec.ts b/core/pfe-core/controllers/test/combobox-controller.spec.ts
index 317dfcc8c6..6bf92712db 100644
--- a/core/pfe-core/controllers/test/combobox-controller.spec.ts
+++ b/core/pfe-core/controllers/test/combobox-controller.spec.ts
@@ -43,12 +43,8 @@ abstract class TestCombobox extends ReactiveElement {
     isExpanded: () => !this.listbox.hidden,
     requestShowListbox: () => void (this.listbox.hidden = false),
     requestHideListbox: () => void (this.listbox.hidden = true),
-    setItemActive(active) {
-      this.classList.toggle('active', active);
-    },
-    setItemSelected(selected) {
-      this.selected = selected;
-    },
+    setItemActive: (item, active) => item.classList.toggle('active', active),
+    setItemSelected: (item, selected) => item.selected = selected,
   });
 
   @query('#listbox') listbox!: HTMLElement;
diff --git a/elements/pf-select/pf-select.ts b/elements/pf-select/pf-select.ts
index 1971706c5b..d1d8aa52e6 100644
--- a/elements/pf-select/pf-select.ts
+++ b/elements/pf-select/pf-select.ts
@@ -127,22 +127,12 @@ export class PfSelect extends LitElement {
     getToggleButton: () => this._toggleButton ?? null,
     getComboboxInput: () => this._toggleInput ?? null,
     isExpanded: () => this.expanded,
-    requestShowListbox: () =>
-      this.expanded ||= true,
-    requestHideListbox: () =>
-      ((this.expanded &&= false), true),
-    setItemHidden(hidden) {
-      if (this.id !== 'placeholder') {
-        this.hidden = hidden;
-      }
-    },
+    requestShowListbox: () => void (this.expanded ||= true),
+    requestHideListbox: () => void (this.expanded &&= false),
+    setItemHidden: (item, hidden) => (item.id !== 'placeholder') && void (item.hidden = hidden),
     isItem: item => item instanceof PfOption,
-    setItemActive(active) {
-      this.active = active;
-    },
-    setItemSelected(selected) {
-      this.selected = selected;
-    },
+    setItemActive: (item, active) => item.active = active,
+    setItemSelected: (item, selected) => item.selected = selected,
   });
 
   /**