fix: correct the display of checkbox read-only state (#3328)

* fix(checkbox): adjust read-only styles and add read-only example This removes some unnecessary read-only styles. Read-only just needs to override disabled styles; otherwise it uses default styles (for both default and emphasized). Also adjusts template to include aria-disabled when in the read-only state, so screen readers such as VoiceOver do not announce the input as normal. Note that aria-readonly is not used as it is not announced property and MDN states that it is "not relevant" for input with type checkbox. * fix(fieldgroup): remove incorrect read-only checkbox styles The previous display of the read-only state, without checkboxes and displaying commas did not match up with any guidelines. This update is to make the display of read-only match with what is implemented in React Spectrum. It also includes aria-labeledby in the template so the group is announced by screen readers. * docs(checkbox): add additional tests for combination of some states * docs(fieldgroup): display a checked option in readonly fieldgroup
adobe · Nov 12, 2024 · a0486b3 · a0486b3
1 parent 0f50c14
commit a0486b3
Show file tree

Hide file tree

Showing 12 changed files with 109 additions and 94 deletions.
diff --git a/.changeset/afraid-dogs-worry.md b/.changeset/afraid-dogs-worry.md
@@ -0,0 +1,5 @@
+---
+"@spectrum-css/checkbox": patch
+---
+
+This removes some unnecessary read-only styles. Read-only just needs to override disabled styles. Otherwise it uses the normal styles (for both default and emphasized).
diff --git a/.changeset/gold-deers-smash.md b/.changeset/gold-deers-smash.md
@@ -0,0 +1,5 @@
+---
+"@spectrum-css/fieldgroup": minor
+---
+
+The previous display of the read-only state checkboxes did not match up with any guidelines. This update removes the read-only specific styles for checkbox within the fieldgroup component, so that the boxes are still displayed and commas are not appended to the label. This includes the removal of `--spectrum-fieldgroup-readonly-delimiter` as it is no longer needed.
diff --git a/components/checkbox/index.css b/components/checkbox/index.css
@@ -13,13 +13,11 @@
 
 @import "./themes/express.css";
 
-/* checkbox/index.css
- *
+/*
  * .spectrum-Checkbox-box::before is the Checkbox "box"
  * .spectrum-Checkbox-box::after is the focus indicator
  */
 
-/* Component tokens by t-shirt size */
 .spectrum-Checkbox {
 	/* Color */
 	--spectrum-checkbox-content-color-default: var(--spectrum-neutral-content-color-default);
@@ -176,41 +174,23 @@
 		}
 	}
 
-	/*
-   * Read-Only
-   *
-   * readonly is not a valid attribute for input[type="checkbox"]
-   * so we borrow the immutability of a disabled checkbox
-   * while using the colors of a default checkbox
-   */
+	/* Read-only */
 	&.is-readOnly {
-		border-color: var(--highcontrast-checkbox-color-default, var(--mod-checkbox-control-selected-color-default, var(--spectrum-checkbox-control-selected-color-default)));
-
-		&:hover {
-			.spectrum-Checkbox-box::before {
-				border-color: var(--highcontrast-checkbox-color-default, var(--mod-checkbox-control-selected-color-default, var(--spectrum-checkbox-control-selected-color-default)));
-			}
+		.spectrum-Checkbox-input {
+			cursor: default;
 		}
 
-		&:active {
-			.spectrum-Checkbox-box::before {
-				border-color: var(--highcontrast-checkbox-selected-color-default, var(--mod-checkbox-control-selected-color-default, var(--spectrum-checkbox-control-selected-color-default)));
-			}
-		}
-	}
-
-	&.is-readOnly .spectrum-Checkbox-input,
-	&.is-readOnly .spectrum-Checkbox-input:checked {
-		&:disabled + .spectrum-Checkbox-box {
-			&::before {
+		/* Overrides disabled styles */
+		.spectrum-Checkbox-input,
+		.spectrum-Checkbox-input:checked {
+			&:disabled + .spectrum-Checkbox-box::before {
 				border-color: var(--highcontrast-checkbox-color-default, var(--mod-checkbox-control-selected-color-default, var(--spectrum-checkbox-control-selected-color-default)));
 				background-color: var(--highcontrast-checkbox-background-color-default, var(--mod-checkbox-checkmark-color, var(--spectrum-checkbox-checkmark-color)));
 			}
-		}
 
-		&:disabled ~ .spectrum-Checkbox-label {
-			forced-color-adjust: none;
-			color: var(--highcontrast-checkbox-color-default, var(--mod-checkbox-content-color-default, var(--spectrum-checkbox-content-color-default)));
+			&:disabled ~ .spectrum-Checkbox-label {
+				color: var(--highcontrast-checkbox-color-default, var(--mod-checkbox-content-color-default, var(--spectrum-checkbox-content-color-default)));
+			}
 		}
 	}
 

diff --git a/components/checkbox/metadata/metadata.json b/components/checkbox/metadata/metadata.json
@@ -67,13 +67,11 @@
     ".spectrum-Checkbox.is-invalid.is-indeterminate:hover .spectrum-Checkbox-label",
     ".spectrum-Checkbox.is-invalid:hover .spectrum-Checkbox-box:before",
     ".spectrum-Checkbox.is-invalid:hover .spectrum-Checkbox-input:checked + .spectrum-Checkbox-box",
-    ".spectrum-Checkbox.is-readOnly",
+    ".spectrum-Checkbox.is-readOnly .spectrum-Checkbox-input",
     ".spectrum-Checkbox.is-readOnly .spectrum-Checkbox-input:checked:disabled + .spectrum-Checkbox-box:before",
     ".spectrum-Checkbox.is-readOnly .spectrum-Checkbox-input:checked:disabled ~ .spectrum-Checkbox-label",
     ".spectrum-Checkbox.is-readOnly .spectrum-Checkbox-input:disabled + .spectrum-Checkbox-box:before",
     ".spectrum-Checkbox.is-readOnly .spectrum-Checkbox-input:disabled ~ .spectrum-Checkbox-label",
-    ".spectrum-Checkbox.is-readOnly:active .spectrum-Checkbox-box:before",
-    ".spectrum-Checkbox.is-readOnly:hover .spectrum-Checkbox-box:before",
     ".spectrum-Checkbox:active .spectrum-Checkbox-box:before",
     ".spectrum-Checkbox:active .spectrum-Checkbox-input:checked + .spectrum-Checkbox-box:before",
     ".spectrum-Checkbox:active .spectrum-Checkbox-label",
@@ -234,7 +232,6 @@
     "--highcontrast-checkbox-highlight-color-default",
     "--highcontrast-checkbox-highlight-color-down",
     "--highcontrast-checkbox-highlight-color-focus",
-    "--highcontrast-checkbox-highlight-color-hover",
-    "--highcontrast-checkbox-selected-color-default"
+    "--highcontrast-checkbox-highlight-color-hover"
   ]
 }
diff --git a/components/checkbox/stories/checkbox.stories.js b/components/checkbox/stories/checkbox.stories.js
@@ -4,16 +4,16 @@ import { isChecked, isDisabled, isEmphasized, isIndeterminate, isInvalid, isRead
 import metadata from "../metadata/metadata.json";
 import packageJson from "../package.json";
 import { CheckboxGroup } from "./checkbox.test.js";
-import { AllVariantsCheckboxGroup, DocsCheckboxGroup, } from "./template.js";
+import { AllVariantsCheckboxGroup, DocsCheckboxGroup, Template } from "./template.js";
 
 /**
  * Checkboxes allow users to select multiple items from a list of individual items, or mark one individual item as selected.
  *
  * ## Usage notes
  *
- * Checkboxes should not be used on their own. They should always be used within a [field group](/docs/components-field-group--docs). Invalid checkboxes are indicated with an alert [help text](/docs/components-help-text--docs) when included in a Field group.
- *
- * When the label is too long for the horizontal space available, it wraps to form another line.
+ * - Checkboxes should not be used on their own. They should always be used within a [field group](/docs/components-field-group--docs).
+ * - Invalid checkboxes are indicated with an alert [help text](/docs/components-help-text--docs) when included in a field group.
+ * - For more information about the read-only state, see the [read-only checkboxes](/docs/components-field-group--docs#read-only-checkboxes) section of the field group documentation.
  */
 export default {
 	title: "Checkbox",
@@ -62,6 +62,11 @@ Default.args = {
 };
 Default.tags = ["!autodocs"];
 
+/**
+ * Checkboxes can be selected, not selected, or in an indeterminate state. They are in an indeterminate state (shown with a dash icon) when they represent both selected and not selected values.
+ *
+ * When the label is too long for the horizontal space available, it wraps to form another line.
+ */
 export const DefaultVariants = AllVariantsCheckboxGroup.bind({});
 DefaultVariants.tags = ["!dev"];
 DefaultVariants.parameters = {
@@ -92,6 +97,21 @@ Sizing.parameters = {
 	chromatic: { disableSnapshot: true },
 };
 
+/**
+ * The class `is-readOnly` is applied to checkboxes in the read-only state. It's worth noting that `<input type="checkbox">` HTML elements do not have a native `readonly` attribute, and the intended behavior is up to the implementation:
+ * - Read-only checkboxes are immutable, i.e. their checked state cannot be changed.
+ * - Unlike disabled checkboxes, the checkbox should remain focusable.
+ */
+export const ReadOnly = Template.bind({});
+ReadOnly.storyName = "Read-only";
+ReadOnly.args = {
+	isReadOnly: true,
+};
+ReadOnly.tags = ["!dev"];
+ReadOnly.parameters = {
+	chromatic: { disableSnapshot: true }
+};
+
 // ********* VRT ONLY ********* //
 export const WithForcedColors = CheckboxGroup.bind({});
 WithForcedColors.args = Default.args;

diff --git a/components/checkbox/stories/checkbox.test.js b/components/checkbox/stories/checkbox.test.js
@@ -27,6 +27,10 @@ export const CheckboxGroup = Variants({
 			testHeading: "Checked",
 			isChecked: true,
 		},
+		{
+			testHeading: "Indeterminate",
+			isIndeterminate: true,
+		},
 		{
 			testHeading: "Invalid",
 			isInvalid: true,
@@ -36,6 +40,11 @@ export const CheckboxGroup = Variants({
 			isInvalid: true,
 			isChecked: true,
 		},
+		{
+			testHeading: "Invalid, indeterminate",
+			isInvalid: true,
+			isIndeterminate: true,
+		},
 		{
 			testHeading: "Disabled",
 			isDisabled: true,
@@ -46,7 +55,8 @@ export const CheckboxGroup = Variants({
 			isChecked: true,
 		},
 		{
-			testHeading: "Indeterminate",
+			testHeading: "Disabled, indeterminate",
+			isDisabled: true,
 			isIndeterminate: true,
 		},
 		{
@@ -58,5 +68,15 @@ export const CheckboxGroup = Variants({
 			isReadOnly: true,
 			isChecked: true,
 		},
+		{
+			testHeading: "Read-only, indeterminate",
+			isReadOnly: true,
+			isIndeterminate: true,
+		},
+		{
+			testHeading: "Read-only, checked, disabled",
+			isReadOnly: true,
+			isChecked: true,
+		},
 	]
 });
diff --git a/components/checkbox/stories/template.js b/components/checkbox/stories/template.js
@@ -50,7 +50,7 @@ export const Template = ({
 					typeof size !== "undefined",
 				[`${rootClass}--emphasized`]: isEmphasized,
 				["is-indeterminate"]: isIndeterminate,
-				["is-disabled"]: isDisabled|| isReadOnly,
+				["is-disabled"]: isDisabled,
 				["is-invalid"]: isInvalid,
 				["is-readOnly"]: isReadOnly,
 				...customClasses.reduce((a, c) => ({ ...a, [c]: true }), {}),
@@ -62,13 +62,19 @@ export const Template = ({
 				type="checkbox"
 				class="${rootClass}-input"
 				aria-labelledby=${ifDefined(ariaLabelledby)}
+				aria-disabled=${ifDefined(isReadOnly ? "true" : undefined)}
 				?checked=${isChecked}
-				?disabled=${isDisabled || isReadOnly}
+				?disabled=${isDisabled}
 				title=${ifDefined(title)}
 				value=${ifDefined(value)}
-				@change=${function() {
-					if (isDisabled) return;
-					updateArgs({ isChecked: !isChecked });
+				@change=${(e) => {
+					if (isReadOnly) {
+						// Make checked value immutable for read-only.
+						e.preventDefault();
+						e.target.checked = !e.target.checked;
+					}
+					if (isDisabled || isReadOnly) return;
+					updateArgs?.({ isChecked: e.target.checked });
 				}}
 				id=${ifDefined(id ? `${id}-input` : undefined)}
 			/>
@@ -103,21 +109,19 @@ export const DocsCheckboxGroup = (args, context) => Container({
 			...args,
 			context,
 			iconName: undefined,
+			label: "Unchecked",
 		})}
 		${Template({
 			...args,
 			context,
 			isChecked: true,
+			label: "Checked",
 		})}
 		${Template({
 			...args,
 			context,
 			isIndeterminate: true,
-		})}
-		${Template({
-			...args,
-			context,
-			isDisabled: true,
+			label: "Indeterminate",
 		})}
 		${Template({
 			...args,

diff --git a/components/fieldgroup/index.css b/components/fieldgroup/index.css
@@ -11,17 +11,8 @@
  * governing permissions and limitations under the License.
  */
 
-/* fieldgroup/index.css
- *
- * fieldgroup contains four component dependences:
- * radio, checkbox, helptext, fieldlabel
- *
- */
-
-/* custom properties */
 .spectrum-FieldGroup {
 	--spectrum-fieldgroup-margin: var(--spectrum-spacing-300);
-	--spectrum-fieldgroup-readonly-delimiter: "\002c";
 }
 
 /* field group */
@@ -65,17 +56,3 @@
 		}
 	}
 }
-
-/* read-only checkbox group */
-.spectrum-FieldGroup {
-	.spectrum-Checkbox.is-readOnly {
-		.spectrum-Checkbox-box {
-			display: none;
-		}
-
-		/* read-only checkbox fields delimited by commas */
-		&:not(:last-child) .spectrum-Checkbox-label::after {
-			content: var(--spectrum-fieldgroup-readonly-delimiter);
-		}
-	}
-}
diff --git a/components/fieldgroup/metadata/metadata.json b/components/fieldgroup/metadata/metadata.json
@@ -2,8 +2,6 @@
   "sourceFile": "index.css",
   "selectors": [
     ".spectrum-FieldGroup",
-    ".spectrum-FieldGroup .spectrum-Checkbox.is-readOnly .spectrum-Checkbox-box",
-    ".spectrum-FieldGroup .spectrum-Checkbox.is-readOnly:not(:last-child) .spectrum-Checkbox-label:after",
     ".spectrum-FieldGroup--horizontal .spectrum-FieldGroupInputLayout",
     ".spectrum-FieldGroup--horizontal .spectrum-FieldGroupInputLayout .spectrum-FieldGroup-item:not(:last-child)",
     ".spectrum-FieldGroup--horizontal .spectrum-FieldGroupInputLayout .spectrum-HelpText",
@@ -13,10 +11,7 @@
     ".spectrum-FieldGroupInputLayout"
   ],
   "modifiers": [],
-  "component": [
-    "--spectrum-fieldgroup-margin",
-    "--spectrum-fieldgroup-readonly-delimiter"
-  ],
+  "component": ["--spectrum-fieldgroup-margin"],
   "global": ["--spectrum-spacing-300"],
   "system-theme": [],
   "passthroughs": [],

diff --git a/components/fieldgroup/stories/fieldgroup.mdx b/components/fieldgroup/stories/fieldgroup.mdx
@@ -50,13 +50,13 @@ A horizontal group of fields:
 
 ### Invalid
 
-An invalid group of fields:
+An invalid group of radio buttons or checkboxes is signified by negative help text.
 
-#### Invalid Radio
+#### Invalid Radios
 
 <Canvas of={FieldGroupStories.InvalidRadio} />
 
-#### Invalid Checkbox
+#### Invalid Checkboxes
 
 <Canvas of={FieldGroupStories.InvalidCheckbox} />
 

diff --git a/components/fieldgroup/stories/fieldgroup.stories.js b/components/fieldgroup/stories/fieldgroup.stories.js
@@ -7,14 +7,18 @@ import { FieldGroupSet } from "./fieldgroup.test.js";
 import { Template } from "./template.js";
 
 /**
- * A field group is a group of fields, usually radios (also known as a radio group) or checkboxes
+ * A field group is a group of fields which are usually radios (also known as a radio group) or checkboxes
  * (also known as a checkbox group). A field group is composed of a field label, a group of radio
- * inputs or checkboxes, and an optional help text component. The field label within the field group
- * can be used to mark a field group as optional or required. The field group items other than the
- * label must be wrapped in a nested `div` with `.spectrum-FieldGroupInputLayout` to control their
- * layout separately from the label. Help text may or may not appear below a field group and is
- * necessary when denoting invalid checkbox fields, invalid radio button fields, and required
- * fields. Invalid radio buttons and checkboxes are signified by negative help text.
+ * inputs or checkboxes, and an optional help text component.
+ *
+ * ## Usage notes
+ *
+ * - **Markup:** The field group items other than the label must be wrapped in a nested `div` with the `spectrum-FieldGroupInputLayout`
+ * class to control their layout separately from the label. The class `spectrum-FieldGroup-item` should also be applied to each checkbox or radio.
+ * - **Roles:** For radio groups, the attribute `role="radiogroup"` should be used. For a checkbox group, use `role="group"`.
+ * - **Field label:** The field label within the field group can be used to mark a field group as [optional or required](#required-or-optional).
+ * - **Help text:** Help text may or may not appear below a field group and is necessary when denoting invalid
+ * checkbox fields, invalid radio button fields, and required fields.
  */
 export default {
 	title: "Field group",
@@ -182,8 +186,7 @@ InvalidCheckbox.parameters = {
 
 /**
  * Field groups can be marked as optional or required, depending on the situation.
- *
-* If required, the field group must either contain a "(required)" label or an asterisk. If an asterisk is used, help text must explain what the asterisk means.
+ * If required, the field group must either contain a "(required)" label or an asterisk. If an asterisk is used, help text must explain what the asterisk means.
  */
 export const Required = Template.bind({});
 Required.tags = ["!dev"];
@@ -268,13 +271,16 @@ HorizontalSideLabelCheckbox.parameters = {
 };
 
 /**
- * A group of read-only checkboxes that have been checked. In U.S. English, use commas to delineate items within read-only checkbox groups. In other languages, use the locale-specific formatting.
+ * Implementations should include the following behavior for read-only checkboxes:
+ * - Read-only checkboxes are immutable, i.e. their checked state cannot be changed.
+ * - Unlike disabled checkbox groups, the normally focusable elements of a checkbox group should remain focusable.
  */
 export const ReadOnly = Template.bind({});
 ReadOnly.tags = ["!dev"];
 ReadOnly.args = {
 	isReadOnly: true,
 	inputType: "checkbox",
+	helpText: undefined,
 };
 ReadOnly.parameters = {
 	chromatic: { disableSnapshot: true },