Fix console errors for Radio and SegmentedControl (#16950)

frontend/src/metabase/admin/permissions/components/PermissionsTabs.jsx

@@ -13,7 +13,7 @@ const PermissionsTabs = ({ tab, onChangeTab }) => (
         { name: t`Data permissions`, value: `databases` },
         { name: t`Collection permissions`, value: `collections` },
       ]}
-      onChange={onChangeTab}
+      onOptionClick={onChangeTab}
       variant="underlined"
       py={2}
     />

frontend/src/metabase/components/Radio.info.js

-import React from "react";
+import React, { useState } from "react";
 import Radio from "metabase/components/Radio";
 
 export const component = Radio;
@@ -8,15 +8,24 @@ export const description = `
 A standard radio button group.
 `;
 
-const PROPS = {
-  value: 0,
-  options: [{ name: "Gadget", value: 0 }, { name: "Gizmo", value: 1 }],
-};
+const OPTIONS = [{ name: "Gadget", value: 0 }, { name: "Gizmo", value: 1 }];
+
+function RadioDemo(props) {
+  const [value, setValue] = useState(0);
+  return (
+    <Radio
+      {...props}
+      options={OPTIONS}
+      value={value}
+      onChange={nextValue => setValue(nextValue)}
+    />
+  );
+}
 
 export const examples = {
-  default: <Radio {...PROPS} />,
-  underlined: <Radio {...PROPS} variant="underlined" />,
-  "show buttons": <Radio {...PROPS} showButtons />,
-  vertical: <Radio {...PROPS} vertical />,
-  bubble: <Radio {...PROPS} variant="bubble" />,
+  default: <RadioDemo />,
+  underlined: <RadioDemo variant="underlined" />,
+  "show buttons": <RadioDemo showButtons />,
+  vertical: <RadioDemo vertical />,
+  bubble: <RadioDemo variant="bubble" />,
 };

frontend/src/metabase/components/Radio.jsx

@@ -4,6 +4,8 @@ import _ from "underscore";
 
 import Icon from "metabase/components/Icon";
 import {
+  RadioInput,
+  RadioButton,
   BubbleList,
   BubbleItem,
   NormalList,
@@ -25,8 +27,12 @@ const optionShape = PropTypes.shape({
 const propTypes = {
   name: PropTypes.string,
   value: PropTypes.any,
-  options: PropTypes.arrayOf(optionShape).isRequired,
+  options: PropTypes.oneOfType([
+    PropTypes.arrayOf(PropTypes.string),
+    PropTypes.arrayOf(optionShape),
+  ]).isRequired,
   onChange: PropTypes.func,
+  onOptionClick: PropTypes.func,
   optionNameFn: PropTypes.func,
   optionValueFn: PropTypes.func,
   optionKeyFn: PropTypes.func,
@@ -51,9 +57,15 @@ const VARIANTS = {
 
 function Radio({
   name: nameFromProps,
-  value,
+  value: currentValue,
   options,
+
+  // onChange won't fire when you click an already checked item
+  // onOptionClick will fire in any case
+  // onOptionClick can be used for e.g. tab navigation like on the admin Permissions page)
+  onOptionClick,
   onChange,
+
   optionNameFn = defaultNameGetter,
   optionValueFn = defaultValueGetter,
   optionKeyFn = defaultValueGetter,
@@ -70,7 +82,7 @@ function Radio({
 
   const [List, Item] = VARIANTS[variant] || VARIANTS.normal;
 
-  if (variant === "underlined" && value === undefined) {
+  if (variant === "underlined" && currentValue === undefined) {
     console.warn(
       "Radio can't underline selected option when no value is given.",
     );
@@ -79,35 +91,48 @@ function Radio({
   return (
     <List {...props} vertical={vertical} showButtons={showButtons}>
       {options.map((option, index) => {
-        const selected = value === optionValueFn(option);
+        const value = optionValueFn(option);
+        const selected = currentValue === value;
         const last = index === options.length - 1;
+        const key = optionKeyFn(option);
+        const id = `${name}-${key}`;
+        const labelId = `${id}-label`;
         return (
-          <Item
-            key={optionKeyFn(option)}
-            selected={selected}
-            last={last}
-            vertical={vertical}
-            showButtons={showButtons}
-            py={py}
-            xspace={xspace}
-            yspace={yspace}
-            onClick={e => onChange(optionValueFn(option))}
-            aria-selected={selected}
-          >
-            {option.icon && <Icon name={option.icon} mr={1} />}
-            <input
-              className="Form-radio"
-              type="radio"
-              name={name}
-              value={optionValueFn(option)}
-              checked={selected}
-              id={name + "-" + optionKeyFn(option)}
-            />
-            {showButtons && (
-              <label htmlFor={name + "-" + optionKeyFn(option)} />
-            )}
-            <span>{optionNameFn(option)}</span>
-          </Item>
+          <li key={key}>
+            <Item
+              id={labelId}
+              htmlFor={id}
+              selected={selected}
+              last={last}
+              vertical={vertical}
+              showButtons={showButtons}
+              py={py}
+              xspace={xspace}
+              yspace={yspace}
+              onClick={() => {
+                if (typeof onOptionClick === "function") {
+                  onOptionClick(value);
+                }
+              }}
+            >
+              {option.icon && <Icon name={option.icon} mr={1} />}
+              <RadioInput
+                id={id}
+                name={name}
+                value={value}
+                checked={selected}
+                onChange={() => {
+                  if (typeof onChange === "function") {
+                    onChange(value);
+                  }
+                }}
+                // Workaround for https://github.com/testing-library/dom-testing-library/issues/877
+                aria-labelledby={labelId}
+              />
+              {showButtons && <RadioButton checked={selected} />}
+              {optionNameFn(option)}
+            </Item>
+          </li>
         );
       })}
     </List>

frontend/src/metabase/components/Radio.styled.js

@@ -4,17 +4,46 @@ import { space } from "styled-system";
 
 import { color, lighten } from "metabase/lib/colors";
 
+export const RadioInput = styled.input.attrs({ type: "radio" })`
+  cursor: inherit;
+  position: absolute;
+  opacity: 0;
+  width: 0;
+  height: 0;
+  top: 0;
+  left: 0;
+  margin: 0;
+  padding: 0;
+  z-index: 1;
+`;
+
+export const RadioButton = styled.div`
+  cursor: pointer;
+  display: inline-block;
+  flex: 0 0 auto;
+  position: relative;
+  margin-right: 0.5rem;
+  width: 12px;
+  height: 12px;
+  border: 2px solid white;
+  box-shadow: 0 0 0 2px ${color("shadow")};
+  border-radius: 12px;
+  background-color: ${props =>
+    props.checked ? color("brand") : "transparent"};
+`;
+
 // BASE
 const BaseList = styled.ul`
   display: flex;
   flex-direction: ${props => (props.vertical ? "column" : "row")};
 `;
 
-const BaseItem = styled.li.attrs({
+const BaseItem = styled.label.attrs({
   mr: props => (!props.vertical && !props.last ? props.xspace : null),
   mb: props => (props.vertical && !props.last ? props.yspace : null),
 })`
   ${space}
+  position: relative;
   display: flex;
   align-items: center;
   cursor: pointer;

frontend/src/metabase/components/SegmentedControl.jsx

@@ -2,7 +2,11 @@ import React, { useMemo } from "react";
 import PropTypes from "prop-types";
 import _ from "underscore";
 import Icon from "metabase/components/Icon";
-import { SegmentedList, SegmentedItem } from "./SegmentedControl.styled";
+import {
+  SegmentedList,
+  SegmentedItem,
+  SegmentedControlRadio,
+} from "./SegmentedControl.styled";
 
 const optionShape = PropTypes.shape({
   name: PropTypes.node.isRequired,
@@ -37,26 +41,30 @@ export function SegmentedControl({
         const isSelected = option.value === value;
         const isFirst = index === 0;
         const isLast = index === options.length - 1;
+        const id = `${name}-${option.value}`;
+        const labelId = `${name}-${option.value}`;
         return (
-          <SegmentedItem
-            key={option.value}
-            isSelected={isSelected}
-            isFirst={isFirst}
-            isLast={isLast}
-            onClick={e => onChange(option.value)}
-            selectedColor={option.selectedColor || "brand"}
-          >
-            {option.icon && <Icon name={option.icon} mr={1} />}
-            <input
-              id={`${name}-${option.value}`}
-              className="Form-radio"
-              type="radio"
-              name={name}
-              value={option.value}
-              checked={isSelected}
-            />
-            <span>{option.name}</span>
-          </SegmentedItem>
+          <li key={option.value}>
+            <SegmentedItem
+              id={labelId}
+              isSelected={isSelected}
+              isFirst={isFirst}
+              isLast={isLast}
+              selectedColor={option.selectedColor || "brand"}
+            >
+              {option.icon && <Icon name={option.icon} mr={1} />}
+              <SegmentedControlRadio
+                id={id}
+                name={name}
+                value={option.value}
+                checked={isSelected}
+                onChange={() => onChange(option.value)}
+                // Workaround for https://github.com/testing-library/dom-testing-library/issues/877
+                aria-labelledby={labelId}
+              />
+              {option.name}
+            </SegmentedItem>
+          </li>
         );
       })}
     </SegmentedList>

frontend/src/metabase/components/SegmentedControl.styled.js

@@ -7,7 +7,8 @@ export const SegmentedList = styled.ul`
   display: flex;
 `;
 
-export const SegmentedItem = styled.li`
+export const SegmentedItem = styled.label`
+  position: relative;
   display: flex;
   align-items: center;
   font-weight: bold;
@@ -26,3 +27,16 @@ export const SegmentedItem = styled.li`
     color: ${props => (!props.isSelected ? color(props.selectedColor) : null)};
   }
 `;
+
+export const SegmentedControlRadio = styled.input.attrs({ type: "radio" })`
+  cursor: inherit;
+  position: absolute;
+  opacity: 0;
+  width: 0;
+  height: 0;
+  top: 0;
+  left: 0;
+  margin: 0;
+  padding: 0;
+  z-index: 1;
+`;

frontend/src/metabase/css/components/form.css

@@ -104,28 +104,6 @@
   transition: border 300ms ease-in-out;
 }
 
-.Form-radio {
-  display: none;
-}
-
-.Form-radio + label {
-  cursor: pointer;
-  display: inline-block;
-  flex: 0 0 auto;
-  position: relative;
-  margin-right: var(--margin-1);
-  width: 8px;
-  height: 8px;
-  border: 2px solid white;
-  box-shadow: 0 0 0 2px var(--color-shadow);
-  border-radius: 8px;
-}
-
-.Form-radio:checked + label {
-  box-shadow: 0 0 0 2px var(--color-shadow);
-  background-color: var(--color-brand);
-}
-
 .Form-field .AdminSelect {
   border-color: var(--form-field-border-color);
 }

frontend/src/metabase/visualizations/components/ChartSettings.jsx

@@ -247,6 +247,7 @@ class ChartSettings extends Component {
         options={sectionNames}
         optionNameFn={v => v}
         optionValueFn={v => v}
+        optionKeyFn={v => v}
         variant="bubble"
       />
     );

frontend/test/metabase/visualizations/components/ChartSettings.unit.spec.js

@@ -20,10 +20,6 @@ function widget(widget = {}) {
   };
 }
 
-function expectTabSelected(node, value) {
-  expect(node.parentNode.getAttribute("aria-selected")).toBe(String(value));
-}
-
 describe("ChartSettings", () => {
   it("should not crash if there are no widgets", () => {
     render(<ChartSettings {...DEFAULT_PROPS} widgets={[]} />);
@@ -38,17 +34,17 @@ describe("ChartSettings", () => {
     );
   });
   it("should default to the first section (if no section in DEFAULT_TAB_PRIORITY)", () => {
-    const { getByText } = render(
+    const { getByLabelText } = render(
       <ChartSettings
         {...DEFAULT_PROPS}
         widgets={[widget({ section: "Foo" }), widget({ section: "Bar" })]}
       />,
     );
-    expectTabSelected(getByText("Foo"), true);
-    expectTabSelected(getByText("Bar"), false);
+    expect(getByLabelText("Foo")).toBeChecked();
+    expect(getByLabelText("Bar")).not.toBeChecked();
   });
   it("should default to the DEFAULT_TAB_PRIORITY", () => {
-    const { getByText } = render(
+    const { getByLabelText } = render(
       <ChartSettings
         {...DEFAULT_PROPS}
         widgets={[
@@ -57,21 +53,21 @@ describe("ChartSettings", () => {
         ]}
       />,
     );
-    expectTabSelected(getByText("Foo"), false);
-    expectTabSelected(getByText("Display"), true);
+    expect(getByLabelText("Foo")).not.toBeChecked();
+    expect(getByLabelText("Display")).toBeChecked();
   });
   it("should be able to switch sections", () => {
-    const { getByText } = render(
+    const { getByText, getByLabelText } = render(
       <ChartSettings
         {...DEFAULT_PROPS}
         widgets={[widget({ section: "Foo" }), widget({ section: "Bar" })]}
       />,
     );
-    expectTabSelected(getByText("Foo"), true);
-    expectTabSelected(getByText("Bar"), false);
+    expect(getByLabelText("Foo")).toBeChecked();
+    expect(getByLabelText("Bar")).not.toBeChecked();
     fireEvent.click(getByText("Bar"));
-    expectTabSelected(getByText("Foo"), false);
-    expectTabSelected(getByText("Bar"), true);
+    expect(getByLabelText("Foo")).not.toBeChecked();
+    expect(getByLabelText("Bar")).toBeChecked();
   });
 
   it("should show widget names", () => {