Read-only support for Checkbox, Radio, Switch [OKTA-731946] #2305

benlister-okta · 2024-07-26T23:26:15Z

OKTA-731946

Summary

Adds boolean for read-only support in Checkbox, Radio, Switch, CheckboxGroup, RadioGroup
Includes aria-readonly attributes added for screen readers, unlike disabled this retains keyboard navigability via TAB, and arrow keys where applicable

Testing & Screenshots

I have confirmed this change with my designer and the Odyssey Design Team.

Test site

bryancunningham-okta · 2024-07-29T13:21:08Z

packages/odyssey-react-mui/src/Checkbox.tsx

+  /**
+   * Determines whether the Checkbox is read-only
+   */
+  isReadOnly?: boolean;


This prop already lives in FieldComponentProps. We should just Pick it from there probably

bryancunningham-okta · 2024-07-29T13:21:32Z

packages/odyssey-react-mui/src/CheckboxGroup.tsx

+  /**
+   * If `true`, the CheckboxGroup is read-only
+   */
+  isReadOnly?: boolean;


Same comment about Pick from FieldComponentProps

bryancunningham-okta · 2024-07-29T13:22:04Z

packages/odyssey-react-mui/src/Radio.tsx

  onBlur?: MuiFormControlLabelProps["onBlur"];
+  isReadOnly?: boolean;


Same comment about Pick from FieldComponentProps

bryancunningham-okta · 2024-07-29T13:27:21Z

packages/odyssey-react-mui/src/RadioGroup.tsx

@@ -46,6 +53,7 @@ export type RadioGroupProps = {
   * The `value` on the selected Radio
   */
  value?: RadioProps["value"];
+  isReadOnly?: boolean;


Same comment about the Pick

bryancunningham-okta · 2024-07-29T13:30:10Z

packages/odyssey-react-mui/src/RadioGroup.tsx

  label,
  name: nameOverride,
  onChange: onChangeProp,
  testId,
  translate,
  value,
 }: RadioGroupProps) => {
+  const [selectedValue, setSelectedValue] = useState(value ?? defaultValue);


Might be missing something but not sure why we need to track the value internally for this component

I refactored this a bit in the commit I just pushed, but the purpose of this is for the uncontrolled case since its not managed by the MUI component itself. Without it, the read-only logic won't work because it will just update when a new selection is made.

bryancunningham-okta · 2024-07-29T13:31:37Z

packages/odyssey-react-mui/src/RadioGroup.tsx

+    [onChangeProp, isReadOnly, isDisabled],
+  );
+
+  const handleMouseDown = useCallback(


I think this needs to be handled in an onClick or onChange. My suspicion is that a user can still update this value via keyboard in the readonly state

Good call, mouseDown triggers before onClick so I was using that after having problems with onChange allowing the value to be changed in readonly, but it looks like onClick works, and is more conventional, plus supports keyboard and mouse.

bryancunningham-okta · 2024-07-29T13:33:58Z

packages/odyssey-react-mui/src/labs/Switch.tsx

-    : isChecked
-      ? odysseyDesignTokens.PaletteSuccessLight
-      : odysseyDesignTokens.HueNeutral300,
+    : isReadOnly


I think we should probably remove these styles from the ternary. It's getting a bit messy. Go with the adding them conditionally based on the prop.

bryancunningham-okta · 2024-07-29T13:35:45Z

packages/odyssey-react-mui/src/labs/Switch.tsx

@@ -127,7 +141,9 @@ const SwitchThumb = styled("span", {
    borderRadius: odysseyDesignTokens.BorderRadiusRound,
    backgroundColor: isDisabled
      ? odysseyDesignTokens.HueNeutral50
-      : odysseyDesignTokens.HueNeutralWhite,
+      : isReadOnly


I guess if we are moving away from the ternary approach in the styles, we might as well update these as well

bryancunningham-okta · 2024-07-29T13:36:09Z

packages/odyssey-react-mui/src/labs/Switch.tsx

-      ? odysseyDesignTokens.HueNeutral50
-      : odysseyDesignTokens.HueNeutralWhite,
+    fill:
+      isDisabled || isReadOnly


Same comment about the ternary

bryancunningham-okta · 2024-07-29T13:37:22Z

packages/odyssey-react-mui/src/labs/Switch.tsx

@@ -292,12 +318,13 @@ const Switch = ({

  const handleOnChange = useCallback<ChangeEventHandler<HTMLInputElement>>(
    (event) => {
+      if (isReadOnly) return;


Suggested change

if (isReadOnly) return;

if (isReadOnly) {

event.preventDefault()

return

};

bryancunningham-okta · 2024-07-29T13:38:44Z

packages/odyssey-react-mui/src/labs/Switch.tsx

@@ -341,21 +369,27 @@ const Switch = ({
            odysseyDesignTokens={odysseyDesignTokens}
            type="checkbox"
            value={value}
+            tabIndex={isReadOnly ? 0 : undefined}


0 is the default value. Feels like this isn't really doing anything. But, maybe I'm missing something

Good catch, this was an artifact from something I was exploring, I just verified this is not needed.

bryancunningham-okta · 2024-07-29T13:40:24Z

packages/odyssey-react-mui/src/theme/components.tsx

+                backgroundColor: odysseyTokens.HueNeutral100,
+                borderColor: odysseyTokens.HueNeutral300,
+
+                //Override hoever styles


Have a typo here :)

bryancunningham-okta · 2024-08-02T20:57:36Z

packages/odyssey-react-mui/src/RadioGroup.tsx

+            isChecked:
+              (isControlled ? inputValues.value : internalValue) ===
+              child.props.value,
+            onReadOnlyClick: handleReadOnlyClick,


@benlister-okta This isn't a valid prop is it? My assumption is this doesn't do anything and probably passes this prop into the DOM.

It isn't a valid prop in RadioGroup, but the component uses it to pass isChecked to the children Radio components in the group to determine which one to check.

@benlister-okta I pulled your branch down, still confused as to what this does. I removed it entirely and everything seems to work fine. Also, before removing, I put a log in the handleReadOnlyClick function and it never gets called

Per our Slack convo: We do not need to pass down the onReadOnlyClick prop. It wasn't firing at all and the onClick handler in Radio already prevents default behavior in the isReadOnly case.

Also, from what I can tell, we do not need to pass isChecked either

bryancunningham-okta · 2024-08-02T20:59:32Z

packages/odyssey-react-mui/src/RadioGroup.tsx

-    (event, value) => {
-      onChangeProp?.(event, value);
+    (event, newValue) => {
+      if (!isReadOnly) {


I don't think we need to check for read only in the onChange handler. If we are handling the click correctly, the onChange won't ever fire right?

This is needed because it prevents changing the selection if isReadOnly = true. It works that way because of the keyboard support for focusing, otherwise tabbing through the options of a Read-only radio would change the selection.

I tested the behavior without and it allows selection change when Read-only.

Per our Slack convo: Don't need the check for isReadOnly here as the change event is not fired due to the preventDefault in the Radio click handler

bryancunningham-okta · 2024-08-02T21:01:22Z

packages/odyssey-react-mui/src/Checkbox.tsx

@@ -131,9 +133,24 @@ const Checkbox = ({

  const onChange = useCallback<NonNullable<MuiCheckboxProps["onChange"]>>(
    (event, checked) => {
+      if (isReadOnly) {


Same comment here, I think this being handled in the onClick is enough. If we prevent the change from happening in the first place, don't think it matters if this is readonly or not

Good call, radio functions and has a different keyboard workflow, so I think this can be removed (still functions as expected when I tested).

bryancunningham-okta · 2024-08-02T21:04:50Z

packages/odyssey-react-mui/src/Radio.tsx

+    [onChangeProp, isReadOnly],
+  );
+
+  const handleMouseDown = useCallback<React.MouseEventHandler<HTMLSpanElement>>(


This should probably be a click handler, rather than mousedown. Can still click with the keyboard

bryancunningham-okta · 2024-08-02T21:12:14Z

packages/odyssey-react-mui/src/labs/Switch.tsx

+    width: odysseyDesignTokens.Spacing4,
+    transform: "translateY(-50%)",
+    transition: `opacity ${odysseyDesignTokens.TransitionDurationMain}`,
+    opacity: getOpacity(),


If we are going to make these functions, we should let them take the args they need and move them out of the styled component declaration probably.

const getOpacity = (isChecked) => isChecked ? 1 : 0;

Or, we conditionally add the styles like

opacity: 0, ...(isChecked && { opacity: 1, })

good call, it might be an over optimization to use the functions, I'll switch it to conditionals

bryancunningham-okta · 2024-08-02T21:14:53Z

packages/odyssey-react-mui/src/labs/Switch.tsx

-  transition: `background-color ${odysseyDesignTokens.TransitionDurationMain}`,
-}));
+>(({ isChecked, isDisabled, isReadOnly, odysseyDesignTokens }) => {
+  const getBackgroundColor = () => {


Same comment as below about moving these functions outside of the styled component declaration. Every time this renders it's going to recreate these functions. Also, we'll need to name this a little more specifically since we'll be seeing it out of context. Like getSwitchTrackBackgroundColor or something like that

bryancunningham-okta · 2024-08-02T21:15:42Z

packages/odyssey-react-mui/src/labs/Switch.tsx

-}));
+>(({ isChecked, isDisabled, isReadOnly, odysseyDesignTokens }) => {
+  const getBackgroundColor = () => {
+    if (isDisabled) return odysseyDesignTokens.HueNeutral200;


Can we do the traditional style if statement with brackets

if (isDisabled) { return odysseyDesignTokens.HueNeutral200; } Same for the other instances of this pattern

switched this to CSS conditionals for simplicity

KevinGhadyani-Okta · 2024-08-05T18:50:46Z

packages/odyssey-react-mui/src/Checkbox.tsx

+      sx={{
+        alignItems: "flex-start",
+        ...(isReadOnly && {
+          cursor: "default", //area between the label and checkbox
+          "& .MuiTypography-root": {
+            cursor: "default", //for the label
+          },
+        }),
+      }}


Can we memoize these styles? They may already be memoized somewhat in Field's render function. I'm not sure where we're at in the code from the PR on GitHub though.

I think so, I'll update it to something like this:

const checkboxStyles = useMemo( () => ({ alignItems: "flex-start", ...(isReadOnly && { cursor: "default", "& .MuiTypography-root": { cursor: "default", }, }), }), [isReadOnly], ); return ( <FormControlLabel sx={checkboxStyles} ...rest...

KevinGhadyani-Okta · 2024-08-05T18:50:59Z

packages/odyssey-react-mui/src/Checkbox.tsx

+          onClick={
+            onClick as unknown as React.MouseEventHandler<HTMLButtonElement>
+          }


Weird. Any way around this syntax like setting our useCallback to MuiCheckboxProps["onClick"]?

Thanks for catching this. I think this was part of something I was debugging that didn't get removed. I updated the onClick to match the existing onChange:

const onClick = useCallback<NonNullable<MuiCheckboxProps["onClick"]>>( (event) => { if (isReadOnly) { event.preventDefault(); } }, [isReadOnly], );

KevinGhadyani-Okta · 2024-08-05T18:52:14Z

packages/odyssey-react-mui/src/Checkbox.tsx

          inputProps={{
            "data-se": testId,
+            "aria-readonly": isReadOnly,
+            readOnly: isReadOnly,
          }}


Can we memoize these inputProps? I can't see the whole file, so it might not make sense, but it looks like it could be memoized. If it's in Field's function, then it's already memoized. My bad.

We could memoize them, but since aria-readonly is being used on the native element via inputProps, and isReadOnly is already in memory, since it's used elsewhere, I wonder if there is much of a benefit of it. What do you think @KevinGhadyani-Okta?

KevinGhadyani-Okta · 2024-08-05T18:55:23Z

packages/odyssey-react-mui/src/Radio.tsx

+    (event, checked) => {
+      if (isReadOnly) {
+        event.preventDefault();
+        return;
+      }
+      onChangeProp?.(event, checked);
+    },


Instead of early-fail, I wonder if we can do if-else instead:

Suggested change

(event, checked) => {

if (isReadOnly) {

event.preventDefault();

return;

}

onChangeProp?.(event, checked);

},

(event, checked) => {

if (isReadOnly) {

event.preventDefault();

}

else {

onChangeProp?.(event, checked);

}

},

That might be more clear. The return in that if statement can disappear if you're not looking for it.

I've used a lot of early-fail, but I've avoided in more recent years to make it clear we're doing one or the other, and return is always at the bottom.

KevinGhadyani-Okta · 2024-08-05T18:56:42Z

packages/odyssey-react-mui/src/Radio.tsx

+            "aria-disabled": isReadOnly,
+            readOnly: isReadOnly,
+            tabIndex: isReadOnly ? 0 : undefined,


Do we want this change?

Suggested change

"aria-disabled": isReadOnly,

readOnly: isReadOnly,

tabIndex: isReadOnly ? 0 : undefined,

"aria-disabled": isDisabled || isReadOnly,

readOnly: isReadOnly,

tabIndex: isReadOnly ? 0 : undefined,

good point. Makes sense to include on the disabled state as well. I found that it is required for read-only but I don't see a reason to not include it for disabled as well.

KevinGhadyani-Okta · 2024-08-05T18:57:18Z

packages/odyssey-react-mui/src/Radio.tsx

+      sx={{
+        ...(isReadOnly && {
+          cursor: "default",
+          "& .MuiTypography-root": {
+            cursor: "default",
+          },
+        }),
+      }}


Usually, these styles are in components.tsx so they apply to all inputs. Can we move them in there?

This style only applies to the label, not the form elements. I wanted this to live in components.tsx but found I was not able to target from the MuiCheckbox/MuiRadio style blocks, since its applied outside of the component. I believe we'd have to re-factor how the MuiFormControlLabel works to accept the isReadOnly prop so I figure this is the easier and cleaner path since it only applies to checkboxes and radios.

…TA-731946

bryancunningham-okta · 2024-08-16T19:35:59Z

packages/odyssey-react-mui/src/RadioGroup.tsx

  label,
  name: nameOverride,
  onChange: onChangeProp,
  testId,
  translate,
  value,
 }: RadioGroupProps) => {
+  const [internalValue, setInternalValue] = useState(value ?? defaultValue);


Per Slack convo: With these other change being made, we no longer need any internal value as far as I can tell

bryancunningham-okta · 2024-08-16T19:38:02Z

packages/odyssey-react-mui/src/Radio.tsx

-    [onChangeProp],
+    (event, checked) => {
+      if (isReadOnly) {
+        event.preventDefault();


Can add event.stopPropagation(); here if we want. May not be necessary but also won't hurt anything

benlister-okta requested a review from a team as a code owner July 26, 2024 23:26

bryancunningham-okta reviewed Jul 29, 2024

View reviewed changes

benlister-okta added 13 commits August 1, 2024 13:24

fix: initial logic

30e97ef

fix: add styles and update logic for checkbox

c306ca3

fix: add readOnly to checkbox and radio groups

d8d70d9

fix: add readOnly to switch

7e2da86

fix: remove unneeded comments

30ee68b

fix: style updates, comments

d68131f

fix: tabindex, comments

c30533c

fix: updates from pr review

c38b805

fix: swap on mousedown for onclick

9e7a575

fix: update switch

dacf3f7

fix: switch updates from pr feedback

2cf8f3c

fix: replace read-only with disabled aria in Radio

5fc601c

fix: add comments

f66a53a

benlister-okta force-pushed the bl_read_only_forms_OKTA-731946 branch from 4e4c306 to f66a53a Compare August 1, 2024 20:42

benlister-okta requested a review from bryancunningham-okta August 1, 2024 21:15

bryancunningham-okta reviewed Aug 2, 2024

View reviewed changes

fix: updates based on PR feedback

7014df6

KevinGhadyani-Okta reviewed Aug 5, 2024

View reviewed changes

benlister-okta added 4 commits August 5, 2024 14:45

fix: adding pr review feedback

f8e01fe

fix: add read-only support to Select

5691113

fix: pass readonly into select chips object

9e5c9ad

Merge remote-tracking branch 'origin/main' into bl_read_only_forms_OK…

9c2b32b

…TA-731946

bryancunningham-okta reviewed Aug 16, 2024

View reviewed changes

benlister-okta and others added 3 commits August 16, 2024 14:50

fix: simplify logic, remove redundancies

37efc4f

feat(odyssey-react-mui): remove internal state from RadioGroup

2485d7f

fix: remove commented playwright test

8a99902

bryancunningham-okta previously approved these changes Aug 21, 2024

View reviewed changes

fix: update integration tests to match content

760edcf

benlister-okta dismissed bryancunningham-okta’s stale review via 760edcf August 21, 2024 17:42

bryancunningham-okta approved these changes Aug 21, 2024

View reviewed changes

oktapp-aperture-okta bot merged commit f92a201 into main Aug 21, 2024
3 checks passed

oktapp-aperture-okta bot deleted the bl_read_only_forms_OKTA-731946 branch August 21, 2024 18:44

		onBlur?: MuiFormControlLabelProps["onBlur"];
		isReadOnly?: boolean;

Read-only support for Checkbox, Radio, Switch [OKTA-731946] #2305

Read-only support for Checkbox, Radio, Switch [OKTA-731946] #2305

Conversation

benlister-okta commented Jul 26, 2024 • edited Loading

Summary

Testing & Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryancunningham-okta Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryancunningham-okta Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinGhadyani-Okta Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

benlister-okta Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

KevinGhadyani-Okta Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinGhadyani-Okta Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlister-okta commented Jul 26, 2024 •

edited

Loading

bryancunningham-okta Aug 2, 2024 •

edited

Loading

bryancunningham-okta Aug 2, 2024 •

edited

Loading

KevinGhadyani-Okta Aug 5, 2024 •

edited

Loading

benlister-okta Aug 5, 2024 •

edited

Loading

KevinGhadyani-Okta Aug 5, 2024 •

edited

Loading

KevinGhadyani-Okta Aug 5, 2024 •

edited

Loading