web: laying the groundwork for future expansion (#7045)

* web: laying the groundwork for future expansion

This commit is a hodge-podge of updates and changes to the web.  Functional changes:

- Makefile: Fixed a bug in the `help` section that prevented the WIDTH from being accurately
  calculated if `help` was included rather than in-lined.

- ESLint: Modified the "unused vars" rule so that variables starting with an underline are not
  considered by the rule.  This allows for elided variables in event handlers.  It's not a perfect
  solution-- a better one would be to use Typescript's function-specialization typing, but there are
  too many places where we elide or ignore some variables in a function's usage that switching over
  to specialization would be a huge lift.

- locale: It turns out, lit-locale does its own context management.  We don't need to have a context
  at all in this space, and that's one less listener we need to attach t othe DOM.

- ModalButton: A small thing, but using `nothing` instead of "html``" allows lit better control over
  rendering and reduces the number of actual renders of the page.

- FormGroup: Provided a means to modify the aria-label, rather than stick with the just the word
  "Details."  Specializing this field will both help users of screen readers in the future, and will
  allow test suites to find specific form groups now.

- RadioButton: provide a more consistent interface to the RadioButton.  First, we dispatch the
  events to the outside world, and we set the value locally so that the current `Form.ts` continues
  to behave as expected.  We also prevent the "button lost value" event from propagating; this
  presents a unified select-like interface to users of the RadioButtonGroup.  The current value
  semantics are preserved; other clients of the RadioButton do not see a change in behavior.

- EventEmitter: If the custom event detail is *not* an object, do not use the object-like semantics
  for forwarding it; just send it as-is.

- Comments: In the course of laying the groundwork for the application wizard, I throw a LOT of
  comments into the code, describing APIs, interfaces, class and function signatures, to better
  document the behavior inside and as signposts for future work.

* web: permit arrays to be sent in custom events without interpolation.

* actually use assignValue or rather serializeFieldRecursive

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

---------

Signed-off-by: Jens Langhammer <jens@goauthentik.io>
Co-authored-by: Jens Langhammer <jens@goauthentik.io>

Makefile

@@ -28,10 +28,13 @@ CODESPELL_ARGS = -D - -D .github/codespell-dictionary.txt \
 
 all: lint-fix lint test gen web  ## Lint, build, and test everything
 
+HELP_WIDTH := $(shell grep -h '^[a-z][^ ]*:.*\#\#' $(MAKEFILE_LIST) 2>/dev/null | \
+	cut -d':' -f1 | awk '{printf "%d\n", length}' | sort -rn | head -1)
+
 help:  ## Show this help
 	@echo "\nSpecify a command. The choices are:\n"
-	@grep -E '^[0-9a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | \
+	@grep -Eh '^[0-9a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | \
-		awk 'BEGIN {FS = ":.*?## "}; {printf "  \033[0;36m%-24s\033[m %s\n", $$1, $$2}' | \
+		awk 'BEGIN {FS = ":.*?## "}; {printf "  \033[0;36m%-$(HELP_WIDTH)s  \033[m %s\n", $$1, $$2}' | \
 		sort
 	@echo ""
 

web/.eslintrc.json

@@ -16,11 +16,21 @@
         "sourceType": "module"
     },
     "plugins": ["@typescript-eslint", "lit", "custom-elements"],
+    "ignorePatterns": ["authentik-live-tests/**"],
     "rules": {
         "indent": "off",
         "linebreak-style": ["error", "unix"],
         "quotes": ["error", "double", { "avoidEscape": true }],
         "semi": ["error", "always"],
-        "@typescript-eslint/ban-ts-comment": "off"
+        "@typescript-eslint/ban-ts-comment": "off",
+        "no-unused-vars": "off",
+        "@typescript-eslint/no-unused-vars": [
+            "error",
+            {
+                "argsIgnorePattern": "^_",
+                "varsIgnorePattern": "^_",
+                "caughtErrorsIgnorePattern": "^_"
+            }
+        ]
     }
 }

web/src/elements/ak-locale-context/ak-locale-context.ts

@@ -2,13 +2,11 @@ import { EVENT_LOCALE_CHANGE } from "@goauthentik/common/constants";
 import { EVENT_LOCALE_REQUEST } from "@goauthentik/common/constants";
 import { customEvent, isCustomEvent } from "@goauthentik/elements/utils/customEvents";
 
-import { provide } from "@lit-labs/context";
 import { LitElement, html } from "lit";
 import { customElement, property } from "lit/decorators.js";
 
 import { initializeLocalization } from "./configureLocale";
 import type { LocaleGetter, LocaleSetter } from "./configureLocale";
-import locale from "./context";
 import {
     DEFAULT_LOCALE,
     autoDetectLanguage,
@@ -32,7 +30,6 @@ import {
 @customElement("ak-locale-context")
 export class LocaleContext extends LitElement {
     /// @attribute The text representation of the current locale */
-    @provide({ context: locale })
     @property({ attribute: true, type: String })
     locale = DEFAULT_LOCALE;
 

web/src/elements/buttons/ModalButton.ts

@@ -1,7 +1,7 @@
 import { AKElement } from "@goauthentik/elements/Base";
 import { PFSize } from "@goauthentik/elements/Spinner";
 
-import { CSSResult, TemplateResult, css, html } from "lit";
+import { CSSResult, TemplateResult, css, html, nothing } from "lit";
 import { customElement, property } from "lit/decorators.js";
 
 import PFBackdrop from "@patternfly/patternfly/components/Backdrop/backdrop.css";
@@ -100,7 +100,7 @@ export class ModalButton extends AKElement {
         });
     }
 
-    renderModalInner(): TemplateResult {
+    renderModalInner(): TemplateResult | typeof nothing {
         return html`<slot name="modal"></slot>`;
     }
 
@@ -136,6 +136,6 @@ export class ModalButton extends AKElement {
 
     render(): TemplateResult {
         return html` <slot name="trigger" @click=${() => this.onClick()}></slot>
-            ${this.open ? this.renderModal() : ""}`;
+            ${this.open ? this.renderModal() : nothing}`;
     }
 }

web/src/elements/forms/Form.ts

@@ -31,6 +31,37 @@ export interface KeyUnknown {
     [key: string]: unknown;
 }
 
+/**
+ * Form
+ *
+ * The base form element for interacting with user inputs.
+ *
+ * All forms either[1] inherit from this class and implement the `renderInlineForm()` method to
+ * produce the actual form, or include the form in-line as a slotted element. Bizarrely, this form
+ * will not render at all if it's not actually in the viewport?[2]
+ *
+ * @element ak-form
+ *
+ * @slot - Where the form goes if `renderInlineForm()` returns undefined.
+ * @fires eventname - description
+ *
+ * @csspart partname - description
+ */
+
+/* TODO:
+ *
+ * 1. Specialization: Separate this component into three different classes:
+ *    - The base class
+ *    - The "use `renderInlineForm` class
+ *    - The slotted class.
+ * 2. There is already specialization-by-type throughout all of our code.
+ *    Consider refactoring serializeForm() so that the conversions are on
+ *    the input types, rather than here. (i.e. "Polymorphism is better than
+ *    switch.")
+ *
+ *
+ */
+
 @customElement("ak-form")
 export abstract class Form<T> extends AKElement {
     abstract send(data: T): Promise<unknown>;
@@ -61,6 +92,10 @@ export abstract class Form<T> extends AKElement {
         ];
     }
 
+    /**
+     * Called by the render function. Blocks rendering the form if the form is not within the
+     * viewport.
+     */
     get isInViewport(): boolean {
         const rect = this.getBoundingClientRect();
         return !(rect.x + rect.y + rect.width + rect.height === 0);
@@ -70,6 +105,11 @@ export abstract class Form<T> extends AKElement {
         return this.successMessage;
     }
 
+    /**
+     * After rendering the form, if there is both a `name` and `slug` element within the form,
+     * events the `name` element so that the slug will always have a slugified version of the
+     * `name.`. This duplicates functionality within ak-form-element-horizontal.
+     */
     updated(): void {
         this.shadowRoot
             ?.querySelectorAll("ak-form-element-horizontal[name=name]")
@@ -103,6 +143,12 @@ export abstract class Form<T> extends AKElement {
         form?.reset();
     }
 
+    /**
+     * Return the form elements that may contain filenames. Not sure why this is quite so
+     * convoluted. There is exactly one case where this is used:
+     * `./flow/stages/prompt/PromptStage: 147: case PromptTypeEnum.File.`
+     * Consider moving this functionality to there.
+     */
     getFormFiles(): { [key: string]: File } {
         const files: { [key: string]: File } = {};
         const elements =
@@ -126,6 +172,10 @@ export abstract class Form<T> extends AKElement {
         return files;
     }
 
+    /**
+     * Convert the elements of the form to JSON.[4]
+     *
+     */
     serializeForm(): T | undefined {
         const elements =
             this.shadowRoot?.querySelectorAll<HorizontalFormElement>(
@@ -147,17 +197,21 @@ export abstract class Form<T> extends AKElement {
                 "multiple" in inputElement.attributes
             ) {
                 const selectElement = inputElement as unknown as HTMLSelectElement;
-                json[element.name] = Array.from(selectElement.selectedOptions).map((v) => v.value);
+                this.assignValue(
+                    inputElement,
+                    Array.from(selectElement.selectedOptions).map((v) => v.value),
+                    json,
+                );
             } else if (
                 inputElement.tagName.toLowerCase() === "input" &&
                 inputElement.type === "date"
             ) {
-                json[element.name] = inputElement.valueAsDate;
+                this.assignValue(inputElement, inputElement.valueAsDate, json);
             } else if (
                 inputElement.tagName.toLowerCase() === "input" &&
                 inputElement.type === "datetime-local"
             ) {
-                json[element.name] = new Date(inputElement.valueAsNumber);
+                this.assignValue(inputElement, new Date(inputElement.valueAsNumber), json);
             } else if (
                 inputElement.tagName.toLowerCase() === "input" &&
                 "type" in inputElement.dataset &&
@@ -165,19 +219,19 @@ export abstract class Form<T> extends AKElement {
             ) {
                 // Workaround for Firefox <93, since 92 and older don't support
                 // datetime-local fields
-                json[element.name] = new Date(inputElement.value);
+                this.assignValue(inputElement, new Date(inputElement.value), json);
             } else if (
                 inputElement.tagName.toLowerCase() === "input" &&
                 inputElement.type === "checkbox"
             ) {
-                json[element.name] = inputElement.checked;
+                this.assignValue(inputElement, inputElement.checked, json);
             } else if ("selectedFlow" in inputElement) {
-                json[element.name] = inputElement.value;
+                this.assignValue(inputElement, inputElement.value, json);
             } else if (inputElement.tagName.toLowerCase() === "ak-search-select") {
                 const select = inputElement as unknown as SearchSelect<unknown>;
                 try {
                     const value = select.toForm();
-                    json[element.name] = value;
+                    this.assignValue(inputElement, value, json);
                 } catch (exc) {
                     if (exc instanceof PreventFormSubmit) {
                         throw new PreventFormSubmit(exc.message, element);
@@ -185,13 +239,16 @@ export abstract class Form<T> extends AKElement {
                     throw exc;
                 }
             } else {
-                this.serializeFieldRecursive(inputElement, inputElement.value, json);
+                this.assignValue(inputElement, inputElement.value, json);
             }
         });
         return json as unknown as T;
     }
 
-    private serializeFieldRecursive(
+    /**
+     * Recursively assign `value` into `json` while interpreting the dot-path of `element.name`
+     */
+    private assignValue(
         element: HTMLInputElement,
         value: unknown,
         json: { [key: string]: unknown },
@@ -211,6 +268,12 @@ export abstract class Form<T> extends AKElement {
         parent[nameElements[nameElements.length - 1]] = value;
     }
 
+    /**
+     * Serialize and send the form to the destination. The `send()` method must be overridden for
+     * this to work. If processing the data results in an error, we catch the error, distribute
+     * field-levels errors to the fields, and send the rest of them to the Notifications.
+     *
+     */
     async submit(ev: Event): Promise<unknown | undefined> {
         ev.preventDefault();
         try {

web/src/elements/forms/FormElement.ts

@@ -9,6 +9,12 @@ import PFFormControl from "@patternfly/patternfly/components/FormControl/form-co
 
 import { ErrorDetail } from "@goauthentik/api";
 
+/**
+ * This is used in two places outside of Flow, and in both cases is used primarily to
+ * display content, not take input.  It displays the TOPT QR code, and the static
+ * recovery tokens.  But it's used a lot in Flow.
+ */
+
 @customElement("ak-form-element")
 export class FormElement extends AKElement {
     static get styles(): CSSResult[] {

web/src/elements/forms/FormGroup.ts

@@ -8,11 +8,26 @@ import PFForm from "@patternfly/patternfly/components/Form/form.css";
 import PFFormControl from "@patternfly/patternfly/components/FormControl/form-control.css";
 import PFBase from "@patternfly/patternfly/patternfly-base.css";
 
+/**
+ * Form Group
+ *
+ * Mostly visual effects, with a single interaction for opening/closing the view.
+ *
+ */
+
+/**
+ * TODO: Listen for custom events from its children about 'invalidation' events, and
+ * trigger the `expanded` property as needed.
+ */
+
 @customElement("ak-form-group")
 export class FormGroup extends AKElement {
-    @property({ type: Boolean })
+    @property({ type: Boolean, reflect: true })
     expanded = false;
 
+    @property({ type: String, attribute: "aria-label", reflect: true })
+    ariaLabel = "Details";
+
     static get styles(): CSSResult[] {
         return [
             PFBase,
@@ -35,7 +50,7 @@ export class FormGroup extends AKElement {
                         class="pf-c-button pf-m-plain"
                         type="button"
                         aria-expanded="${this.expanded}"
-                        aria-label="Details"
+                        aria-label=${this.ariaLabel}
                         @click=${() => {
                             this.expanded = !this.expanded;
                         }}

web/src/elements/forms/HorizontalFormElement.ts

@@ -12,9 +12,30 @@ import PFFormControl from "@patternfly/patternfly/components/FormControl/form-co
 import PFBase from "@patternfly/patternfly/patternfly-base.css";
 
 /**
+ *
+ * Horizontal Form Element Container.
+ *
+ * This element provides the interface between elements of our forms and the
+ * form itself.
  * @custom-element ak-form-element-horizontal
  */
 
+/* TODO
+
+ * 1. Replace the "probe upward for a parent object to event" with an event handler on the parent
+ *    group.
+ * 2. Updated() has a lot of that slug code again. Really, all you want is for the slug input object
+ *    to update itself if its content seems to have been tracking some other key element.
+ * 3. Updated() pushes the `name` field down to the children, as if that were necessary; why isn't
+ *    it being written on-demand when the child is written? Because it's slotted... despite there
+ *    being very few unique uses.
+ * 4. There is some very specific use-case around the `writeOnly` boolean; this seems to be a case
+ *    where the field isn't available for the user to view unless they explicitly request to be able
+ *    to see the content; otherwise, a dead password field is shown. There are 10 uses of this
+ *    feature.
+ * 
+ */
+
 @customElement("ak-form-element-horizontal")
 export class HorizontalFormElement extends AKElement {
     static get styles(): CSSResult[] {
@@ -56,6 +77,9 @@ export class HorizontalFormElement extends AKElement {
 
     _invalid = false;
 
+    /* If this property changes, we want to make sure the parent control is "opened" so
+     * that users can see the change.[1]
+     */
     @property({ type: Boolean })
     set invalid(v: boolean) {
         this._invalid = v;

web/src/elements/forms/ModelForm.ts

@@ -5,6 +5,13 @@ import { Form } from "@goauthentik/elements/forms/Form";
 import { TemplateResult, html } from "lit";
 import { property } from "lit/decorators.js";
 
+/**
+ * Model form
+ *
+ * A base form that automatically tracks the server-side object (instance)
+ * that we're interested in.  Handles loading and tracking of the instance.
+ */
+
 export abstract class ModelForm<T, PKT extends string | number> extends Form<T> {
     abstract loadInstance(pk: PKT): Promise<T>;
 

web/src/elements/forms/Radio.ts

@@ -1,7 +1,9 @@
 import { AKElement } from "@goauthentik/elements/Base";
+import { CustomEmitterElement } from "@goauthentik/elements/utils/eventEmitter";
 
-import { CSSResult, TemplateResult, css, html } from "lit";
+import { CSSResult, TemplateResult, css, html, nothing } from "lit";
 import { customElement, property } from "lit/decorators.js";
+import { map } from "lit/directives/map.js";
 
 import PFForm from "@patternfly/patternfly/components/Form/form.css";
 import PFRadio from "@patternfly/patternfly/components/Radio/radio.css";
@@ -15,7 +17,7 @@ export interface RadioOption<T> {
 }
 
 @customElement("ak-radio")
-export class Radio<T> extends AKElement {
+export class Radio<T> extends CustomEmitterElement(AKElement) {
     @property({ attribute: false })
     options: RadioOption<T>[] = [];
 
@@ -36,44 +38,70 @@ export class Radio<T> extends AKElement {
                         var(--pf-c-form--m-horizontal__group-label--md--PaddingTop) * 1.3
                     );
                 }
+                .pf-c-radio label,
+                .pf-c-radio span {
+                    user-select: none;
+                }
             `,
         ];
     }
 
-    render(): TemplateResult {
+    constructor() {
+        super();
+        this.renderRadio = this.renderRadio.bind(this);
+        this.buildChangeHandler = this.buildChangeHandler.bind(this);
+    }
+
+    // Set the value if it's not set already. Property changes inside the `willUpdate()` method do
+    // not trigger an element update.
+    willUpdate() {
         if (!this.value) {
-            const def = this.options.filter((opt) => opt.default);
+            const maybeDefault = this.options.filter((opt) => opt.default);
-            if (def.length > 0) {
+            if (maybeDefault.length > 0) {
-                this.value = def[0].value;
+                this.value = maybeDefault[0].value;
             }
         }
-        return html`<div class="pf-c-form__group-control pf-m-stack">
+    }
-            ${this.options.map((opt) => {
+
-                const elId = `${this.name}-${opt.value}`;
+    // When a user clicks on `type="radio"`, *two* events happen in rapid succession: the original
-                return html`<div class="pf-c-radio">
+    // radio loses its setting, and the selected radio gains its setting. We want radio buttons to
+    // present a unified event interface, so we prevent the event from triggering if the value is
+    // already set.
+    buildChangeHandler(option: RadioOption<T>) {
+        return (ev: Event) => {
+            // This is a controlled input. Stop the native event from escaping or affecting the
+            // value.  We'll do that ourselves.
+            ev.stopPropagation();
+            ev.preventDefault();
+            this.value = option.value;
+            this.dispatchCustomEvent("change", option.value);
+            this.dispatchCustomEvent("input", option.value);
+        };
+    }
+
+    renderRadio(option: RadioOption<T>) {
+        const elId = `${this.name}-${option.value}`;
+        const handler = this.buildChangeHandler(option);
+        return html`<div class="pf-c-radio" @click=${handler}>
             <input
                 class="pf-c-radio__input"
                 type="radio"
                 name="${this.name}"
                 id=${elId}
-                        @change=${() => {
+                .checked=${option.value === this.value}
-                            this.value = opt.value;
-                            this.dispatchEvent(
-                                new CustomEvent("change", {
-                                    bubbles: true,
-                                    composed: true,
-                                    detail: opt.value,
-                                }),
-                            );
-                        }}
-                        .checked=${opt.value === this.value}
             />
-                    <label class="pf-c-radio__label" for=${elId}>${opt.label}</label>
+            <label class="pf-c-radio__label" for=${elId}>${option.label}</label>
-                    ${opt.description
+            ${option.description
-                        ? html`<span class="pf-c-radio__description">${opt.description}</span>`
+                ? html`<span class="pf-c-radio__description">${option.description}</span>`
-                        : html``}
+                : nothing}
         </div>`;
-            })}
+    }
+
+    render() {
+        return html`<div class="pf-c-form__group-control pf-m-stack">
+            ${map(this.options, this.renderRadio)}
         </div>`;
     }
 }
+
+export default Radio;

web/src/elements/forms/stories/Radio.stories.ts

@@ -0,0 +1,61 @@
+import "@goauthentik/elements/messages/MessageContainer";
+import { Meta } from "@storybook/web-components";
+
+import { TemplateResult, html } from "lit";
+
+import "../Radio";
+import Radio from "../Radio";
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+const metadata: Meta<Radio<any>> = {
+    title: "Elements / Basic Radio",
+    component: "ak-radio",
+    parameters: {
+        docs: {
+            description: {
+                component: "Our stylized radio button",
+            },
+        },
+    },
+};
+
+export default metadata;
+
+const container = (testItem: TemplateResult) =>
+    html` <div style="background: #fff; padding: 2em">
+        <style>
+            li {
+                display: block;
+            }
+            p {
+                margin-top: 1em;
+            }
+        </style>
+        <ak-message-container></ak-message-container>
+        ${testItem}
+        <ul id="radio-message-pad" style="margin-top: 1em"></ul>
+    </div>`;
+
+const testOptions = [
+    { label: "Option One", description: html`This is option one.`, value: 1 },
+    { label: "Option Two", description: html`This is option two.`, value: 2 },
+    { label: "Option Three", description: html`This is option three.`, value: 3 },
+];
+
+export const BasicRadioElement = () => {
+    const displayChange = (ev: InputEvent) => {
+        document.getElementById("radio-message-pad")!.innerText = `Value selected: ${JSON.stringify(
+            (ev.target as HTMLInputElement)!.value,
+            null,
+            2,
+        )}`;
+    };
+
+    return container(
+        html`<ak-radio
+            @input=${displayChange}
+            name="ak-test-radio-input"
+            .options=${testOptions}
+        ></ak-radio>`,
+    );
+};

web/src/elements/utils/eventEmitter.ts

@@ -9,16 +9,21 @@ export const isCustomEvent = (v: any): v is CustomEvent =>
 
 export function CustomEmitterElement<T extends Constructor<LitElement>>(superclass: T) {
     return class EmmiterElementHandler extends superclass {
-        dispatchCustomEvent(eventName: string, detail = {}, options = {}) {
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        dispatchCustomEvent(eventName: string, detail: any = {}, options = {}) {
+            const fullDetail =
+                typeof detail === "object" && !Array.isArray(detail)
+                    ? {
+                          target: this,
+                          ...detail,
+                      }
+                    : detail;
             this.dispatchEvent(
                 new CustomEvent(eventName, {
                     composed: true,
                     bubbles: true,
                     ...options,
-                    detail: {
+                    detail: fullDetail,
-                        target: this,
-                        ...detail,
-                    },
                 }),
             );
         }

web/src/elements/wizard/WizardFormPage.ts

@@ -19,6 +19,9 @@ export abstract class WizardForm extends Form<KeyUnknown> {
     @property({ attribute: false })
     nextDataCallback!: (data: KeyUnknown) => Promise<boolean>;
 
+    /* Override the traditional behavior of the form and instead simply serialize the form and push
+     * it's contents to the next page.
+     */
     async submit(): Promise<boolean | undefined> {
         const data = this.serializeForm();
         if (!data) {

web/src/elements/wizard/WizardPage.ts

@@ -21,10 +21,18 @@ export class WizardPage extends AKElement {
         return this.parentElement as Wizard;
     }
 
+    /**
+     * Called when this is the page brought into view
+     */
     activeCallback: () => Promise<void> = async () => {
         this.host.isValid = false;
     };
 
+    /**
+     * Called when the `next` button on the wizard is pressed. For forms, results in the submission
+     * of the current form to the back-end before being allowed to proceed to the next page. This is
+     * sub-optimal if we want to collect multiple bits of data before finishing the whole course.
+     */
     nextCallback: () => Promise<boolean> = async () => {
         return true;
     };