From 8a7d3d5a40f360b5bb3314cc0cae66277151427b Mon Sep 17 00:00:00 2001
From: JesmoDev <26099018+JesmoDev@users.noreply.github.com>
Date: Thu, 25 Jul 2024 16:21:10 +0200
Subject: [PATCH] fix(uui-radio): keyboard navigation does not work as intended
 (#870)

* remove tabindex

* fix wrapping keyboard nav

* Improve keyboard navigation by emulating native radio group behavior

* remove unused code

* fix double keypress issue

* cleanup

* fix selection bug

* rename function

* change focus outline functionality to pure css

* remove aria hidden as it is not needed anymore

* remove redundant get,set

* cleanup radio

* cleanup

* fix findRadio function to fix focus issues

* remove keypress eventListener

* Fix pre checked radios

* fix test to use same keydown event as the component

* make sonar happy

* fix tab navigation when there is a pre-checked radio
---
 .../uui-radio/lib/uui-radio-group.element.ts  | 315 +++++++++---------
 .../uui-radio/lib/uui-radio-group.test.ts     |   2 +-
 packages/uui-radio/lib/uui-radio.element.ts   |  93 ++----
 3 files changed, 182 insertions(+), 228 deletions(-)

diff --git a/packages/uui-radio/lib/uui-radio-group.element.ts b/packages/uui-radio/lib/uui-radio-group.element.ts
index 78ff05dec..6cff562be 100644
--- a/packages/uui-radio/lib/uui-radio-group.element.ts
+++ b/packages/uui-radio/lib/uui-radio-group.element.ts
@@ -12,6 +12,7 @@ const ARROW_UP = 'ArrowUp';
 const ARROW_RIGHT = 'ArrowRight';
 const ARROW_DOWN = 'ArrowDown';
 const SPACE = ' ';
+const ENTER = 'Enter';
 
 /**
  * @element uui-radio-group
@@ -49,22 +50,21 @@ export class UUIRadioGroupElement extends UUIFormControlMixin(LitElement, '') {
   }
   set value(newValue) {
     super.value = newValue;
-    if (!newValue || newValue === '') {
-      this._makeFirstEnabledFocusable();
-    }
-    this._updateRadioElementsCheckedState(newValue as string);
+    this.#updateRadioElementsCheckedState(newValue as string);
   }
 
-  private _selected: number | null = null;
+  #selected: number | null = null;
+  #radioElements: UUIRadioElement[] = [];
 
   constructor() {
     super();
-    this.addEventListener('keydown', this._onKeydown);
-    this.addEventListener('keypress', this._onKeypress);
+    this.addEventListener('keydown', this.#onKeydown);
+    this.addEventListener('focusin', this.#onFocusIn);
+    this.addEventListener('focusout', this.#onFocusOut);
 
     // Wait for the radio elements to be added to the dom before updating the checked state.
     this.updateComplete.then(() => {
-      this._updateRadioElementsCheckedState(this.value as string);
+      this.#updateRadioElementsCheckedState(this.value as string);
     });
   }
 
@@ -73,111 +73,165 @@ export class UUIRadioGroupElement extends UUIFormControlMixin(LitElement, '') {
     if (!this.hasAttribute('role')) this.setAttribute('role', 'radiogroup');
   }
 
+  updated(_changedProperties: Map<string | number | symbol, unknown>) {
+    super.updated(_changedProperties);
+    if (_changedProperties.has('disabled')) {
+      this.#setDisableOnRadios(this.disabled);
+    }
+
+    if (_changedProperties.has('readonly')) {
+      this.#setReadonlyOnRadios(this.readonly);
+    }
+
+    if (_changedProperties.has('name')) {
+      this.#setNameOnRadios(_changedProperties.get('name') as string);
+    }
+  }
+
   /**
    * This method enables <label for="..."> to focus the select
    */
-  async focus() {
+  public async focus() {
     await this.updateComplete;
-    if (this._selected !== null) {
-      this._radioElements[this._selected]?.focus();
+    if (this.#selected !== null) {
+      this.#radioElements[this.#selected]?.focus();
     } else {
-      this._findNextEnabledElement()?.focus();
+      this.#findAdjacentRadioElement(1, false)?.focus();
     }
   }
-  async blur() {
+  public async blur() {
     await this.updateComplete;
-    if (this._selected !== null) {
-      this._radioElements[this._selected]?.blur();
+    if (this.#selected !== null) {
+      this.#radioElements[this.#selected]?.blur();
     } else {
-      this._findNextEnabledElement()?.blur();
+      this.#findAdjacentRadioElement(1, false)?.blur();
     }
   }
-  async click() {
+  public async click() {
     await this.updateComplete;
-    if (this._selected !== null) {
-      this._radioElements[this._selected]?.click();
+    if (this.#selected !== null) {
+      this.#radioElements[this.#selected]?.click();
     } else {
-      this._findNextEnabledElement()?.click();
+      this.#findAdjacentRadioElement(1, false)?.click();
     }
   }
 
   protected getFormElement(): HTMLElement | undefined {
-    if (this._radioElements && this._selected) {
-      return this._radioElements[this._selected];
+    if (this.#radioElements && this.#selected) {
+      return this.#radioElements[this.#selected];
     }
     return undefined;
   }
 
-  private _radioElements: UUIRadioElement[] = [];
-  private _setNameOnRadios(name: string) {
-    this._radioElements?.forEach(el => (el.name = name));
-  }
-
-  private _updateRadioElementsCheckedState(
-    newValue: FormData | FormDataEntryValue,
-  ) {
-    this._radioElements.forEach((el, index) => {
-      if (el.value === newValue) {
-        el.checked = true;
-        this._selected = index;
+  #onFocusIn = (event: FocusEvent) => {
+    // Ensure only the selected radio element is focusable to improve tab navigation
+    // by skipping unselected radios and moving to the next radio group or focusable element.
+    this.#radioElements?.forEach(el => {
+      if (el !== event.target) {
+        el.makeUnfocusable();
       } else {
-        el.checked = false;
+        el.makeFocusable();
       }
     });
-  }
+  };
 
-  private _setDisableOnRadios(value: boolean) {
-    this._radioElements?.forEach(el => (el.disabled = value));
-  }
+  #onFocusOut = (event: FocusEvent) => {
+    // When a focus event is fired from a radio, check if the focus is still inside the radio group
+    if (this.contains(event.relatedTarget as Node)) return;
 
-  private _setReadonlyOnRadios(value: boolean) {
-    this._radioElements?.forEach(el => (el.readonly = value));
-  }
+    // If there is a selected radio element, no action is needed since only the selected radio should remain focusable
+    if (this.#selected !== null) return;
+
+    // If focus has moved outside the radio group and no radio is selected, make all radio elements focusable again
+    this.#radioElements?.forEach(el => {
+      el.makeFocusable();
+    });
+  };
+
+  #onChildBlur = () => {
+    this.pristine = false;
+  };
+
+  #onSelectClick = (e: UUIRadioEvent) => {
+    if (e.target.checked === true) {
+      this.value = e.target.value;
+      this.#fireChangeEvent();
+    }
+  };
+
+  #onKeydown = (e: KeyboardEvent) => {
+    switch (e.key) {
+      case ARROW_LEFT:
+      case ARROW_UP: {
+        e.preventDefault();
+        this.#selectPreviousRadio();
+        break;
+      }
+      case ARROW_RIGHT:
+      case ARROW_DOWN: {
+        e.preventDefault();
+        this.#selectNextRadio();
+        break;
+      }
+
+      case SPACE: {
+        if (this.#selected === null) {
+          this.value = this.#findAdjacentRadioElement(1, false)
+            ?.value as string;
+          this.#fireChangeEvent();
+        }
+        break;
+      }
+
+      case ENTER:
+        this.submit();
+    }
+  };
 
-  private _handleSlotChange(e: Event) {
+  #onSlotChange(e: Event) {
     e.stopPropagation();
     // TODO: make sure to diff new and old ones to only add and remove event listeners on relevant elements.
 
-    this._radioElements?.forEach(el => {
+    this.#radioElements?.forEach(el => {
       el.removeEventListener(
         UUIRadioEvent.CHANGE,
         // @ts-ignore TODO: fix typescript error
-        this._handleSelectOnClick as EventHandlerNonNull,
+        this.#onSelectClick as EventHandlerNonNull,
       );
-      el.removeEventListener('blur', this._onChildBlur);
+      el.removeEventListener('blur', this.#onChildBlur);
     });
 
-    this._selected = null;
-    this._radioElements = (e.target as HTMLSlotElement)
+    this.#selected = null;
+    this.#radioElements = (e.target as HTMLSlotElement)
       .assignedElements({ flatten: true })
       .filter(el => el instanceof UUIRadioElement) as UUIRadioElement[];
 
     // Only continue if there are radio elements
-    if (this._radioElements.length === 0) return;
+    if (this.#radioElements.length === 0) return;
 
-    this._radioElements.forEach(el => {
+    this.#radioElements.forEach(el => {
       el.addEventListener(
         UUIRadioEvent.CHANGE,
         // @ts-ignore TODO: fix typescript error
-        this._handleSelectOnClick as EventHandlerNonNull,
+        this.#onSelectClick as EventHandlerNonNull,
       );
-      el.addEventListener('blur', this._onChildBlur);
+      el.addEventListener('blur', this.#onChildBlur);
     });
 
-    this._setNameOnRadios(this.name);
+    this.#setNameOnRadios(this.name);
 
     if (this.disabled) {
-      this._setDisableOnRadios(true);
+      this.#setDisableOnRadios(true);
     }
 
     if (this.readonly) {
-      this._setReadonlyOnRadios(true);
+      this.#setReadonlyOnRadios(true);
     }
 
-    const checkedRadios = this._radioElements.filter(el => el.checked === true);
+    const checkedRadios = this.#radioElements.filter(el => el.checked === true);
 
     if (checkedRadios.length > 1) {
-      this._radioElements.forEach(el => {
+      this.#radioElements.forEach(el => {
         el.checked = false;
       });
       this.value = '';
@@ -192,133 +246,84 @@ export class UUIRadioGroupElement extends UUIFormControlMixin(LitElement, '') {
     if (checkedRadios.length === 1) {
       const firstCheckedRadio = checkedRadios[0];
       this.value = firstCheckedRadio.value;
-      this._selected = this._radioElements.indexOf(firstCheckedRadio);
-
-      if (
-        firstCheckedRadio.disabled === false &&
-        firstCheckedRadio.readonly === false
-      ) {
-        this._radioElements.forEach(el => {
-          el.makeUnfocusable();
-        });
-        firstCheckedRadio.makeFocusable();
-      } else {
-        this._makeFirstEnabledFocusable();
-      }
-    } else {
-      this._makeFirstEnabledFocusable();
+      this.#selected = this.#radioElements.indexOf(firstCheckedRadio);
     }
   }
 
-  private _makeFirstEnabledFocusable() {
-    this._selected = null;
-    this._radioElements?.forEach(el => {
-      el.makeUnfocusable();
-    });
-    this._findNextEnabledElement()?.makeFocusable();
+  #setNameOnRadios(name: string) {
+    this.#radioElements?.forEach(el => (el.name = name));
   }
 
-  private _findNextEnabledElement(
-    direction: number = 1,
-  ): UUIRadioElement | null {
-    if (!this._radioElements) {
-      return null;
-    }
-    const origin = this._selected || 0;
-    const len = this._radioElements.length;
-    let i = this._selected === null ? 0 : 1; //If we have something selected we will skip checking it self.
-    while (i < len) {
-      let checkIndex = (origin + i * direction) % len;
-      const radioElement = this._radioElements[checkIndex];
-
-      if (checkIndex < 0) {
-        checkIndex += len;
-      }
+  #updateRadioElementsCheckedState(newValue: FormData | FormDataEntryValue) {
+    const notChecked: Array<UUIRadioElement> = [];
 
-      if (radioElement.disabled === false && radioElement.readonly === false) {
-        return radioElement;
+    this.#radioElements.forEach((el, index) => {
+      if (el.value === newValue) {
+        el.checked = true;
+        el.makeFocusable();
+        this.#selected = index;
+      } else {
+        el.checked = false;
+        notChecked.push(el);
       }
+    });
 
-      i++;
+    // If there is a selected radio, make all other radios unfocusable.
+    if (this.#selected !== null) {
+      notChecked.forEach(el => el.makeUnfocusable());
     }
-    return null;
   }
 
-  private _selectPreviousElement() {
-    this.value = this._findNextEnabledElement(-1)?.value || '';
-    this._radioElements[this._selected || 0]?.focus();
-    this._fireChangeEvent();
+  #setDisableOnRadios(value: boolean) {
+    this.#radioElements?.forEach(el => (el.disabled = value));
   }
 
-  private _selectNextElement() {
-    this.value = this._findNextEnabledElement()?.value || '';
-    this._radioElements[this._selected || 0]?.focus();
-    this._fireChangeEvent();
+  #setReadonlyOnRadios(value: boolean) {
+    this.#radioElements?.forEach(el => (el.readonly = value));
   }
 
-  private _onKeydown(e: KeyboardEvent) {
-    switch (e.key) {
-      case ARROW_LEFT:
-      case ARROW_UP: {
-        e.preventDefault();
-        this._selectPreviousElement();
-        break;
-      }
-      case ARROW_RIGHT:
-      case ARROW_DOWN: {
-        e.preventDefault();
-        this._selectNextElement();
-        break;
-      }
+  #findAdjacentRadioElement(
+    direction = 1,
+    skipFirst = true,
+  ): UUIRadioElement | null {
+    if (!this.#radioElements || this.#radioElements.length === 0) return null;
 
-      case SPACE: {
-        if (this._selected === null) {
-          this.value = this._findNextEnabledElement()?.value as string;
-          this._fireChangeEvent();
+    const len = this.#radioElements.length;
+    let index = this.#selected ?? 0;
+
+    for (let i = 0; i < len + 1; i++) {
+      if (!skipFirst || i > 0) {
+        const radioElement = this.#radioElements[index];
+        if (!radioElement.disabled && !radioElement.readonly) {
+          return radioElement;
         }
       }
+
+      index = (index + direction + len) % len;
     }
+
+    return null;
   }
 
-  private _onKeypress(e: KeyboardEvent): void {
-    if (e.key == 'Enter') {
-      this.submit();
-    }
+  #selectPreviousRadio() {
+    this.value = this.#findAdjacentRadioElement(-1)?.value ?? '';
+    this.#radioElements[this.#selected ?? 0]?.focus();
+    this.#fireChangeEvent();
   }
 
-  private _fireChangeEvent() {
-    this.pristine = false;
-    this.dispatchEvent(new UUIRadioGroupEvent(UUIRadioGroupEvent.CHANGE));
+  #selectNextRadio() {
+    this.value = this.#findAdjacentRadioElement()?.value ?? '';
+    this.#radioElements[this.#selected ?? 0]?.focus();
+    this.#fireChangeEvent();
   }
 
-  private _onChildBlur = () => {
+  #fireChangeEvent() {
     this.pristine = false;
-  };
-
-  private _handleSelectOnClick = (e: UUIRadioEvent) => {
-    if (e.target.checked === true) {
-      this.value = e.target.value;
-      this._fireChangeEvent();
-    }
-  };
-
-  updated(_changedProperties: Map<string | number | symbol, unknown>) {
-    super.updated(_changedProperties);
-    if (_changedProperties.has('disabled')) {
-      this._setDisableOnRadios(this.disabled);
-    }
-
-    if (_changedProperties.has('readonly')) {
-      this._setReadonlyOnRadios(this.readonly);
-    }
-
-    if (_changedProperties.has('name')) {
-      this._setNameOnRadios(_changedProperties.get('name') as string);
-    }
+    this.dispatchEvent(new UUIRadioGroupEvent(UUIRadioGroupEvent.CHANGE));
   }
 
   render() {
-    return html` <slot @slotchange=${this._handleSlotChange}></slot> `;
+    return html` <slot @slotchange=${this.#onSlotChange}></slot> `;
   }
 
   static styles = [
diff --git a/packages/uui-radio/lib/uui-radio-group.test.ts b/packages/uui-radio/lib/uui-radio-group.test.ts
index 73b79e4d9..bb74fbab3 100644
--- a/packages/uui-radio/lib/uui-radio-group.test.ts
+++ b/packages/uui-radio/lib/uui-radio-group.test.ts
@@ -195,7 +195,7 @@ describe('UuiRadioGroup in a Form', () => {
   describe('submit', () => {
     it('should submit when pressing enter', async () => {
       const listener = oneEvent(formElement, 'submit', false);
-      element.dispatchEvent(new KeyboardEvent('keypress', { key: 'Enter' }));
+      element.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter' }));
 
       const event = await listener;
       expect(event).to.exist;
diff --git a/packages/uui-radio/lib/uui-radio.element.ts b/packages/uui-radio/lib/uui-radio.element.ts
index b85df1a54..b249ba92a 100644
--- a/packages/uui-radio/lib/uui-radio.element.ts
+++ b/packages/uui-radio/lib/uui-radio.element.ts
@@ -18,7 +18,7 @@ import { UUIRadioEvent } from './UUIRadioEvent';
 @defineElement('uui-radio')
 export class UUIRadioElement extends LitElement {
   @query('#input')
-  private inputElement!: HTMLInputElement;
+  private _inputElement!: HTMLInputElement;
 
   /**
    * This is a name property of the `<uui-radio>` component. It reflects the behaviour of the native `<input />` element and its name attribute.
@@ -42,24 +42,7 @@ export class UUIRadioElement extends LitElement {
   public label = '';
 
   @property({ type: Boolean, reflect: true })
-  public get checked() {
-    return this._checked;
-  }
-  public set checked(value) {
-    const oldValue = this._checked;
-    this._checked = value;
-    if (value === true) {
-      this.setAttribute('aria-checked', '');
-      if (!this.disabled) {
-        this.setAttribute('tabindex', '0');
-      }
-    } else {
-      this.setAttribute('tabindex', '-1');
-      this.removeAttribute('aria-checked');
-    }
-    this.requestUpdate('checked', oldValue);
-  }
-  private _checked = false;
+  public checked = false;
 
   /**
    * Disables the input.
@@ -68,18 +51,7 @@ export class UUIRadioElement extends LitElement {
    * @default false
    */
   @property({ type: Boolean, reflect: true })
-  get disabled() {
-    return this._disabled;
-  }
-  set disabled(newVal) {
-    const oldVal = this._disabled;
-    this._disabled = newVal;
-
-    this.setAttribute('aria-hidden', newVal ? 'true' : 'false');
-    this.setAttribute('tabindex', newVal ? '-1' : '0');
-    this.requestUpdate('disabled', oldVal);
-  }
-  private _disabled = false;
+  public disabled = false;
 
   /**
    * Sets the input to readonly mode, meaning value cannot be changed but still able to read and select its content.
@@ -90,39 +62,15 @@ export class UUIRadioElement extends LitElement {
   @property({ type: Boolean, reflect: true })
   readonly = false;
 
-  constructor() {
-    super();
-    this.addEventListener('mousedown', this.#hideFocusOutline);
-    this.addEventListener('blur', this.#showFocusOutline);
-  }
-
-  focus() {
-    this.inputElement.focus();
+  public focus() {
+    this._inputElement.focus();
   }
-  click() {
-    this.inputElement.click();
-  }
-
-  #showFocusOutline = () => {
-    this.style.setProperty('--uui-show-focus-outline', '1');
-  };
-
-  #hideFocusOutline = () => {
-    this.style.setProperty('--uui-show-focus-outline', '0');
-  };
-
-  private _onChange(e: Event) {
-    e.stopPropagation();
-    const checked = this.inputElement.checked;
-    this.checked = checked;
-    if (checked) {
-      this.focus();
-    }
-    this.dispatchEvent(new UUIRadioEvent(UUIRadioEvent.CHANGE));
+  public click() {
+    this._inputElement.click();
   }
 
   /**
-   * Call to uncheck the element. This method changes the tabindex and aria -checked attributes.
+   * Call to uncheck the element
    * @method uncheck
    */
   public uncheck() {
@@ -142,11 +90,11 @@ export class UUIRadioElement extends LitElement {
    */
   public makeFocusable() {
     if (!this.disabled) {
-      this.setAttribute('tabindex', '0');
+      this.removeAttribute('tabindex');
     }
   }
   /**
-   * Call to make the element focusable, this sets tabindex to 0.
+   * Call to make the element focusable, this sets tabindex to -1.
    * @method makeUnfocusable
    */
   public makeUnfocusable() {
@@ -155,12 +103,14 @@ export class UUIRadioElement extends LitElement {
     }
   }
 
-  connectedCallback() {
-    super.connectedCallback();
-    //if (!this.hasAttribute('role')) this.setAttribute('role', 'radio');
-    if (!this.hasAttribute('tabindex')) this.setAttribute('tabindex', '-1');
-    if (!this.hasAttribute('aria-checked'))
-      this.removeAttribute('aria-checked');
+  #onChange(e: Event) {
+    e.stopPropagation();
+    const checked = this._inputElement.checked;
+    this.checked = checked;
+    if (checked) {
+      this.focus();
+    }
+    this.dispatchEvent(new UUIRadioEvent(UUIRadioEvent.CHANGE));
   }
 
   render() {
@@ -172,7 +122,7 @@ export class UUIRadioElement extends LitElement {
         value=${this.value}
         .checked=${this.checked}
         .disabled=${this.disabled || this.readonly}
-        @change=${this._onChange} />
+        @change=${this.#onChange} />
       <div id="button"></div>
       <div id="label">
         ${this.label ? html`<span>${this.label}</span>` : html`<slot></slot>`}
@@ -250,9 +200,8 @@ export class UUIRadioElement extends LitElement {
       :host(:focus) {
         outline: none;
       }
-      :host(:focus) #button {
-        outline: calc(2px * var(--uui-show-focus-outline, 1)) solid
-          var(--uui-color-focus);
+      :host(:focus-within) input:focus-visible + #button {
+        outline: 2px solid var(--uui-color-focus);
       }
 
       input:checked ~ #button::after {