From 56014cf64c387bd4492270905327c570f6814a6b Mon Sep 17 00:00:00 2001
From: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
Date: Thu, 14 Mar 2024 11:14:21 -0400
Subject: [PATCH] fix(range, select): prefer labels passed by developer
 (#29145)

---
 .github/COMPONENT-GUIDE.md                    | 124 +++++++-----------
 BREAKING.md                                   |   4 +-
 core/src/components/range/range.tsx           |  52 ++------
 .../components/range/test/label/range.spec.ts |  30 +++++
 core/src/components/select/select.tsx         |   9 +-
 .../components/select/test/select.spec.tsx    |  28 ++++
 core/src/utils/helpers.ts                     |  58 --------
 core/src/utils/test/aria.spec.ts              |  98 --------------
 8 files changed, 122 insertions(+), 281 deletions(-)
 delete mode 100644 core/src/utils/test/aria.spec.ts

diff --git a/.github/COMPONENT-GUIDE.md b/.github/COMPONENT-GUIDE.md
index 15c74b82e4..28206dc9ab 100644
--- a/.github/COMPONENT-GUIDE.md
+++ b/.github/COMPONENT-GUIDE.md
@@ -439,53 +439,38 @@ render() {
 
 #### Labels
 
-A helper function has been created to get the proper `aria-label` for the checkbox. This can be imported as `getAriaLabel` like the following:
+Labels should be passed directly to the component in the form of either visible text or an `aria-label`. The visible text can be set inside of a `label` element, and the `aria-label` can be set directly on the interactive element.
 
-```tsx
-const { label, labelId, labelText } = getAriaLabel(el, inputId);
-```
+In the following example the `aria-label` can be inherited from the Host using the `inheritAttributes` or `inheritAriaAttributes` utilities. This allows developers to set `aria-label` on the host element since they do not have access to inside the shadow root.
 
-where `el` and `inputId` are the following:
+> [!NOTE]
+> Use `inheritAttributes` to specify which attributes should be inherited or `inheritAriaAttributes` to inherit all of the possible `aria` attributes.
 
 ```tsx
-export class Checkbox implements ComponentInterface {
-  private inputId = `ion-cb-${checkboxIds++}`;
+import { Prop } from '@stencil/core';
+import { inheritAttributes } from '@utils/helpers';
+import type { Attributes } from '@utils/helpers';
 
-  @Element() el!: HTMLElement;
-
-  ...
-}
-```
+...
 
-This can then be added to the `Host` like the following:
+private inheritedAttributes: Attributes = {};
 
-```tsx
-<Host
-  aria-labelledby={label ? labelId : null}
-  aria-checked={`${checked}`}
-  aria-hidden={disabled ? 'true' : null}
-  role="checkbox"
->
-```
+@Prop() labelText?: string;
 
-In addition to that, the checkbox input should have a label added:
+componentWillLoad() {
+  this.inheritedAttributes = inheritAttributes(this.el, ['aria-label']);
+}
 
-```tsx
-<Host
-  aria-labelledby={label ? labelId : null}
-  aria-checked={`${checked}`}
-  aria-hidden={disabled ? 'true' : null}
-  role="checkbox"
->
-  <label htmlFor={inputId}>
-    {labelText}
-  </label>
-  <input
-    type="checkbox"
-    aria-checked={`${checked}`}
-    disabled={disabled}
-    id={inputId}
-  />
+render() {
+  return (
+    <Host>
+      <label>
+        {this.labelText}
+        <input type="checkbox" {...this.inheritedAttributes} /> 
+      </label>
+    </Host>
+  )
+}
 ```
 
 #### Hidden Input
@@ -567,57 +552,40 @@ render() {
 
 #### Labels
 
-A helper function has been created to get the proper `aria-label` for the switch. This can be imported as `getAriaLabel` like the following:
+Labels should be passed directly to the component in the form of either visible text or an `aria-label`. The visible text can be set inside of a `label` element, and the `aria-label` can be set directly on the interactive element.
 
-```tsx
-const { label, labelId, labelText } = getAriaLabel(el, inputId);
-```
+In the following example the `aria-label` can be inherited from the Host using the `inheritAttributes` or `inheritAriaAttributes` utilities. This allows developers to set `aria-label` on the host element since they do not have access to inside the shadow root.
 
-where `el` and `inputId` are the following:
+> [!NOTE]
+> Use `inheritAttributes` to specify which attributes should be inherited or `inheritAriaAttributes` to inherit all of the possible `aria` attributes.
 
 ```tsx
-export class Toggle implements ComponentInterface {
-  private inputId = `ion-tg-${toggleIds++}`;
-
-  @Element() el!: HTMLElement;
+import { Prop } from '@stencil/core';
+import { inheritAttributes } from '@utils/helpers';
+import type { Attributes } from '@utils/helpers';
 
-  ...
-}
-```
+...
 
-This can then be added to the `Host` like the following:
+private inheritedAttributes: Attributes = {};
 
-```tsx
-<Host
-  aria-labelledby={label ? labelId : null}
-  aria-checked={`${checked}`}
-  aria-hidden={disabled ? 'true' : null}
-  role="switch"
->
-```
+@Prop() labelText?: string;
 
-In addition to that, the checkbox input should have a label added:
+componentWillLoad() {
+  this.inheritedAttributes = inheritAttributes(this.el, ['aria-label']);
+}
 
-```tsx
-<Host
-  aria-labelledby={label ? labelId : null}
-  aria-checked={`${checked}`}
-  aria-hidden={disabled ? 'true' : null}
-  role="switch"
->
-  <label htmlFor={inputId}>
-    {labelText}
-  </label>
-  <input
-    type="checkbox"
-    role="switch"
-    aria-checked={`${checked}`}
-    disabled={disabled}
-    id={inputId}
-  />
+render() {
+  return (
+    <Host>
+      <label>
+        {this.labelText}
+        <input type="checkbox" role="switch" {...this.inheritedAttributes} /> 
+      </label>
+    </Host>
+  )
+}
 ```
 
-
 #### Hidden Input
 
 A helper function to render a hidden input has been added, it can be added in the `render`:
diff --git a/BREAKING.md b/BREAKING.md
index 5291db672f..27696cf2cb 100644
--- a/BREAKING.md
+++ b/BREAKING.md
@@ -254,11 +254,11 @@ For more information on styling toast buttons, refer to the [Toast Theming docum
 
 <h4 id="version-8x-range">Range</h4>
 
-- The `legacy` property and support for the legacy syntax, which involved placing an `ion-range` inside of an `ion-item` with an `ion-label`, have been removed. For more information on migrating from the legacy range syntax, refer to the [Range documentation](https://ionicframework.com/docs/api/range#migrating-from-legacy-range-syntax).
+- The `legacy` property and support for the legacy syntax, which involved placing an `ion-range` inside of an `ion-item` with an `ion-label`, have been removed. Ionic will also no longer attempt to automatically associate form controls with sibling `<label>` elements as these label elements are now used inside the form control. Developers should provide a label (either visible text or `aria-label`) directly to the form control. For more information on migrating from the legacy range syntax, refer to the [Range documentation](https://ionicframework.com/docs/api/range#migrating-from-legacy-range-syntax).
 
 <h4 id="version-8x-select">Select</h4>
 
-- The `legacy` property and support for the legacy syntax, which involved placing an `ion-select` inside of an `ion-item` with an `ion-label`, have been removed. For more information on migrating from the legacy select syntax, refer to the [Select documentation](https://ionicframework.com/docs/api/select#migrating-from-legacy-select-syntax).
+- The `legacy` property and support for the legacy syntax, which involved placing an `ion-select` inside of an `ion-item` with an `ion-label`, have been removed. Ionic will also no longer attempt to automatically associate form controls with sibling `<label>` elements as these label elements are now used inside the form control. Developers should provide a label (either visible text or `aria-label`) directly to the form control. For more information on migrating from the legacy select syntax, refer to the [Select documentation](https://ionicframework.com/docs/api/select#migrating-from-legacy-select-syntax).
 
 <h4 id="version-8x-textarea">Textarea</h4>
 
diff --git a/core/src/components/range/range.tsx b/core/src/components/range/range.tsx
index d3339a1317..b9e19b3197 100644
--- a/core/src/components/range/range.tsx
+++ b/core/src/components/range/range.tsx
@@ -2,7 +2,7 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core';
 import { Component, Element, Event, Host, Prop, State, Watch, h } from '@stencil/core';
 import { findClosestIonContent, disableContentScrollY, resetContentScrollY } from '@utils/content';
 import type { Attributes } from '@utils/helpers';
-import { inheritAriaAttributes, clamp, debounceEvent, getAriaLabel, renderHiddenInput } from '@utils/helpers';
+import { inheritAriaAttributes, clamp, debounceEvent, renderHiddenInput } from '@utils/helpers';
 import { printIonWarning } from '@utils/logging';
 import { isRTL } from '@utils/rtl';
 import { createColorClasses, hostContext } from '@utils/theme';
@@ -624,28 +624,16 @@ export class Range implements ComponentInterface {
       min,
       max,
       step,
-      el,
       handleKeyboard,
       pressedKnob,
       disabled,
       pin,
       ratioLower,
       ratioUpper,
-      inheritedAttributes,
-      rangeId,
       pinFormatter,
+      inheritedAttributes,
     } = this;
 
-    /**
-     * Look for external label, ion-label, or aria-labelledby.
-     * If none, see if user placed an aria-label on the host
-     * and use that instead.
-     */
-    let { labelText } = getAriaLabel(el, rangeId!);
-    if (labelText === undefined || labelText === null) {
-      labelText = inheritedAttributes['aria-label'];
-    }
-
     let barStart = `${ratioLower * 100}%`;
     let barEnd = `${100 - ratioUpper * 100}%`;
 
@@ -715,11 +703,6 @@ export class Range implements ComponentInterface {
       }
     }
 
-    let labelledBy: string | undefined;
-    if (this.hasLabel) {
-      labelledBy = 'range-label';
-    }
-
     return (
       <div
         class="range-slider"
@@ -791,8 +774,7 @@ export class Range implements ComponentInterface {
           handleKeyboard,
           min,
           max,
-          labelText,
-          labelledBy,
+          inheritedAttributes,
         })}
 
         {this.dualKnobs &&
@@ -807,8 +789,7 @@ export class Range implements ComponentInterface {
             handleKeyboard,
             min,
             max,
-            labelText,
-            labelledBy,
+            inheritedAttributes,
           })}
       </div>
     );
@@ -887,27 +868,13 @@ interface RangeKnob {
   pressed: boolean;
   pin: boolean;
   pinFormatter: PinFormatter;
-  labelText?: string | null;
-  labelledBy?: string;
+  inheritedAttributes: Attributes;
   handleKeyboard: (name: KnobName, isIncrease: boolean) => void;
 }
 
 const renderKnob = (
   rtl: boolean,
-  {
-    knob,
-    value,
-    ratio,
-    min,
-    max,
-    disabled,
-    pressed,
-    pin,
-    handleKeyboard,
-    labelText,
-    labelledBy,
-    pinFormatter,
-  }: RangeKnob
+  { knob, value, ratio, min, max, disabled, pressed, pin, handleKeyboard, pinFormatter, inheritedAttributes }: RangeKnob
 ) => {
   const start = rtl ? 'right' : 'left';
 
@@ -919,6 +886,9 @@ const renderKnob = (
     return style;
   };
 
+  // The aria label should be preferred over visible text if both are specified
+  const ariaLabel = inheritedAttributes['aria-label'];
+
   return (
     <div
       onKeyDown={(ev: KeyboardEvent) => {
@@ -946,8 +916,8 @@ const renderKnob = (
       style={knobStyle()}
       role="slider"
       tabindex={disabled ? -1 : 0}
-      aria-label={labelledBy === undefined ? labelText : null}
-      aria-labelledby={labelledBy !== undefined ? labelledBy : null}
+      aria-label={ariaLabel !== undefined ? ariaLabel : null}
+      aria-labelledby={ariaLabel === undefined ? 'range-label' : null}
       aria-valuemin={min}
       aria-valuemax={max}
       aria-disabled={disabled ? 'true' : null}
diff --git a/core/src/components/range/test/label/range.spec.ts b/core/src/components/range/test/label/range.spec.ts
index c7006561ad..d714e157e8 100644
--- a/core/src/components/range/test/label/range.spec.ts
+++ b/core/src/components/range/test/label/range.spec.ts
@@ -20,4 +20,34 @@ describe('range: label', () => {
     expect(propEl).not.toBeNull();
     expect(slotEl).toBeNull();
   });
+  it('should prefer aria label if both attribute and visible text provided', async () => {
+    const page = await newSpecPage({
+      components: [Range],
+      html: `
+        <ion-range aria-label="Aria Label Text" label="Label Prop Text"></ion-range>
+      `,
+    });
+
+    const range = page.body.querySelector('ion-range')!;
+
+    const nativeSlider = range.shadowRoot!.querySelector('.range-knob-handle')!;
+
+    expect(nativeSlider.getAttribute('aria-label')).toBe('Aria Label Text');
+    expect(nativeSlider.getAttribute('aria-labelledby')).toBe(null);
+  });
+  it('should prefer visible label if only visible text provided', async () => {
+    const page = await newSpecPage({
+      components: [Range],
+      html: `
+        <ion-range label="Label Prop Text"></ion-range>
+      `,
+    });
+
+    const range = page.body.querySelector('ion-range')!;
+
+    const nativeSlider = range.shadowRoot!.querySelector('.range-knob-handle')!;
+
+    expect(nativeSlider.getAttribute('aria-label')).toBe(null);
+    expect(nativeSlider.getAttribute('aria-labelledby')).toBe('range-label');
+  });
 });
diff --git a/core/src/components/select/select.tsx b/core/src/components/select/select.tsx
index 0680c02406..740adad020 100644
--- a/core/src/components/select/select.tsx
+++ b/core/src/components/select/select.tsx
@@ -2,7 +2,7 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core';
 import { Component, Element, Event, Host, Method, Prop, State, Watch, h, forceUpdate } from '@stencil/core';
 import type { NotchController } from '@utils/forms';
 import { compareOptions, createNotchController, isOptionSelected } from '@utils/forms';
-import { focusVisibleElement, getAriaLabel, renderHiddenInput, inheritAttributes } from '@utils/helpers';
+import { focusVisibleElement, renderHiddenInput, inheritAttributes } from '@utils/helpers';
 import type { Attributes } from '@utils/helpers';
 import { actionSheetController, alertController, popoverController } from '@utils/overlays';
 import type { OverlaySelect } from '@utils/overlays-interface';
@@ -874,10 +874,11 @@ export class Select implements ComponentInterface {
   }
 
   private get ariaLabel() {
-    const { placeholder, el, inputId, inheritedAttributes } = this;
+    const { placeholder, inheritedAttributes } = this;
     const displayValue = this.getText();
-    const { labelText } = getAriaLabel(el, inputId);
-    const definedLabel = this.labelText ?? inheritedAttributes['aria-label'] ?? labelText;
+
+    // The aria label should be preferred over visible text if both are specified
+    const definedLabel = inheritedAttributes['aria-label'] ?? this.labelText;
 
     /**
      * If developer has specified a placeholder
diff --git a/core/src/components/select/test/select.spec.tsx b/core/src/components/select/test/select.spec.tsx
index 1cfd4e8bb8..ef1b23306a 100644
--- a/core/src/components/select/test/select.spec.tsx
+++ b/core/src/components/select/test/select.spec.tsx
@@ -68,6 +68,34 @@ describe('ion-select', () => {
     expect(propEl).not.toBe(null);
     expect(slotEl).toBe(null);
   });
+  it('should prefer aria label if both attribute and visible text provided', async () => {
+    const page = await newSpecPage({
+      components: [Select],
+      html: `
+        <ion-select aria-label="Aria Label Text" label="Label Prop Text"></ion-select>
+      `,
+    });
+
+    const select = page.body.querySelector('ion-select')!;
+
+    const nativeButton = select.shadowRoot!.querySelector('button')!;
+
+    expect(nativeButton.getAttribute('aria-label')).toBe('Aria Label Text');
+  });
+  it('should prefer visible label if only visible text provided', async () => {
+    const page = await newSpecPage({
+      components: [Select],
+      html: `
+        <ion-select label="Label Prop Text"></ion-select>
+      `,
+    });
+
+    const select = page.body.querySelector('ion-select')!;
+
+    const nativeButton = select.shadowRoot!.querySelector('button')!;
+
+    expect(nativeButton.getAttribute('aria-label')).toBe('Label Prop Text');
+  });
 });
 
 describe('select: slot interactivity', () => {
diff --git a/core/src/utils/helpers.ts b/core/src/utils/helpers.ts
index 6b17098ae6..97681f6652 100644
--- a/core/src/utils/helpers.ts
+++ b/core/src/utils/helpers.ts
@@ -273,64 +273,6 @@ export const focusVisibleElement = (el: HTMLElement) => {
   }
 };
 
-/**
- * This method is used for Ionic's input components that use Shadow DOM. In
- * order to properly label the inputs to work with screen readers, we need
- * to get the text content of the label outside of the shadow root and pass
- * it to the input inside of the shadow root.
- *
- * Referencing label elements by id from outside of the component is
- * impossible due to the shadow boundary, read more here:
- * https://developer.salesforce.com/blogs/2020/01/accessibility-for-web-components.html
- *
- * @param componentEl The shadow element that needs the aria label
- * @param inputId The unique identifier for the input
- */
-export const getAriaLabel = (
-  componentEl: HTMLElement,
-  inputId: string
-): { label: Element | null; labelId: string; labelText: string | null | undefined } => {
-  let labelText;
-
-  // If the user provides their own label via the aria-labelledby attr
-  // we should use that instead of looking for an ion-label
-  const labelledBy = componentEl.getAttribute('aria-labelledby');
-
-  // Grab the id off of the component in case they are using
-  // a custom label using the label element
-  const componentId = componentEl.id;
-
-  let labelId = labelledBy !== null && labelledBy.trim() !== '' ? labelledBy : inputId + '-lbl';
-
-  let label = labelledBy !== null && labelledBy.trim() !== '' ? document.getElementById(labelledBy) : null;
-
-  if (label) {
-    if (labelledBy === null) {
-      label.id = labelId;
-    }
-
-    labelText = label.textContent;
-    label.setAttribute('aria-hidden', 'true');
-
-    // if there is no label, check to see if the user has provided
-    // one by setting an id on the component and using the label element
-  } else if (componentId.trim() !== '') {
-    label = document.querySelector(`label[for="${componentId}"]`);
-
-    if (label) {
-      if (label.id !== '') {
-        labelId = label.id;
-      } else {
-        label.id = labelId = `${componentId}-lbl`;
-      }
-
-      labelText = label.textContent;
-    }
-  }
-
-  return { label, labelId, labelText };
-};
-
 /**
  * This method is used to add a hidden input to a host element that contains
  * a Shadow DOM. It does not add the input inside of the Shadow root which
diff --git a/core/src/utils/test/aria.spec.ts b/core/src/utils/test/aria.spec.ts
deleted file mode 100644
index ec992073a7..0000000000
--- a/core/src/utils/test/aria.spec.ts
+++ /dev/null
@@ -1,98 +0,0 @@
-import { newSpecPage } from '@stencil/core/testing';
-
-import { Item } from '../../components/item/item';
-import { Label } from '../../components/label/label';
-import { Toggle } from '../../components/toggle/toggle';
-import { getAriaLabel } from '../helpers';
-
-// TODO FW-5969
-describe.skip('getAriaLabel()', () => {
-  it('should correctly link component to label', async () => {
-    const page = await newSpecPage({
-      components: [Item, Label, Toggle],
-      html: `
-        <ion-item>
-          <ion-label>My Label</ion-label>
-          <ion-toggle></ion-toggle>
-        </ion-item>
-      `,
-    });
-
-    const toggle = page.body.querySelector('ion-toggle')!;
-
-    const { label, labelId, labelText } = getAriaLabel(toggle, 'ion-tg-0');
-
-    expect(labelText).toEqual('My Label');
-    expect(labelId).toEqual('ion-tg-0-lbl');
-    expect(label).toEqual(page.body.querySelector('ion-label'));
-  });
-
-  it('should correctly link component when using custom label', async () => {
-    const page = await newSpecPage({
-      components: [Toggle],
-      html: `
-        <div id="my-label">Hello World</div>
-        <ion-toggle legacy="true" aria-labelledby="my-label"></ion-toggle>
-      `,
-    });
-
-    const toggle = page.body.querySelector('ion-toggle')!;
-
-    const { label, labelId, labelText } = getAriaLabel(toggle, 'ion-tg-0');
-
-    expect(labelText).toEqual('Hello World');
-    expect(labelId).toEqual('my-label');
-    expect(label).toEqual(page.body.querySelector('#my-label'));
-  });
-
-  it('should correctly link component when special characters are used', async () => {
-    const page = await newSpecPage({
-      components: [Toggle],
-      html: `
-        <div id="id.1">Hello World</div>
-        <ion-toggle legacy="true" aria-labelledby="id.1"></ion-toggle>
-      `,
-    });
-
-    const toggle = page.body.querySelector('ion-toggle')!;
-
-    const { labelId, labelText } = getAriaLabel(toggle, 'ion-tg-0');
-
-    expect(labelText).toEqual('Hello World');
-    expect(labelId).toEqual('id.1');
-  });
-
-  it('should only set the label id if one was not set already', async () => {
-    const page = await newSpecPage({
-      components: [Toggle],
-      html: `
-        <label id="my-id" for="id.1">Hello World</label>
-        <ion-toggle legacy="true" id="id.1"></ion-toggle>
-      `,
-    });
-
-    const toggle = page.body.querySelector('ion-toggle')!;
-
-    const { labelId, labelText } = getAriaLabel(toggle, 'ion-tg-0');
-
-    expect(labelText).toEqual('Hello World');
-    expect(labelId).toEqual('my-id');
-  });
-
-  it('should set label id', async () => {
-    const page = await newSpecPage({
-      components: [Toggle],
-      html: `
-        <label for="id.1">Hello World</label>
-        <ion-toggle legacy="true" id="id.1"></ion-toggle>
-      `,
-    });
-
-    const toggle = page.body.querySelector('ion-toggle')!;
-
-    const { labelId, labelText } = getAriaLabel(toggle, 'ion-tg-0');
-
-    expect(labelText).toEqual('Hello World');
-    expect(labelId).toEqual('id.1-lbl');
-  });
-});