fix(web-forms#883): allow users to recover from xpath errors

.changeset/cool-dragons-divide.md

@@ -0,0 +1,7 @@
+---
+"@getodk/xforms-engine": minor
+"@getodk/web-forms": minor
+"@getodk/xpath": minor
+---
+
+Improved error messages for invalid forms.

packages/web-forms/locales/strings_en.json

@@ -19,9 +19,13 @@
     "string": "Next",
     "developer_comment": "Label for the button that moves to the next page in a paginated form."
   },
+  "odk_web_forms.evaluation.error": {
+    "string": "Error found while evaluating this form. Error message: \"{message}\". Reported by {count, plural, one {field} other {fields}}: {fields}. Please contact the person who sent you the form link.",
+    "developer_comment": "Form error banner message. {message} is the raw error message. {fields} is a string naming the fields affected. {count} is the number of fields reporting the error."
+  },
   "odk_web_forms.validation.error": {
     "string": "{count, plural, one {# question with error} other {# questions with errors}}",
-    "developer_comment": "Error banner message. {count} is the number of validation violations."
+    "developer_comment": "Validation error banner message. {count} is the number of validation issues."
   },
   "odk_web_forms.validation.view.label": {
     "string": "View",

packages/web-forms/src/components/OdkWebForm.i18n.json

@@ -11,9 +11,13 @@
     "string": "Next",
     "developer_comment": "Label for the button that moves to the next page in a paginated form."
   },
+  "odk_web_forms.evaluation.error": {
+    "string": "Error found while evaluating this form. Error message: \"{message}\". Reported by {count, plural, one {field} other {fields}}: {fields}. Please contact the person who sent you the form link.",
+    "developer_comment": "Form error banner message. {message} is the raw error message. {fields} is a string naming the fields affected. {count} is the number of fields reporting the error."
+  },
   "odk_web_forms.validation.error": {
     "string": "{count, plural, one {# question with error} other {# questions with errors}}",
-    "developer_comment": "Error banner message. {count} is the number of validation violations."
+    "developer_comment": "Validation error banner message. {count} is the number of validation issues."
   },
   "odk_web_forms.validation.view.label": {
     "string": "View",

packages/web-forms/src/components/OdkWebForm.vue

@@ -322,10 +322,28 @@ provide(REVEAL_VIOLATIONS, revealViolations);
 // It returns violations for questions the user has seen.
 const revealedViolations = computed(() => {
 	const violations = state.value.root?.validationState.violations ?? [];
+	const nonErrorViolations = violations.filter((violation) => violation.violation.condition !== 'error');
 	if (submitPressed.value) {
-		return violations;
+		return nonErrorViolations;
 	}
-	return violations.filter(({ nodeId }) => touchedQuestions.has(nodeId));
+	return nonErrorViolations.filter((violation) => touchedQuestions.has(violation.nodeId));
+});
+
+const errorViolations = computed(() => {
+	const violations = state.value.root?.validationState.violations ?? [];
+	const grouped = new Map<string, string[]>();
+	violations.forEach(error => {
+		const key = error.violation.condition === 'error' && error.violation.message;
+		if (!key) {
+			return;
+		}
+		const fieldReferences = grouped.get(key) ?? [];
+		const rootNodeName = state.value.root?.definition.nodeset;
+		const fieldReference = error.reference.replace(rootNodeName + '/', '');
+		fieldReferences.push(fieldReference);
+		grouped.set(key, fieldReferences);
+	});
+	return grouped;
 });
 
 const validationErrorMessage = computed(() => {
@@ -339,7 +357,11 @@ const showValidationError = computed(() => {
 	if (errorBannerDismissed.value) {
 		return false;
 	}
-	return !!(validationErrorMessage.value.length || geolocationErrorMessage.value?.length);
+	return (
+		!!validationErrorMessage.value.length ||
+		!!geolocationErrorMessage.value?.length ||
+		!!errorViolations.value.size
+	);
 });
 
 onUnmounted(() => {
@@ -389,6 +411,13 @@ onUnmounted(() => {
 			>
 				<IconSVG name="mdiAlertCircleOutline" variant="error" />
 				<ul class="form-error-text-wrap">
+					<li v-for="[message, references] in errorViolations" :key="message">
+						{{ t('odk_web_forms.evaluation.error', {
+							message,
+							fields: references.join(', '),
+							count: references.length
+						}) }}
+					</li>
 					<li v-if="validationErrorMessage?.length">
 						{{ validationErrorMessage }}
 					</li>

packages/web-forms/src/components/common/ValidationMessage.vue

@@ -39,7 +39,12 @@ const defaultMessage = computed(() => {
 	<div :class="{ 'validation-placeholder': addPlaceholder }">
 		<span v-show="showMessage" class="validation-message">
 			<template v-if="violation?.message">
-				<MarkdownBlock v-for="elem in violation.message.formatted" :key="elem.id" :elem="elem" />
+				<template v-if="violation?.condition === 'error'">
+					<span>{{ violation.message }}</span>
+				</template>
+				<template v-else>
+					<MarkdownBlock v-for="elem in violation.message.formatted" :key="elem.id" :elem="elem" />
+				</template>
 			</template>
 			<template v-else-if="defaultMessage">{{ defaultMessage }}</template>
 		</span>

packages/web-forms/tests/components/OdkWebForm.test.ts

@@ -332,24 +332,16 @@ describe('OdkWebForm', () => {
       expect(component.get('.form-load-failure-dialog').isVisible()).toBe(true);
     });
 
-    // TODO: tests failure which is currently produced by throwing a string.
-    // Checking the text content here is intended to ensure we are actually
-    // presenting the message to a user.
     it('presents an error message when failing to load a form with a computation referencing an unknown XPath function', async () => {
       const xpathUnknownFunctionFormXML = await getWebFormsTestFixture(
         'xpath-unknown-function.xml'
       );
       const component = mountComponent(xpathUnknownFunctionFormXML);
-
       await flushPromises();
-
-      const formLoadFailureDialog = component.get('.form-load-failure-dialog');
-
-      expect(formLoadFailureDialog.isVisible()).toBe(true);
-
-      const message = formLoadFailureDialog.get('.message');
-
-      expect(message.text()).toMatch(/\bnope\b/);
+      expectErrorBanner(
+        component,
+        'Error found while evaluating this form. Error message: "Unknown function in form definition: \'nope\'". Reported by field: first-question. Please contact the person who sent you the form link.'
+      );
     });
   });
 

packages/xforms-engine/src/client/TextRange.ts

@@ -79,7 +79,7 @@ export interface TextChunk {
 // eslint-disable-next-line @typescript-eslint/sort-type-constituents
 export type ElementTextRole = 'hint' | 'label' | 'item-label';
 export type ValidationTextRole = 'constraintMsg' | 'requiredMsg';
-export type TextRole = ElementTextRole | ValidationTextRole;
+export type TextRole = ElementTextRole | ValidationTextRole | 'errorMsg';
 
 /**
  * Represents aspects of a form which produce text, which _might_ be:

packages/xforms-engine/src/client/validation.ts

@@ -1,10 +1,14 @@
+import type { Attribute } from '../instance/Attribute.ts';
 import type { BaseNode, BaseNodeState } from './BaseNode.ts';
+import type { AnyNode } from './hierarchy.ts';
 import type { FormNodeID } from './identity.ts';
 import type { OpaqueReactiveObjectFactory } from './OpaqueReactiveObjectFactory.ts';
 import type { TextRange } from './TextRange.ts';
 
 // This interface exists so that extensions can share JSDoc for `valid`.
-interface BaseValidity {
+interface BaseValidity<Condition> {
+  readonly condition: Condition;
+
   /**
    * Specifies the unambiguous validity state for each validity condition of a
    * given node, or for the derived validity of any parent node whose descendants
@@ -24,6 +28,9 @@ interface BaseValidity {
    * - \* = default (expression not defined)
    * - ✅ = `valid: true`
    * - ❌ = `valid: false`
+   *
+   * `error` condition represents an invalid state, for example, if an xpath expression
+   * cannot be parsed.
    */
   readonly valid: boolean;
 }
@@ -33,11 +40,12 @@ interface BaseValidity {
  *
  * @see {@link https://getodk.github.io/xforms-spec/#bind-attributes | `constraint` and `required` bind attributes}
  */
-export type ValidationCondition = 'constraint' | 'required';
+export type ValidationCondition = 'constraint' | 'error' | 'required';
 
 interface ValidationConditionMessageRoles {
   readonly constraint: 'constraintMsg';
   readonly required: 'requiredMsg';
+  readonly error: 'errorMsg';
 }
 
 export type ValidationConditionMessageRole<Condition extends ValidationCondition> =
@@ -57,23 +65,33 @@ export interface ViolationMessage<Condition extends ValidationCondition> extends
   get asString(): string;
 }
 
-export interface ConditionSatisfied<Condition extends ValidationCondition> extends BaseValidity {
-  readonly condition: Condition;
+export interface ConditionSatisfied<
+  Condition extends ValidationCondition,
+> extends BaseValidity<Condition> {
   readonly valid: true;
   readonly message: null;
 }
 
-export interface ConditionViolation<Condition extends ValidationCondition> extends BaseValidity {
-  readonly condition: Condition;
+export interface ConstraintViolation extends BaseValidity<'constraint'> {
+  readonly valid: false;
+  readonly message: ViolationMessage<'constraint'> | null;
+}
+
+export interface RequiredViolation extends BaseValidity<'required'> {
   readonly valid: false;
-  readonly message: ViolationMessage<Condition> | null;
+  readonly message: ViolationMessage<'required'> | null;
+}
+
+export interface ErrorViolation extends BaseValidity<'error'> {
+  readonly valid: false;
+  readonly message: string | null;
 }
 
 export type ConditionValidation<Condition extends ValidationCondition> =
-  | ConditionSatisfied<Condition>
-  | ConditionViolation<Condition>;
+  | AnyViolation
+  | ConditionSatisfied<Condition>;
 
-export type AnyViolation = ConditionViolation<ValidationCondition>;
+export type AnyViolation = ConstraintViolation | ErrorViolation | RequiredViolation;
 
 /**
  * Represents the validation state of a leaf (or value) node.
@@ -129,6 +147,7 @@ export interface LeafNodeValidationState {
 export interface DescendantNodeViolationReference {
   readonly nodeId: FormNodeID;
 
+  get node(): AnyNode | Attribute;
   get reference(): string;
   get violation(): AnyViolation;
 }
@@ -150,15 +169,7 @@ export interface AncestorNodeValidationState {
   get violations(): readonly DescendantNodeViolationReference[];
 }
 
-/**
- * Convenience interface for nodes that cannot be invalid.
- */
-export interface NullValidationState {
-  get violations(): readonly [];
-}
-
 // prettier-ignore
 export type NodeValidationState =
 	| AncestorNodeValidationState
-	| LeafNodeValidationState
-	| NullValidationState;
+	| LeafNodeValidationState;

packages/xforms-engine/src/instance/Attribute.ts

@@ -1,7 +1,7 @@
 import { XPathNodeKindKey } from '@getodk/xpath';
 import type { Accessor } from 'solid-js';
 import type { AttributeNode } from '../client/AttributeNode.ts';
-import type { InstanceState, NullValidationState } from '../client/index.ts';
+import type { AncestorNodeValidationState, InstanceState } from '../client/index.ts';
 import type { XFormsXPathAttribute } from '../integration/xpath/adapter/XFormsXPathNode.ts';
 import type { StaticAttribute } from '../integration/xpath/static-dom/StaticAttribute.ts';
 import { createAttributeNodeInstanceState } from '../lib/client-reactivity/instance-state/createAttributeNodeInstanceState.ts';
@@ -51,7 +51,7 @@ export class Attribute
 
   protected readonly state: SharedNodeState<AttributeStateSpec>;
   protected readonly engineState: EngineState<AttributeStateSpec>;
-  readonly validationState: NullValidationState;
+  readonly validationState: AncestorNodeValidationState;
 
   readonly nodeType = 'attribute';
   readonly currentState: CurrentState<AttributeStateSpec>;
@@ -146,6 +146,10 @@ export class Attribute
     };
   }
 
+  protected override canReportViolation(): boolean {
+    return true;
+  }
+
   setValue(value: string): Root {
     this.setValueState(value);
 

packages/xforms-engine/src/instance/Root.ts

@@ -12,7 +12,11 @@ import type {
   InstancePayloadType,
 } from '../client/serialization/InstancePayloadOptions.ts';
 import type { InstanceState } from '../client/serialization/InstanceState.ts';
-import type { AncestorNodeValidationState, BlockingViolations } from '../client/validation.ts';
+import type {
+  AncestorNodeValidationState,
+  BlockingViolations,
+  DescendantNodeViolationReference,
+} from '../client/validation.ts';
 import type { XFormsXPathElement } from '../integration/xpath/adapter/XFormsXPathNode.ts';
 import { createRootInstanceState } from '../lib/client-reactivity/instance-state/createRootInstanceState.ts';
 import {
@@ -34,10 +38,11 @@ import { createAggregatedViolations } from '../lib/reactivity/validation/createA
 import type { BodyClassList } from '../parse/body/BodyDefinition.ts';
 import type { RootDefinition } from '../parse/model/RootDefinition.ts';
 import { DescendantNode } from './abstract/DescendantNode.ts';
+import { ValueNode } from './abstract/ValueNode.ts';
 import { buildAttributes } from './buildAttributes.ts';
 import { Attribute } from './Attribute.ts';
 import { buildChildren } from './children/buildChildren.ts';
-import type { GeneralChildNode } from './hierarchy.ts';
+import type { AnyControlInstanceNode, GeneralChildNode } from './hierarchy.ts';
 import type { EvaluationContext } from './internal-api/EvaluationContext.ts';
 import type { ClientReactiveSerializableParentNode } from './internal-api/serialization/ClientReactiveSerializableParentNode.ts';
 import type { TranslationContext } from './internal-api/TranslationContext.ts';
@@ -73,6 +78,17 @@ interface RootStateSpec {
   readonly navigationTarget: Accessor<FormNodeID | null>;
 }
 
+const findViolationControl = (
+  reference: DescendantNodeViolationReference
+): AnyControlInstanceNode | null => {
+  const { node } = reference;
+  const target = node.nodeType === 'attribute' ? node.owner : node;
+  if (target instanceof ValueNode && target.nodeType !== 'model-value') {
+    return target;
+  }
+  return null;
+};
+
 export class Root
   extends DescendantNode<RootDefinition, RootStateSpec, PrimaryInstance, GeneralChildNode>
   implements
@@ -221,15 +237,20 @@ export class Root
     return [];
   }
 
-  // Nodes without a page (model-only values) have no leaf page id, so they never block.
+  private isViolationOnPage(reference: DescendantNodeViolationReference, page: PageBoundary) {
+    const control = findViolationControl(reference);
+    return control != null && this.pagination.getLeafPageId(control.nodeId) === page;
+  }
+
+  // Only violations of controls block, other nodes (groups, model-only values) have no page.
   getBlockingViolations(): BlockingViolations {
     const currentPage = this.pageNavigation.currentPage();
     if (currentPage == null) {
       return [];
     }
 
-    return this.validationState.violations.filter(({ nodeId }) => {
-      return this.pagination.getLeafPageId(nodeId) === currentPage;
+    return this.validationState.violations.filter((reference) => {
+      return this.isViolationOnPage(reference, currentPage);
     });
   }
 
@@ -242,17 +263,19 @@ export class Root
   }
 
   navigateToFirstViolation(): void {
-    const violation = this.validationState.violations?.[0];
-    if (violation == null) {
+    const control = this.validationState.violations
+      .map((reference) => findViolationControl(reference))
+      .find((violationControl) => violationControl != null);
+    if (control == null) {
       return;
     }
 
     batch(() => {
-      const page = this.pagination.getLeafPageId(violation.nodeId);
+      const page = this.pagination.getLeafPageId(control.nodeId);
       if (page != null) {
         this.setCurrentPage(page);
       }
-      this.setNavigationTarget(violation.nodeId);
+      this.setNavigationTarget(control.nodeId);
     });
   }