Merge pull request #816 from openSUSE/section-no-title

[web] Agama core/Section improvements

web/package.json

@@ -111,5 +111,8 @@
  "regenerator-runtime": "^0.14.0",
  "sprintf-js": "^1.1.2",
  "xbytes": "^1.8.0"
+ },
+ "overrides": {
+ "jsdom": "^22.1.0"
  }
 }

web/package/cockpit-agama.changes

@@ -1,3 +1,11 @@
+-------------------------------------------------------------------
+Tue Oct 31 09:41:33 UTC 2023 - David Diaz <dgonzalez@suse.com>
+
+- UI: sections improvements (gh#openSUSE/agama#816):
+ - allow using them without title.
+ - make them more accessible by using aria-label, aria-live-region,
+ and aria-busy attributes.
+
 -------------------------------------------------------------------
 Fri Oct 27 10:25:46 UTC 2023 - Imobach Gonzalez Sosa <igonzalezsosa@suse.com>
 

web/src/components/core/DBusError.jsx

@@ -21,6 +21,7 @@
 
 import React from "react";
 import { Button, EmptyState, EmptyStateIcon, EmptyStateBody, EmptyStateHeader } from "@patternfly/react-core";
+import { locationReload } from "~/utils";
 import { _ } from "~/i18n";
 
 import {
@@ -35,7 +36,7 @@ const ErrorIcon = () => <Icon name="error" className="icon-big" />;
 
 // TODO: an example
 const ReloadAction = () => (
- <Button size="lg" variant="primary" onClick={() => location.reload()}>
+ <Button size="lg" variant="primary" onClick={() => locationReload()}>
  {/* TRANSLATORS: button label */}
  {_("Reload")}
  </Button>

web/src/components/core/DBusError.test.jsx

@@ -23,6 +23,7 @@ import React from "react";
 
 import { screen } from "@testing-library/react";
 import { plainRender } from "~/test-utils";
+import * as utils from "~/utils";
 
 import { DBusError } from "~/components/core";
 
@@ -35,20 +36,13 @@ describe("DBusError", () => {
  });
 
  it("calls location.reload when user clicks on 'Reload'", async () => {
+ jest.spyOn(utils, "locationReload").mockImplementation(utils.noop);
+
  const { user } = plainRender(<DBusError />, { layout: true });
 
  const reloadButton = await screen.findByRole("button", { name: /Reload/i });
 
- // Mock location.reload
- // https://remarkablemark.org/blog/2021/04/14/jest-mock-window-location-href
- const { location } = window;
- delete window.location;
- window.location = { reload: jest.fn() };
-
  await user.click(reloadButton);
- expect(window.location.reload).toHaveBeenCalled();
-
- // restore windows.location
- window.location = location;
+ expect(utils.locationReload).toHaveBeenCalled();
  });
 });

web/src/components/core/IssuesPage.test.jsx

@@ -65,7 +65,7 @@ it("loads the issues", async () => {
 it("renders sections with issues", async () => {
  installerRender(withNotificationProvider(<IssuesPage />));
 
- const section = await screen.findByText(/Storage/);
+ const section = await screen.findByRole("region", { name: "Storage" });
  within(section).findByText(/Issue 1/);
  within(section).findByText(/Issue 2/);
 });

web/src/components/core/Section.jsx

@@ -23,7 +23,6 @@
 
 import React from "react";
 import { Link } from "react-router-dom";
-import { Text, TextVariants } from "@patternfly/react-core";
 import { Icon } from '~/components/layout';
 import { ValidationErrors } from "~/components/core";
 
@@ -46,13 +45,14 @@ const SectionIcon = ({ name, size = 32 }) => {
  * Internal component for rendering the section title
  *
  * @param {object} props
- * @param {string} props.text - the title for the section
- * @param {string} props.path - the path where the section links to. If present, props.openDialog is ignored
+ * @param {string} props.id - the id for the header.
+ * @param {string} props.text - the title for the section.
+ * @param {string} props.path - the path where the section links to. If present, props.openDialog is ignored.
  * @param {React.MouseEventHandler|undefined} [props.openDialog] - callback to be triggered when user clicks on the title, used for opening a dialog.
  *
  * @return {JSX.Element}
  */
-const SectionTitle = ({ text, path, openDialog }) => {
+const SectionTitle = ({ id, text, path, openDialog }) => {
  let title = <>{text}</>;
 
  if (path && path !== "") {
@@ -63,9 +63,9 @@ const SectionTitle = ({ text, path, openDialog }) => {
  }
 
  return (
- <Text component={TextVariants.h2}>
+ <h2 id={id}>
  {title}
- </Text>
+ </h2>
  );
 };
 
@@ -94,48 +94,81 @@ const SectionContent = ({ children }) => {
  * openDialog callback will be completely ignored.
  *
  * @example <caption>Simple usage</caption>
- * <Section title="Users" icon="manage_accounts">
+ * <Section title="Users" name="users" icon="manage_accounts">
+ * <UsersSummary />
+ * </Section>
+ *
+ * @example <caption>A section without title</caption>
+ * <Section aria-label="Users summary">
  * <UsersSummary />
  * </Section>
  *
  * @example <caption>A section that allows navigating to a page</caption>
- * <Section title="Users" icon="manage_accounts" path="/users">
+ * <Section title="Users" name="users" icon="manage_accounts" path="/users">
  * <UsersSummary />
  * </Section>
  *
  * @example <caption>A section that allows opening a settings dialog</caption>
  * <Section
  * title="L10n"
+ * name="localization"
  * icon="translate"
  * openDialog={() => setLanguageSettingsOpen(true)}
  * >
  * <L10nSummary />
  * <L10nSettings />
  * </Section>
  *
- * @param {object} props
- * @param {string} [props.icon] - the name of the icon section, if any
- * @param {string} props.title - The title for the section
- * @param {string} props.path - The path where the section links to. If present, props.openDialog is ignored
- * @param {React.MouseEventHandler|undefined} [props.openDialog] - callback to be triggered
+ * @typedef { Object } SectionProps
+ * @property {string} [icon] - Name of the section icon. Not rendered if title is not provided.
+ * @property {string} [title] - The section title. If not given, aria-label must be provided.
+ * @property {string} [name] - The section name. Used to build the header id.
+ * @property {string} [path] - Path where the section links to. If present, props.openDialog is ignored.
+ * @property {React.MouseEventHandler|undefined} [props.openDialog] - callback to be triggered
  * when user clicks on the title, used for opening a dialog.
- * @param {boolean} [props.loading] - whether the section is busy loading its content or not
- * @param {import("~/client/mixins").ValidationError[]} [props.errors] - Validation errors to be shown before the title
- * @param {React.ReactElement} props.children - the section content
+ * @property {boolean} [loading] - Whether the section is busy loading its content or not.
+ * @property {import("~/client/mixins").ValidationError[]} [props.errors] - Validation errors to be shown before the title.
+ * @property {React.ReactElement} [children] - The section content.
+ * @property {string} [aria-label] - aria-label attribute, required if title if not given
+ *
+ * @param { SectionProps } props
  */
 export default function Section({
  icon,
  title,
+ name,
  path,
  openDialog,
  loading,
  errors,
  children,
+ "aria-label": ariaLabel
 }) {
+ const headerId = `${name || crypto.randomUUID()}-section-header`;
+
+ if (!title && !ariaLabel) {
+ console.error("The Section component must have either, a 'title' or an 'aria-label'");
+ }
+
+ const SectionHeader = () => {
+ if (!title) return;
+
+ return (
+ <>
+ <SectionIcon name={loading ? "loading" : icon} />
+ <SectionTitle id={headerId} text={title} path={path} openDialog={openDialog} />
+ </>
+ );
+ };
+
  return (
- <section>
- <SectionIcon name={loading ? "loading" : icon} />
- <SectionTitle text={title} path={path} openDialog={openDialog} />
+ <section
+ aria-live="polite"
+ aria-busy={loading}
+ aria-label={ariaLabel || undefined}
+ aria-labelledby={ title && !ariaLabel ? headerId : undefined}
+ >
+ <SectionHeader />
  <SectionContent>
  {errors?.length > 0 &&
  <ValidationErrors errors={errors} title={`${title} errors`} />}

web/src/components/core/Section.test.jsx

@@ -25,10 +25,102 @@ import { plainRender, installerRender } from "~/test-utils";
 import { Section } from "~/components/core";
 
 describe("Section", () => {
- it("renders given title", () => {
- plainRender(<Section title="settings" />);
+ beforeAll(() => {
+ jest.spyOn(console, "error").mockImplementation();
+ });
+
+ afterAll(() => {
+ console.error.mockRestore();
+ });
+
+ describe("when title is given", () => {
+ it("renders the section header", () => {
+ plainRender(<Section title="Settings" />);
+ screen.getByRole("heading", { name: "Settings" });
+ });
+
+ it("renders an icon if valid icon name is given", () => {
+ const { container } = plainRender(<Section title="Settings" icon="settings" />);
+ const icon = container.querySelector("svg");
+ expect(icon).toHaveAttribute("data-icon-name", "settings");
+ });
+
+ it("does not render an icon if icon name not given", () => {
+ const { container } = plainRender(<Section title="Settings" />);
+ const icon = container.querySelector("svg");
+ expect(icon).toBeNull();
+ });
+
+ it("does not render an icon if not valid icon name is given", () => {
+ const { container } = plainRender(<Section title="Settings" icon="not-valid-icon-name" />);
+ const icon = container.querySelector("svg");
+ expect(icon).toBeNull();
+ });
+ });
+
+ describe("when title is not given", () => {
+ it("does not render the section header", async () => {
+ plainRender(<Section />);
+ const header = await screen.queryByRole("heading");
+ expect(header).not.toBeInTheDocument();
+ });
+
+ it("does not render the section icon", () => {
+ const { container } = plainRender(<Section icon="settings" />);
+ const icon = container.querySelector("svg");
+ expect(icon).toBeNull();
+ });
+
+ it("does not render the loading icon", () => {
+ const { container } = plainRender(<Section loading />);
+ const icon = container.querySelector("svg");
+ expect(icon).toBeNull();
+ });
+ });
+
+ describe("when aria-label is given", () => {
+ it("sets aria-label attribute", () => {
+ plainRender(<Section title="Settings" aria-label="User settings" />);
+ const section = screen.getByRole("region", { name: "User settings" });
+ expect(section).toHaveAttribute("aria-label", "User settings");
+ });
 
- screen.getByRole("heading", { name: "settings" });
+ it("does not set aria-labelledby", () => {
+ plainRender(<Section title="Settings" aria-label="User settings" />);
+ const section = screen.getByRole("region", { name: "User settings" });
+ expect(section).not.toHaveAttribute("aria-labelledby");
+ });
+ });
+
+ describe("when aria-label is not given", () => {
+ it("sets aria-labelledby if title is provided", () => {
+ plainRender(<Section title="Settings" />);
+ const section = screen.getByRole("region", { name: "Settings" });
+ expect(section).toHaveAttribute("aria-labelledby");
+ });
+
+ it("does not set aria-label", () => {
+ plainRender(<Section title="Settings" />);
+ const section = screen.getByRole("region", { name: "Settings" });
+ expect(section).not.toHaveAttribute("aria-label");
+ });
+ });
+
+ it("sets predictable header id if name is given", () => {
+ plainRender(<Section title="Settings" name="settings" />);
+ screen.getByRole("heading", { name: "Settings", id: "settings-header-section" });
+ });
+
+ it("sets partially random header id if name is not given", () => {
+ plainRender(<Section title="Settings" name="settings" />);
+ screen.getByRole("heading", { name: "Settings", id: /.*(-header-section)$/ });
+ });
+
+ it("renders as a polite live region", () => {
+ plainRender(<Section title="Settings" />);
+
+ const section = screen.getByRole("region", { name: "Settings" });
+ expect(section).toHaveAttribute("aria-live", "polite");
  });
 
  it("renders given errors", () => {
@@ -49,25 +141,31 @@ describe("Section", () => {
  screen.getByText("A settings summary");
  });
 
- it("renders an icon when set as loading", () => {
- // TODO: add a mechanism to check that it's the expected icon. data-something attribute?
- const { container } = plainRender(<Section title="Settings" loading />);
- container.querySelector("svg");
- });
+ it("does not set aria-busy", () => {
+ plainRender(<Section title="Settings" />);
 
- it("renders an icon when a valid icon name is given", () => {
- // TODO: add a mechanism to check that it's the expected icon. data-something attribute?
- const { container } = plainRender(<Section title="Settings" icon="settings" />);
- container.querySelector("svg");
+ screen.getByRole("region", { name: "Settings", busy: false });
  });
 
- it("does not render an icon when either, not loading or not icon name was given", () => {
- // TODO: add a mechanism to check that it's the expected icon. data-something attribute?
- const { container } = plainRender(<Section title="Settings" />);
- const icon = container.querySelector("svg");
- expect(icon).toBeNull();
- });
+ describe("when set as loading", () => {
+ it("sets aria-busy", () => {
+ plainRender(<Section title="Settings" loading />);
+
+ screen.getByRole("region", { busy: true });
+ });
+
+ it("renders the loading icon if title was given", () => {
+ const { container } = plainRender(<Section title="Settings" loading />);
+ const icon = container.querySelector("svg");
+ expect(icon).toHaveAttribute("data-icon-name", "loading");
+ });
 
+ it("does not render the loading icon if title was not given", () => {
+ const { container } = plainRender(<Section loading />);
+ const icon = container.querySelector("svg");
+ expect(icon).toBeNull();
+ });
+ });
  describe("when path is given", () => {
  it("renders a link for navigating to it", async () => {
  installerRender(<Section title="Settings" path="/settings" />);

web/src/components/layout/Icon.jsx

@@ -136,8 +136,6 @@ const icons = {
  * - https://stackoverflow.com/a/61472427
  * - https://ryanhutzley.medium.com/dynamic-svg-imports-in-create-react-app-d6d411f6d6c6
  *
- * @todo: find how to render the "icon not found" warning only in _development_ mode
- *
  * @example
  * <Icon name="warning" size="16" />
  *
@@ -149,10 +147,19 @@ const icons = {
  *
  */
 export default function Icon({ name, className = "", size = 32, ...otherProps }) {
+ if (!icons[name]) {
+ console.error(sprintf(_("Icon %s not found!"), name));
+ return null;
+ }
+
  const IconComponent = icons[name];
- className = `${className} icon-size-${size}`.trim();
 
- return (IconComponent)
- ? <IconComponent className={className} aria-hidden="true" {...otherProps} />
- : <em>{sprintf(_("Icon %s not found!"), name)}</em>;
+ return (
+ <IconComponent
+ aria-hidden="true"
+ data-icon-name={name}
+ className={`${className} icon-size-${size}`.trim()}
+ {...otherProps}
+ />
+ );
 }