From fda881ba072ee11126e3e968fda9a445d395ffc1 Mon Sep 17 00:00:00 2001 From: yinon Date: Sat, 11 Mar 2023 01:56:14 +0200 Subject: [PATCH] fix(foundation): remove readonly from radio button (#6438) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(foundation): remove readonly from radio button readonly on radio button violates a WCAG rule. see #6437 there's no evidence readonly applies on radio button. it does on radio group however - [aria-readonly associated roles on MDN](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-readonly#associated_roles) doesn’t list radio - https://stackoverflow.com/questions/1953017/why-cant-radio-buttons-be-readonly - https://www.w3schools.com/jsref/dom_obj_radio.asp * remove any evidence of readonly from class * completely remove any readonly * Change files * group slotted radio readonly handling * docs update * Update change/@microsoft-fast-foundation-b9885563-e3e8-4c85-a55d-bac04c627fec.json Co-authored-by: Nicholas Rice <3213292+nicholasrice@users.noreply.github.com> * Update change/@microsoft-fast-foundation-b9885563-e3e8-4c85-a55d-bac04c627fec.json Co-authored-by: Nicholas Rice <3213292+nicholasrice@users.noreply.github.com> * prevent check of radio buttons * Update packages/web-components/fast-foundation/src/radio/radio.ts Co-authored-by: Nicholas Rice <3213292+nicholasrice@users.noreply.github.com> * revert * remove unused * FASTRadioGroup to be used as type only * fixup prettier and API report --------- Co-authored-by: Nicholas Rice <3213292+nicholasrice@users.noreply.github.com> Co-authored-by: Rob Eisenberg Co-authored-by: Chris Holt --- ...-b9885563-e3e8-4c85-a55d-bac04c627fec.json | 7 ++++ .../fast-foundation/docs/api-report.md | 7 +--- .../fast-foundation/src/radio-group/README.md | 1 - .../src/radio-group/radio-group.pw.spec.ts | 26 --------------- .../src/radio-group/radio-group.ts | 33 ++----------------- .../fast-foundation/src/radio/README.md | 22 ++++--------- .../src/radio/radio.pw.spec.ts | 11 ------- .../fast-foundation/src/radio/radio.spec.md | 6 ---- .../src/radio/radio.template.ts | 1 - .../fast-foundation/src/radio/radio.ts | 33 +++++++------------ .../src/radio/stories/radio.register.ts | 2 -- .../src/radio/stories/radio.stories.ts | 2 -- 12 files changed, 29 insertions(+), 122 deletions(-) create mode 100644 change/@microsoft-fast-foundation-b9885563-e3e8-4c85-a55d-bac04c627fec.json diff --git a/change/@microsoft-fast-foundation-b9885563-e3e8-4c85-a55d-bac04c627fec.json b/change/@microsoft-fast-foundation-b9885563-e3e8-4c85-a55d-bac04c627fec.json new file mode 100644 index 00000000000..07589d54677 --- /dev/null +++ b/change/@microsoft-fast-foundation-b9885563-e3e8-4c85-a55d-bac04c627fec.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "remove readonly support from fast radio", + "packageName": "@microsoft/fast-foundation", + "email": "yinon@hotmail.com", + "dependentChangeType": "prerelease" +} diff --git a/packages/web-components/fast-foundation/docs/api-report.md b/packages/web-components/fast-foundation/docs/api-report.md index c9f3bff58f4..f68f10db26c 100644 --- a/packages/web-components/fast-foundation/docs/api-report.md +++ b/packages/web-components/fast-foundation/docs/api-report.md @@ -1638,9 +1638,6 @@ export class FASTRadio extends FormAssociatedRadio implements RadioControl { // @beta keypressHandler(e: KeyboardEvent): boolean | void; name: string; - readOnly: boolean; - // (undocumented) - protected readOnlyChanged(): void; } // @public @@ -1667,8 +1664,6 @@ export class FASTRadioGroup extends FASTElement { protected nameChanged(): void; orientation: RadioGroupOrientation; readOnly: boolean; - // (undocumented) - protected readOnlyChanged(): void; // @internal (undocumented) slottedRadioButtons: HTMLElement[]; // (undocumented) @@ -2532,7 +2527,7 @@ export interface PropertyTarget { export type ProxyElement = HTMLSelectElement | HTMLTextAreaElement | HTMLInputElement; // @public -export type RadioControl = Pick; +export type RadioControl = Pick; // @public export const RadioGroupOrientation: { diff --git a/packages/web-components/fast-foundation/src/radio-group/README.md b/packages/web-components/fast-foundation/src/radio-group/README.md index 1b0a93595bd..738a3c0a992 100644 --- a/packages/web-components/fast-foundation/src/radio-group/README.md +++ b/packages/web-components/fast-foundation/src/radio-group/README.md @@ -88,7 +88,6 @@ export const myRadioGroup = RadioGroup.compose({ | Name | Privacy | Description | Parameters | Return | Inherited From | | ---------------------------- | --------- | ----------- | -------------------------------------------- | ------ | -------------- | -| `readOnlyChanged` | protected | | | `void` | | | `disabledChanged` | protected | | | `void` | | | `nameChanged` | protected | | | `void` | | | `valueChanged` | protected | | | `void` | | diff --git a/packages/web-components/fast-foundation/src/radio-group/radio-group.pw.spec.ts b/packages/web-components/fast-foundation/src/radio-group/radio-group.pw.spec.ts index 0756de72eb1..4a1cbe83ff4 100644 --- a/packages/web-components/fast-foundation/src/radio-group/radio-group.pw.spec.ts +++ b/packages/web-components/fast-foundation/src/radio-group/radio-group.pw.spec.ts @@ -317,32 +317,6 @@ test.describe("Radio Group", () => { } }); - test("should set all child radio elements to readonly when the `readonly` property is true", async () => { - await root.evaluate(node => { - node.innerHTML = /* html */ ` - - - - - - `; - }); - - await expect(element).toHaveBooleanAttribute("readonly"); - - expect( - await radios.evaluateAll(radios => - radios.every(radio => radio.hasAttribute("readonly")) - ) - ).toBeTruthy(); - - expect( - await radios.evaluateAll(radios => - radios.every(radio => radio.getAttribute("aria-readonly") === "true") - ) - ).toBeTruthy(); - }); - test("should set tabindex of 0 to a child radio with a matching `value`", async () => { await root.evaluate(node => { node.innerHTML = /* html */ ` diff --git a/packages/web-components/fast-foundation/src/radio-group/radio-group.ts b/packages/web-components/fast-foundation/src/radio-group/radio-group.ts index e662c91c169..12122fcf1e7 100644 --- a/packages/web-components/fast-foundation/src/radio-group/radio-group.ts +++ b/packages/web-components/fast-foundation/src/radio-group/radio-group.ts @@ -32,17 +32,6 @@ export class FASTRadioGroup extends FASTElement { */ @attr({ attribute: "readonly", mode: "boolean" }) public readOnly: boolean; - protected readOnlyChanged(): void { - if (this.slottedRadioButtons !== undefined) { - this.slottedRadioButtons.forEach((radio: FASTRadio) => { - if (this.readOnly) { - radio.readOnly = true; - } else { - radio.readOnly = false; - } - }); - } - } /** * Disables the radio group and child radios. @@ -173,10 +162,6 @@ export class FASTRadioGroup extends FASTElement { radio.setAttribute("name", this.name); } - if (this.readOnly) { - radio.readOnly = true; - } - if (this.value && this.value === radio.value) { this.selectedRadio = radio; this.focusedRadio = radio; @@ -239,16 +224,8 @@ export class FASTRadioGroup extends FASTElement { const radio: FASTRadio = group[index] as FASTRadio; if (!this.isInsideToolbar) { radio.setAttribute("tabindex", "0"); - if (radio.readOnly) { - this.slottedRadioButtons.forEach((nextRadio: FASTRadio) => { - if (nextRadio !== radio) { - nextRadio.setAttribute("tabindex", "-1"); - } - }); - } else { - radio.checked = true; - this.selectedRadio = radio; - } + radio.checked = true; + this.selectedRadio = radio; } this.focusedRadio = radio; radio.focus(); @@ -350,11 +327,7 @@ export class FASTRadioGroup extends FASTElement { }; private checkFocusedRadio = (): void => { - if ( - this.focusedRadio !== null && - !this.focusedRadio.readOnly && - !this.focusedRadio.checked - ) { + if (this.focusedRadio !== null && !this.focusedRadio.checked) { this.focusedRadio.checked = true; this.focusedRadio.setAttribute("tabindex", "0"); this.focusedRadio.focus(); diff --git a/packages/web-components/fast-foundation/src/radio/README.md b/packages/web-components/fast-foundation/src/radio/README.md index fc986127c3f..d89775e7a8e 100644 --- a/packages/web-components/fast-foundation/src/radio/README.md +++ b/packages/web-components/fast-foundation/src/radio/README.md @@ -113,18 +113,16 @@ export const myRadio = Radio.compose({ #### Fields -| Name | Privacy | Type | Default | Description | Inherited From | -| ---------- | ------- | --------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------- | -| `readOnly` | public | `boolean` | | When true, the control will be immutable by user interaction. See [readonly HTML attribute](https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly) for more information. | | -| `name` | public | `string` | | The name of the radio. See [name attribute](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#htmlattrdefname) for more info. | | -| `proxy` | | | | | FormAssociatedRadio | +| Name | Privacy | Type | Default | Description | Inherited From | +| ------- | ------- | -------- | ------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | ------------------- | +| `name` | public | `string` | | The name of the radio. See [name attribute](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#htmlattrdefname) for more info. | | +| `proxy` | | | | | FormAssociatedRadio | #### Methods -| Name | Privacy | Description | Parameters | Return | Inherited From | -| ----------------- | --------- | --------------------------------- | ------------------ | ----------------- | -------------- | -| `readOnlyChanged` | protected | | | `void` | | -| `keypressHandler` | public | Handles key presses on the radio. | `e: KeyboardEvent` | `boolean or void` | | +| Name | Privacy | Description | Parameters | Return | Inherited From | +| ----------------- | ------- | --------------------------------- | ------------------ | ----------------- | -------------- | +| `keypressHandler` | public | Handles key presses on the radio. | `e: KeyboardEvent` | `boolean or void` | | #### Events @@ -132,12 +130,6 @@ export const myRadio = Radio.compose({ | -------- | ---- | ---------------------------------------------------------- | -------------- | | `change` | | Emits a custom change event when the checked state changes | | -#### Attributes - -| Name | Field | Inherited From | -| ---------- | -------- | -------------- | -| `readonly` | readOnly | | - #### CSS Parts | Name | Description | diff --git a/packages/web-components/fast-foundation/src/radio/radio.pw.spec.ts b/packages/web-components/fast-foundation/src/radio/radio.pw.spec.ts index d0286e703c1..405e2f93bca 100644 --- a/packages/web-components/fast-foundation/src/radio/radio.pw.spec.ts +++ b/packages/web-components/fast-foundation/src/radio/radio.pw.spec.ts @@ -71,17 +71,6 @@ test.describe("Radio", () => { await element.evaluate((node: FASTRadio) => (node.disabled = false)); await expect(element).toHaveAttribute("aria-disabled", "false"); - - // Readonly - await expect(element).not.hasAttribute("aria-readonly"); - - await element.evaluate((node: FASTRadio) => (node.readOnly = true)); - - await expect(element).toHaveAttribute("aria-readonly", "true"); - - await element.evaluate((node: FASTRadio) => (node.readOnly = false)); - - await expect(element).toHaveAttribute("aria-readonly", "false"); }); test("should set a tabindex of 0 on the element", async () => { diff --git a/packages/web-components/fast-foundation/src/radio/radio.spec.md b/packages/web-components/fast-foundation/src/radio/radio.spec.md index ab0fe7e439b..2e37f6c8bf6 100644 --- a/packages/web-components/fast-foundation/src/radio/radio.spec.md +++ b/packages/web-components/fast-foundation/src/radio/radio.spec.md @@ -35,8 +35,6 @@ Extends [form associated custom element](../form-associated/form-associated-cust - The current checked state of the radio *Content attributes* -- `readonly` - - The radio should be submitted with the form but should not be editable. - `disabled` - The radio should be disabled from user interaction and will not be submitted with the form data. - `value` - Not visible to the user, it's used for form data and to distinguish between other radio buttons of the same name attribute value. @@ -72,7 +70,6 @@ Extends [form associated custom element](../form-associated/form-associated-cust - checked - disabled - required -- readonly *Slotted Content/Slotted Classes* *CSS Parts* @@ -89,9 +86,6 @@ The checked state can be toggled by: **disabled**: `true` or `false` When disabled, the value will not be changeable through user interaction. It should also not expose it's value to a form submission. -**readonly**: `true` or `false` -When readonly, the value will not be changeable through user interaction. The value will still be exposed to forms on submission. - ### Accessibility The root element inside the shadow-dom of the radio will be a focusable element with the following accessibility content attributes: [MDN Web docs](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_radio_role) diff --git a/packages/web-components/fast-foundation/src/radio/radio.template.ts b/packages/web-components/fast-foundation/src/radio/radio.template.ts index 91ef16c4d0e..0a8e522aeec 100644 --- a/packages/web-components/fast-foundation/src/radio/radio.template.ts +++ b/packages/web-components/fast-foundation/src/radio/radio.template.ts @@ -17,7 +17,6 @@ export function radioTemplate( aria-checked="${x => x.checked}" aria-required="${x => x.required}" aria-disabled="${x => x.disabled}" - aria-readonly="${x => x.readOnly}" @keypress="${(x, c) => x.keypressHandler(c.event as KeyboardEvent)}" >
diff --git a/packages/web-components/fast-foundation/src/radio/radio.ts b/packages/web-components/fast-foundation/src/radio/radio.ts index 2885b0ac4f9..eeee7f74b7a 100644 --- a/packages/web-components/fast-foundation/src/radio/radio.ts +++ b/packages/web-components/fast-foundation/src/radio/radio.ts @@ -1,6 +1,6 @@ -import { attr, observable } from "@microsoft/fast-element"; +import { observable } from "@microsoft/fast-element"; import { keySpace } from "@microsoft/fast-web-utilities"; -import { FASTRadioGroup } from "../radio-group/index.js"; +import type { FASTRadioGroup } from "../radio-group/index.js"; import type { StaticallyComposableHTML } from "../utilities/template-helpers.js"; import { FormAssociatedRadio } from "./radio.form-associated.js"; @@ -10,7 +10,7 @@ import { FormAssociatedRadio } from "./radio.form-associated.js"; */ export type RadioControl = Pick< HTMLInputElement, - "checked" | "disabled" | "readOnly" | "focus" | "setAttribute" | "getAttribute" + "checked" | "disabled" | "focus" | "setAttribute" | "getAttribute" >; /** @@ -34,20 +34,6 @@ export type RadioOptions = { * @public */ export class FASTRadio extends FormAssociatedRadio implements RadioControl { - /** - * When true, the control will be immutable by user interaction. See {@link https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly | readonly HTML attribute} for more information. - * @public - * @remarks - * HTML Attribute: readonly - */ - @attr({ attribute: "readonly", mode: "boolean" }) - public readOnly: boolean; // Map to proxy element - protected readOnlyChanged(): void { - if (this.proxy instanceof HTMLInputElement) { - this.proxy.readOnly = this.readOnly; - } - } - /** * The name of the radio. See {@link https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#htmlattrdefname | name attribute} for more info. */ @@ -68,6 +54,12 @@ export class FASTRadio extends FormAssociatedRadio implements RadioControl { @observable public defaultSlottedNodes: Node[]; + private get radioGroup() { + return (this as HTMLElement).closest( + "[role=radiogroup]" + ) as FASTRadioGroup | null; + } + /** * @internal */ @@ -118,10 +110,7 @@ export class FASTRadio extends FormAssociatedRadio implements RadioControl { } private isInsideRadioGroup(): boolean { - const parent: HTMLElement | null = (this as HTMLElement).closest( - "[role=radiogroup]" - ); - return parent !== null; + return this.radioGroup !== null; } /** @@ -131,7 +120,7 @@ export class FASTRadio extends FormAssociatedRadio implements RadioControl { public keypressHandler(e: KeyboardEvent): boolean | void { switch (e.key) { case keySpace: - if (!this.checked && !this.readOnly) { + if (!this.checked && !this.radioGroup?.readOnly) { this.checked = true; } return; diff --git a/packages/web-components/fast-foundation/src/radio/stories/radio.register.ts b/packages/web-components/fast-foundation/src/radio/stories/radio.register.ts index 4cc26fb5d8d..24cffbc4ab2 100644 --- a/packages/web-components/fast-foundation/src/radio/stories/radio.register.ts +++ b/packages/web-components/fast-foundation/src/radio/stories/radio.register.ts @@ -119,8 +119,6 @@ const styles = css` } :host([disabled]) .label, - :host([readonly]) .label, - :host([readonly]) .control, :host([disabled]) .control { cursor: not-allowed; } diff --git a/packages/web-components/fast-foundation/src/radio/stories/radio.stories.ts b/packages/web-components/fast-foundation/src/radio/stories/radio.stories.ts index a81aabe3d71..46165d70fe0 100644 --- a/packages/web-components/fast-foundation/src/radio/stories/radio.stories.ts +++ b/packages/web-components/fast-foundation/src/radio/stories/radio.stories.ts @@ -8,7 +8,6 @@ export const storyTemplate = html>` ?checked="${x => x.checked}" ?disabled="${x => x.disabled}" ?required="${x => x.required}" - ?readonly="${x => x.readOnly}" name="${x => x.name}" value="${x => x.value}" > @@ -23,7 +22,6 @@ export default { checked: false, disabled: false, required: false, - readOnly: false, storyContent: "Label", }, argTypes: {