From 3b171a02b78e259f50f31c13bffcdf60d60d22f0 Mon Sep 17 00:00:00 2001 From: Ken Sternberg <133134217+kensternberg-authentik@users.noreply.github.com> Date: Mon, 2 Oct 2023 13:33:27 -0700 Subject: [PATCH] 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 --------- Signed-off-by: Jens Langhammer Co-authored-by: Jens Langhammer --- Makefile | 7 +- web/.eslintrc.json | 12 ++- .../ak-locale-context/ak-locale-context.ts | 3 - web/src/elements/buttons/ModalButton.ts | 6 +- web/src/elements/forms/Form.ts | 81 ++++++++++++++-- web/src/elements/forms/FormElement.ts | 6 ++ web/src/elements/forms/FormGroup.ts | 19 +++- .../elements/forms/HorizontalFormElement.ts | 24 +++++ web/src/elements/forms/ModelForm.ts | 7 ++ web/src/elements/forms/Radio.ts | 92 ++++++++++++------- .../elements/forms/stories/Radio.stories.ts | 61 ++++++++++++ web/src/elements/utils/eventEmitter.ts | 15 ++- web/src/elements/wizard/WizardFormPage.ts | 3 + web/src/elements/wizard/WizardPage.ts | 8 ++ 14 files changed, 287 insertions(+), 57 deletions(-) create mode 100644 web/src/elements/forms/stories/Radio.stories.ts diff --git a/Makefile b/Makefile index a6c4b3e69..54c53afdf 100644 --- a/Makefile +++ b/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) | \ - awk 'BEGIN {FS = ":.*?## "}; {printf " \033[0;36m%-24s\033[m %s\n", $$1, $$2}' | \ + @grep -Eh '^[0-9a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | \ + awk 'BEGIN {FS = ":.*?## "}; {printf " \033[0;36m%-$(HELP_WIDTH)s \033[m %s\n", $$1, $$2}' | \ sort @echo "" diff --git a/web/.eslintrc.json b/web/.eslintrc.json index a423627c0..fdae375c6 100644 --- a/web/.eslintrc.json +++ b/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": "^_" + } + ] } } diff --git a/web/src/elements/ak-locale-context/ak-locale-context.ts b/web/src/elements/ak-locale-context/ak-locale-context.ts index 304ddacac..af69fbf19 100644 --- a/web/src/elements/ak-locale-context/ak-locale-context.ts +++ b/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; diff --git a/web/src/elements/buttons/ModalButton.ts b/web/src/elements/buttons/ModalButton.ts index 9a2b4fd0d..dfc594e2c 100644 --- a/web/src/elements/buttons/ModalButton.ts +++ b/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``; } @@ -136,6 +136,6 @@ export class ModalButton extends AKElement { render(): TemplateResult { return html` this.onClick()}> - ${this.open ? this.renderModal() : ""}`; + ${this.open ? this.renderModal() : nothing}`; } } diff --git a/web/src/elements/forms/Form.ts b/web/src/elements/forms/Form.ts index 6c69365a7..6da2baff7 100644 --- a/web/src/elements/forms/Form.ts +++ b/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 extends AKElement { abstract send(data: T): Promise; @@ -61,6 +92,10 @@ export abstract class Form 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 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 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 extends AKElement { return files; } + /** + * Convert the elements of the form to JSON.[4] + * + */ serializeForm(): T | undefined { const elements = this.shadowRoot?.querySelectorAll( @@ -147,17 +197,21 @@ export abstract class Form 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 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; 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 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 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 { ev.preventDefault(); try { diff --git a/web/src/elements/forms/FormElement.ts b/web/src/elements/forms/FormElement.ts index b910caa90..bb408af88 100644 --- a/web/src/elements/forms/FormElement.ts +++ b/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[] { diff --git a/web/src/elements/forms/FormGroup.ts b/web/src/elements/forms/FormGroup.ts index 347a06a40..0c867d719 100644 --- a/web/src/elements/forms/FormGroup.ts +++ b/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; }} diff --git a/web/src/elements/forms/HorizontalFormElement.ts b/web/src/elements/forms/HorizontalFormElement.ts index 3709536b0..9d00327da 100644 --- a/web/src/elements/forms/HorizontalFormElement.ts +++ b/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; diff --git a/web/src/elements/forms/ModelForm.ts b/web/src/elements/forms/ModelForm.ts index f61a0c659..c45cc7344 100644 --- a/web/src/elements/forms/ModelForm.ts +++ b/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 extends Form { abstract loadInstance(pk: PKT): Promise; diff --git a/web/src/elements/forms/Radio.ts b/web/src/elements/forms/Radio.ts index 55b60a3ac..3d82ca4cb 100644 --- a/web/src/elements/forms/Radio.ts +++ b/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 { } @customElement("ak-radio") -export class Radio extends AKElement { +export class Radio extends CustomEmitterElement(AKElement) { @property({ attribute: false }) options: RadioOption[] = []; @@ -36,44 +38,70 @@ export class Radio 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); - if (def.length > 0) { - this.value = def[0].value; + const maybeDefault = this.options.filter((opt) => opt.default); + if (maybeDefault.length > 0) { + this.value = maybeDefault[0].value; } } + } + + // When a user clicks on `type="radio"`, *two* events happen in rapid succession: the original + // 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) { + 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) { + const elId = `${this.name}-${option.value}`; + const handler = this.buildChangeHandler(option); + return html`
+ + + ${option.description + ? html`${option.description}` + : nothing} +
`; + } + + render() { return html`
- ${this.options.map((opt) => { - const elId = `${this.name}-${opt.value}`; - return html`
- { - this.value = opt.value; - this.dispatchEvent( - new CustomEvent("change", { - bubbles: true, - composed: true, - detail: opt.value, - }), - ); - }} - .checked=${opt.value === this.value} - /> - - ${opt.description - ? html`${opt.description}` - : html``} -
`; - })} + ${map(this.options, this.renderRadio)}
`; } } + +export default Radio; diff --git a/web/src/elements/forms/stories/Radio.stories.ts b/web/src/elements/forms/stories/Radio.stories.ts new file mode 100644 index 000000000..20f32f922 --- /dev/null +++ b/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> = { + title: "Elements / Basic Radio", + component: "ak-radio", + parameters: { + docs: { + description: { + component: "Our stylized radio button", + }, + }, + }, +}; + +export default metadata; + +const container = (testItem: TemplateResult) => + html`
+ + + ${testItem} +
    +
    `; + +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``, + ); +}; diff --git a/web/src/elements/utils/eventEmitter.ts b/web/src/elements/utils/eventEmitter.ts index 9afa8be3b..54b472825 100644 --- a/web/src/elements/utils/eventEmitter.ts +++ b/web/src/elements/utils/eventEmitter.ts @@ -9,16 +9,21 @@ export const isCustomEvent = (v: any): v is CustomEvent => export function CustomEmitterElement>(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: { - target: this, - ...detail, - }, + detail: fullDetail, }), ); } diff --git a/web/src/elements/wizard/WizardFormPage.ts b/web/src/elements/wizard/WizardFormPage.ts index 2d3e5b3b4..60b386545 100644 --- a/web/src/elements/wizard/WizardFormPage.ts +++ b/web/src/elements/wizard/WizardFormPage.ts @@ -19,6 +19,9 @@ export abstract class WizardForm extends Form { @property({ attribute: false }) nextDataCallback!: (data: KeyUnknown) => Promise; + /* 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 { const data = this.serializeForm(); if (!data) { diff --git a/web/src/elements/wizard/WizardPage.ts b/web/src/elements/wizard/WizardPage.ts index 6fdee111f..3a6be581b 100644 --- a/web/src/elements/wizard/WizardPage.ts +++ b/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 = 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 = async () => { return true; };