Constraint/required validation UI

[sadiqkhoja]

Closes #130

Changes:

• Added new icons for error in icomoon font
• Added violation message for leaf nodes, message is shown only when the field value is changed or 'send' button is pressed. I add dirty class to the control when its value is changed.
• Added Form validation banner at the top, it is shown only when 'send' button is pressed. It has 'fix error' link that scrolls to the first invalid question.

[changeset-bot]

🦋 Changeset detected

Latest commit: 82d35d0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@getodk/ui-solid	Patch
@getodk/web-forms	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[eyelidlessness]

Great to see this in action! Overall it's a great first step on one of the remaining bits of essential functionality for many use cases.

I've added a fair bit of commentary, much of it around approach. While I'd like to spend some time thinking more deeply about the questions I've raised in those comments, I'd generally prioritize them lower (and happy to defer to future work) than a few areas of UX which feel need some improvement.

To my mind, I'd prioritize those (highest to lowest):

1. Reducing the difference between conditions for first- and subsequent-display of input validation messages. I'm not totally sure the suggestion I made is the best/only way to achieve that, but my experience before/after the suggested change is that it's drastically easier to understand what happens in various cases I've tried (both in terms of initial form definition, and various usage and state flows).
2. Some refinement of the form-wide sticky banner for narrow screens. As I mentioned inline, a single-breakpoint to switch from 100% to 70% width would be a great start. I'm happy to consider more specific improvements as well.
3. I do think it'd be good now to anticipate app translations, with some preliminary structure for referencing static/templated user-facing text. This could be as simple as importing those from a common module.

I have several concerns about scrolling (and DOM scroll APIs) as a mechanism for in-form navigation. I think the most immediately pressing aspect there is also the one with the most immediate UX impact: focus. I'd be happy to defer the rest of the scrolling related concerns, but it's worth thinking about whether there's a quick win in terms of jump to first validation error -> focus field with first validation error. I think the rest may come into play more as we address other view modes, other aspects of in-form navigation, etc. But if we can reasonably bring the issue of focus into scope, it'll be good for a variety of accessibility/usability reasons.

[eyelidlessness]

I don't know if it's best to put this here, it's more of a holistic concern. I'd like to think about leaning more heavily on flex/grid to handle spacing concerns like this. Using margin is notoriously problematic for a ton of reasons. I've had this concern in numerous places while I look through this PR, but it really stands out here because this is pretty much a perfect example of why margin is a poor fit for spacing when other options are available. I think I know why it is changing here, but I would never in a million years be able to look back at a git blame and make the connection. I really only make the connection now because I'm trying very hard to keep all of the cross-component coupling in my head at once.

[sadiqkhoja]

I will handle this with #173 (comment)

[eyelidlessness]

Getting really close! I left a few suggestions inline. Feel free to take them or leave them now, but I'd strongly encourage the ones related to scrolling and mocking.

I would like to see a changeset before merge. I'd be happy to add these for you when they're missing, if that's what you'd prefer. Otherwise I'll suggest that I've found the GitHub bot provides a helpful reminder.

These are the aspects I think are worth addressing or considering now.

I'll add a separate comment with a variety of other notes I took during this review pass, which we can file in one or more issues for followup as appropriate.

[eyelidlessness]

If we still want this...

Suggested change

	window.scrollTo(0,0);
	document.scrollingElement?.scrollTo(0, 0);

Both because it's more flexible for host applications, and because it doesn't seem to produce error logs in tests.

[eyelidlessness]

I know there's a lot here, but it's worth considering. There are a few good candidates for moving to somewhere shared/reusable, but we can start here for now.

Main goals:

1. Clean up any mutation of global scope/prototypes
2. Use mocking facilities consistently
3. Generalize enough that we can apply similar techniques without a lot of extra fuss in the future

Suggested change

	if (!Element.prototype.scrollIntoView) {
	// eslint-disable-next-line @typescript-eslint/no-empty-function
	Element.prototype.scrollIntoView = () => {};
	}
	if (!HTMLElement.prototype.showPopover) {
	HTMLElement.prototype.showPopover = function () {
	this.style.display = 'block';
	};
	}
	if (!HTMLElement.prototype.hidePopover) {
	HTMLElement.prototype.hidePopover = function () {
	this.style.display = 'none';
	};
	}
	// TODO: how the heck is `undefined` a key of anything?!
	type StringKeyOf<T> = Extract<keyof T, string>;
	type StringKeyOfDocument = StringKeyOf<Document>;
	// prettier-ignore
	type DocumentPropertyName = {
	[PropertyName in StringKeyOfDocument]:
	Document[PropertyName] extends AnyFunction
	? never
	: PropertyName;
	}[StringKeyOfDocument];
	let documentKeysAdded: StringKeyOfDocument[] = [];
	const mockDocumentGetter = <PropertyName extends DocumentPropertyName>(
	propertyName: PropertyName,
	mockImplementation: () => Document[PropertyName]
	) => {
	if (propertyName in document) {
	return vi.spyOn(document, propertyName, 'get').mockImplementation(mockImplementation);
	}
	documentKeysAdded.push(propertyName);
	const mock = vi.fn(mockImplementation);
	Object.defineProperty(document, propertyName, {
	get: mock,
	configurable: true,
	});
	return mock;
	};
	type StringKeyOfHTMLElement = StringKeyOf<HTMLElement>;
	// prettier-ignore
	type ElementMethodName = {
	[PropertyName in StringKeyOfHTMLElement]:
	HTMLElement[PropertyName] extends AnyFunction
	? PropertyName
	: never;
	}[keyof HTMLElement];
	type ElementMethodMock<MethodName extends ElementMethodName> = (
	this: HTMLElement,
	...args: Parameters<HTMLElement[MethodName]>
	) => ReturnType<HTMLElement[MethodName]>;
	let elementKeysAdded: StringKeyOfHTMLElement[] = [];
	const mockElementPrototypeMethod = <MethodName extends ElementMethodName>(
	methodName: MethodName,
	mockImplementation: ElementMethodMock<MethodName>
	) => {
	if (methodName in HTMLElement.prototype) {
	return vi.spyOn(HTMLElement.prototype, methodName).mockImplementation(function (...args) {
	// eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-explicit-any
	return mockImplementation.apply(this as HTMLElement, args as any);
	});
	}
	elementKeysAdded.push(methodName);
	const mock = vi.fn(mockImplementation);
	// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-explicit-any
	HTMLElement.prototype[methodName] = mock as any;
	return mock;
	};
	beforeEach(() => {
	documentKeysAdded = [];
	elementKeysAdded = [];
	// Not necessary if we scroll `document.scrollingElement` instead.
	// Including to show how it'd be done if not.
	vi.stubGlobal('scrollTo', () => {
	/* Do nothing, unless we test this behavior. */
	});
	mockDocumentGetter('scrollingElement', () => {
	return document.documentElement;
	});
	mockElementPrototypeMethod('showPopover', function () {
	this.style.display = 'block';
	});
	mockElementPrototypeMethod('hidePopover', function () {
	this.style.display = 'none';
	});
	});
	afterEach(() => {
	documentKeysAdded.forEach((propertyName) => {
	delete document[propertyName];
	});
	elementKeysAdded.forEach((methodName) => {
	delete HTMLElement.prototype[methodName];
	});
	vi.restoreAllMocks();
	vi.unstubAllGlobals();
	});

(You'll also need these import changes:)

- import { describe, expect, it } from 'vitest';
+ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+ import type { AnyFunction } from '@getodk/common/types/helpers.d.ts';

[eyelidlessness]

This is at least 2 tests. Fine for now, but probably worth breaking up when we revisit the programmatic in-form navigation aspect.

[eyelidlessness]

(Sorry if any formatting is off here, or anything is confusing. These were notes I was scrawling in Notes.app while I reviewed, and may not have clarified or Markdown-ified everything sufficiently.)

Nice to have in follow-up PR(s)

Vue conventions

• When using provide/inject, would be nice to establish a pattern where they use normal references so the connection between provided/injected state can be followed in editor, and types can be defined in provide. I did a quick spike on this not long ago and it seemed possible, happy to dig up the stash if there’s interest.
• Events make me nervous for reasons similar to provide/inject. They seem a bit easier to follow at component boundaries, but the lack of clear types with a single source of truth is still concerning.
• Consider some DOM-level way of identifying which component rendered what to the DOM
• Maybe there’s a lint rule (or maybe we could write one trivially?) to ensure consistent use of class/:class? It’s confusing every time I see both used on the same element. (And maybe I’ll get over that seeing it more?)
• Consider extracting event handlers into functions, especially where their logic is repeated. Case that stood out/made me think about this: several additions of @change="touched = true”.
• How do we choose between when to apply utility classes, versus using styles that achieve the same? (flex etc stand out)
• The tooling boundary between .vue and .ts modules is a continuing headache. Common case: TS/ESLint differing on whether a type is any. Case that stood out here: props interface exported from component, referenced in test. Would like to consider other approaches that minimize this. And/or set aside some time to see if any tooling updates address these shortcomings.
• I’m normally not fond of “test helpers”, but have some concern about inconsistent use of mount and globalMountOptions: especially (but not only) whether or not PrimeVue plugin is in use. Worth considering a shared custom mount helper to ensure we’re testing all components under the same assumptions.

Validation

• It would be good if select constraint messages don’t show up on first selection if user hasn’t moved on. Inputs use blur. It would require a little more logic to produce a similar experience for selects as radios/checkboxes (e.g. some delay on blur to cancel on interaction with additional items)

Styles

• Language menu in wide view should be its widest available width and keep a consistent width when switching languages
• Needs better engine support: would be nice to not show validation placeholder where a field will never show a validation message (i.e. has neither a constraint or required expression)
• Some way of visually distinguishing UnsupportedAppearance (and similar dev-focused components, maybe worth having a base component for them to compose)
• FormHeader.vue:

  • .smaller-screens .print-button doesn’t render justified end when form has no translations

  • Consider changing toggle of language menu visibility to languages.length > 1. (It’s not common, but I’ve seen some forms define only a single language; also worth considering suppressing languages in the engine for this scenario instead)

  • .smaller-screens shadow looks larger/heavier than design. Much closer if we replace the current filter rule with:

    position: relative; /* to ensure shadow is visible */
    box-shadow: 0px 2px 6px 0px rgba(0, 0, 0, 0.15), 0px 1px 2px 0px rgba(0, 0, 0, 0.3);

    This does differ from the styles produced by Figma. I suspect we’ll need similar finesse/eyeball comparison for other styles we derive from the designs directly

• In Design tweaks (#139) #173, made comment about leaning more on flex/grid for alignment instead of small pixel/small unit alignment adjustments
• Naming things: maybe this will be better addressed with PrimeVue 4, but it would be nice if we had fewer magic values and more use of system of CSS variables or other named values. Will hopefully make it easier to understand why something changes, e.g. font-weight 400 to 500 (or vice versa), font-size changes, accommodations for spacing moving up or down component/DOM hierarchy
• Various use of negative margins is concerning. Especially where it seems to be counteracting spacing controlled by the same general thing (FormPanel). Would generally like to do a focused overhaul on how layout/spacing/alignment/etc are handled.
  • In such focused overhaul, also want to be mindful of RTL where we have styles producing asymmetric horizontal spacing/alignment
• There’s a layout shift when validation message is shown/hidden within rows of FieldListTable

Component structure/naming

• (Not directly related to PR, want to track as it’s come up in recent conversation) FieldListTable naming is confusing. I think we’ve both repeatedly associated it with the field-list appearance, but it is not.
• In general, worth thinking about:
  • System for naming and categorical structuring of components
  • How that can/will align with designs in Figma, tokens in PrimeVue, our own token system
• Adding the same thing in multiple places can be a smell, may suggest there’s a composable component wanting to exist. Case that stood out/made me think about this: several additions of @change="touched = true”.
• Props naming:
  • “add” is a weird prefix for something that’s not an action/handler/callback. In general, verbs tend to suggest some kind of event handling or interaction -> state reaction.
• Events naming: we should consider developing a predictable system for this as well

Interactivity

• A little surprised by DOM API usage for popover, but didn’t realize PrimeVue’s Popover component was introduced in v4. We should track migration to that component in the v4 transition, if it’s suitable. If not, worth considering a composable base component to isolate the DOM API aspects in an obvious place