From a0486b341581c610ebc9b3acd1837be2b66eb348 Mon Sep 17 00:00:00 2001
From: Josh Winn <965114+jawinn@users.noreply.github.com>
Date: Tue, 12 Nov 2024 10:50:26 -0500
Subject: [PATCH] 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
---
.changeset/afraid-dogs-worry.md | 5 +++
.changeset/gold-deers-smash.md | 5 +++
components/checkbox/index.css | 42 +++++--------------
components/checkbox/metadata/metadata.json | 7 +---
.../checkbox/stories/checkbox.stories.js | 28 +++++++++++--
components/checkbox/stories/checkbox.test.js | 22 +++++++++-
components/checkbox/stories/template.js | 24 ++++++-----
components/fieldgroup/index.css | 23 ----------
components/fieldgroup/metadata/metadata.json | 7 +---
components/fieldgroup/stories/fieldgroup.mdx | 6 +--
.../fieldgroup/stories/fieldgroup.stories.js | 26 +++++++-----
components/fieldgroup/stories/template.js | 8 +++-
12 files changed, 109 insertions(+), 94 deletions(-)
create mode 100644 .changeset/afraid-dogs-worry.md
create mode 100644 .changeset/gold-deers-smash.md
diff --git a/.changeset/afraid-dogs-worry.md b/.changeset/afraid-dogs-worry.md
new file mode 100644
index 0000000000..1b57d3f1f8
--- /dev/null
+++ 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
new file mode 100644
index 0000000000..a19ff44767
--- /dev/null
+++ 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
index 742bfcc43d..f2c87f2c4d 100644
--- 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
index c6f46aba37..4ce727d04d 100644
--- 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
index 2cebc9dc62..07ab09a792 100644
--- 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 `` 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
index 1266419975..4de9e4eed4 100644
--- 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
index 82845f6ee1..920b1f98b5 100644
--- 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
index e253a7a71c..6f7cc395f0 100644
--- 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
index 3d17618cf3..20528391a1 100644
--- 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
index 73e9ab2dfa..5d07c8d9c6 100644
--- 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
-#### Invalid Checkbox
+#### Invalid Checkboxes
diff --git a/components/fieldgroup/stories/fieldgroup.stories.js b/components/fieldgroup/stories/fieldgroup.stories.js
index d7236daaba..0284a5e73d 100644
--- 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 },
diff --git a/components/fieldgroup/stories/template.js b/components/fieldgroup/stories/template.js
index a0980be9d4..a657149eaa 100644
--- a/components/fieldgroup/stories/template.js
+++ b/components/fieldgroup/stories/template.js
@@ -1,6 +1,7 @@
import { Template as CheckBox } from "@spectrum-css/checkbox/stories/template.js";
import { Template as FieldLabel } from "@spectrum-css/fieldlabel/stories/template.js";
import { Template as HelpText } from "@spectrum-css/helptext/stories/template.js";
+import { getRandomId } from "@spectrum-css/preview/decorators";
import { Template as Radio } from "@spectrum-css/radio/stories/template.js";
import { html } from "lit";
import { classMap } from "lit/directives/class-map.js";
@@ -25,6 +26,8 @@ export const Template = (
} = {},
context = {},
) => {
+ const groupLabelId = getRandomId("group-label");
+
return html`