Initial selects support: (select, select1, item, itemset) #14

eyelidlessness · 2023-12-07T00:42:59Z

Screen.Recording.2023-12-06.at.4.38.16.PM.mov

Screen.Recording.2023-12-06.at.4.38.39.PM.mov

Expression/dependency generalization

Quite a bit of this change is focused on generalizing the way various expressions contribute to the dependency graph. I'm not entirely thrilled with the exact solution I arrived at, but it was running rather long and I think it'll be better to discuss than keep trying to refine on my own.

In general, the idea is that several (and growing) sub-structures of XFormDefinition can be one of:

DependentExpression: corresponding to an XPath expression in an XForm definition, potentially dependent on references to other nodes; examples include bind computations (e.g. calculate, relevant; finally generalizing the concept in what was previously named BindExpression), label references (this also generalizes the concept of active language state as a dependency), and now dynamic select expressions (<itemset nodeset>, <value ref>, <label ref> in an itemset context)
DependencyContext: corresponding to any number of nodes referencing a <bind nodeset> (this isn't explicitly enforced, but in general the design fell naturally into types which already do so)

There are some corresponding refinements to reactive state around such expressions. What that means is that all of the following now generally share the same underlying state logic (in some cases deriving specializations from it):

Bind computations
Label (and presumably hint) ref translations
New: select itemsets nodeset references and their corresponding value/label refs
Newish/bonus: outputs are now reactive as well

`instance()` XPath support

Following that, there is the @odk/xpath implementation of the instance() function, also to support itemsets. I hope this part is pretty straightforward. It's worth noting that this also includes optimization of the jr:itext() function introduced in #13 (and I'd be open to squashing these two, although both are surely large enough in their own right).

Selects proper

Finally, there is the set of concerns specific to selects (<select>, <select1>) and items (<item>, <itemset>). Again I hope this is mostly pretty straightforward, as most of it builds on the expression/dependency generalizations above. There are some notes in the comments about how state is handled (or not) for <select><itemset>s with filters, which is worth some attention.

What's missing

Tests specific to select definition/logic. Quite a lot of the shared logic is already tested in other ways (although it certainly could be much moreso). I'm a little bit hesitant to keep building up the current unit-ish tests around the current interfaces, and I'm eager to port more JavaRosa tests. But there's still more Scenario interface to cover to get there, and I'm also eager to get this into review. I will say that aside from the couple of demo forms included in this PR, I also used one of our favorite real world forms to spot check, and I'm pretty happy with how it's coming along as of this PR!

The same types will be used to handle the growing set of expressions which contribute to the dependency graph, such as those currently implemented (e.g. outputs, translated labels/hints) and coming soon (itemsets/values/labels, repeat jr:count)

Includes dependencies determined by parsing body elements: - to establish reactivity for those expressions (i.e. <output value>) - to consolidate reactivity for dependent expressions which didn’t already use this mechanism (i.e. <label ref>) - to set precedent for coming <itemset> support - to include all such dependencies consistently in topological sort

… and improve XFormsXPathEvaluator constructor performance when `rootNode` is in a form with a very large number of itext translations. - Both `instance` and `jr:itext` follow the same broad strokes approach: lookup maps, cached on demand. - `jr:itext` isn’t quite as on-demand, as available/default languages are expected to be needed upfront (although even this could probably be deferred, as the current @odk/web-forms init process constructs more than one evaluator instance, but currently only references translations from one of them) - `jr:itext` is inherently more complicated, because: - it is inherently stateful (as discussed in the first translations PR) - its lookups are nested - its retrieved values are derived, and again from a nested structure within the resolved node (i.e. whereas `instance(id)` maps directly to an <instance id> element, `jr:itext(id)` maps to `<text id><value>**THIS**</value></text>` The `instance` cache is probably an unnecessary micro-optimization **for now**, but it’s a bit of a hint at how a more optimal XForms evaluator could work, where all manner of static (or even semi-static) filters/steps/predicates might be cached upfront to optimize **quite a lot** of the currently slower aspects of evaluation.

lognaturel

It might be helpful to connect DependentExpression/DependencyContext to JR's Triggerable/Trigger

I'd find it helpful to get a little bit of a refresher on the DOM compatibility layer and where/how the browser DOM would be used in a browser context.

lognaturel · 2024-03-07T17:49:42Z

packages/odk-web-forms/src/lib/xform/model/BindDefinition.ts

+	readonly relevant: BindComputation<'relevant'>;
+	readonly required: BindComputation<'required'>;
+
+	// According to


Some weird things happened with the comment here.

This may be a bit of a user experience question. It may not necessarily be helpful for fields, especially ones off the screen, to get constraint updates at the time of a value changing. It might be easier for a user to handle constraint issues as they are working with a particular field.

If I recall correctly, this is configurable in Enketo, and I believe it was for performance reasons.

My thinking (on this, and in general) is that we start from the assumption that form state can be computed synchronously, and resolved completely, for any state change... which is a desirable default unless we have specific cause to diverge.

In terms of performance, the worst case for full-form computation of constraint is the same as it is for calculate, relevant, etc. If that becomes a particular area of optimization interest, I hope we'll still be in a position to ask "should we compute the full form at once?" holistically, regardless of which computations are potentially deferred (even if we then ask whether to treat constraint or anything else specially).

In terms of user experience, I already anticipate a distinction between:

The state of a field is invalid (constraint not satisfied, required but blank)

Invalid state is presented to the user

This is a UX consideration that goes beyond the XForms computation graph (and which a lot of forms/libraries/services do poorly, either subtly or glaringly so!). And I think you're right that it'll probably be best to progressively disclose validity issues along with user progress through a form.

This does raise some questions about the boundary of engine/client responsibility for that. I can imagine it being:

entirely a client concern

something the engine facilitates in part, such as tracking whether a given field's invalid state was [most recently] caused by client/user intervention

an even greater engine responsibility, which probably implicates other functionality we've discussed (such as tracking position and progress)

lognaturel · 2024-03-07T17:53:54Z

packages/odk-web-forms/src/lib/xform/model/BindDefinition.ts


 	// TODO: it is unclear whether this will need to be supported.
 	// https://github.com/getodk/collect/issues/3758 mentions deprecation.
-	readonly saveIncomplete: StaticBooleanBindExpression;
+	readonly saveIncomplete: BindComputation<'saveIncomplete'>;


We've decided we're keeping it in Collect for two reasons:

when launching an external app (Android-only), there's a bigger chance of something going wrong so it's helpful to be able to explicitly save a draft before that happens.

when adding a lot of repeat instances things can get slow or unstable and again, explicitly saving a draft at the end of each repeat instance can be helpful.

This would only be relevant to web forms in an offline mode with drafts. I imagine that means it would have to be exposed to the embedding application in some way. I think we can leave it aside for now.

packages/odk-web-forms/src/lib/xform/model/BindDefinition.ts

lognaturel · 2024-03-07T18:02:10Z

packages/odk-web-forms/src/lib/xform/expression/DependentExpression.ts

+
+		this.dependencyReferences = dependencyReferences;
+
+		const isTranslated = semanticDependencies.translations && isItextFunctionCalled(expression);


Should jr:choice-name also be represented?

I think this approach is fairly clean and should be easy enough to keep correct but wanted to mention that I think it would be acceptable to do something like recompute everything on language change since language change doesn't happen often. I guess that would be tracking translation-related expression adds complexity or impacts performance for regular changes.

Ah, I think it's just not implemented in the XPath evaluator yet

Yeah, we deferred jr:choice-name when we prioritized jr:itext and instance.

lognaturel · 2024-03-07T18:04:24Z

packages/odk-web-forms/fixtures/xforms/computations-demo/8-outputs.xml

          <d>10</d>
          <e />
+          <c>defualt value of c</c>


lognaturel · 2024-03-07T18:26:44Z

packages/odk-web-forms/fixtures/xforms/select/1-static-selects.xml

@@ -0,0 +1,270 @@
+<?xml version="1.0"?>


New versions of pyxform no longer generate inline selects. I don't think we should stop supporting them as they're an explicit part of the spec but want y'all to know this.

lognaturel · 2024-03-07T18:28:59Z

packages/odk-web-forms/src/lib/xform/body/control/select/SelectDefinition.ts

+
+// TODO: `<trigger>` is *almost* reasonable to support here too. The main
+// hesitation is that its single, implicit "item" does not have a distinct
+// <label>, and presumably has different UX **and translation** considerations.


Sigh, trigger in ODK XForms is the worst. It has static text that is client-defined.

lognaturel · 2024-03-07T18:31:44Z

packages/odk-web-forms/src/lib/xform/body/text/LabelDefinition.ts

+		return this.staticDefinition(form, item);
+	}
+
+	static forItemset(form: XFormDefinition, itemset: ItemsetDefinition): LabelDefinition | null {


I think this means that an output in a choice label "just works", right? It is supported and used for things like defining a select with values from a repeat (though the preferred way is to use a reference to the repeat as the itemset expression).

Tangentially related, outputs in itext need their expressions added to the dependency graph.

I believe outputs in itemset labels are in the same TODO bucket as outputs in itext translations. And I believe we'll be able to solve both the same way (leveraging the same groundwork laid in this PR).

lognaturel · 2024-03-07T18:35:35Z

packages/odk-web-forms/src/lib/xform/state/select/SelectState.ts

+	 *    - submission: A, B
+	 *
+	 * It's assumed that the above behavior is **at least** user-friendly, and
+	 * quite likely expected.


Interesting! I'm not sure whether this is more or less expected than B not being selected at step 4 (that's what JR does). I feel like I'd be a bit surprised if something that had stopped existing was back and selected. But I think I'd have to run through some realistic examples to really know. It feels like an uncommon case and as long as this doesn't add too much complexity I think it's ok.

The reasoning behind this behavior was based on trying it out on a user-filtered itemset. You can see the behavior in the dev/demo UI (yarn workspace @odk/web-forms dev):

Open the "Dynamic selects (itemset)" demo form (which is this form)

On question 2, select Alaska.

On question 1, type the letter "H". Note that Question 2's list is filtered to only show Hawaii.

Clear question 1. Note that Alaska has been restored to the list, and remains selected.

This is the behavior I would expect in a purpose-built form, at least by default without considering additional factors, because:

I took an explicit action on Question 2

I didn't take any further action on Question 2

The action I took on Question 1 was temporary and, after seeing the filtering effect, I reverted it

This is exactly the behavior we use and expect for alternating a relevance state, and around which there is fairly extensive testing (and rather painful accommodation) in Enketo.

I think it would be really upsetting, for instance, if I were filling a form with a consent guard and I:

Answer a consent question (/data/consent): yes

Proceed to fill the consent-relevant aspects of the form (/data/consented/*)

Accidentally switch /data/consent: no

Realize my mistake, reverting to /data/consent: yes

Discover that everything I had previously entered in /data/consented/* has been discarded

I think it could also be really confusing if I:

Answer a select question which activates relevance for some subset of the form (say, setting /data/foo: bar, making /data/foo-bar/* relevant)

Proceed to fill that subset of the form (/data/foo-bar/*)

Change some other answer (/data/quux) which filters /data/foo, removing "bar" as an option

Restore the previous value of /data/quux

Discover that /data/foo has been cleared, restore its answer: bar

Discover that /data/foo-bar/* has not been cleared

Granted, I think there's a TON of nuance—in the distinction between itemset filtering and relevance, as well as in the vast range of form design possibilities and how any approach to these behaviors might feel to an end user. I don't know how we'd make them all "feel right", or even if it's possible to do. I'm even open to the possibility that the (really confusing) scenario I described above is the more reasonable behavior to start with. But I wanted to make sure that at least the reasons motivating the current approach are articulated better than the offhand "user-friendly" remark in the comment.

Agree very strongly about the relevance case.

I agree they do feel somewhat related but indeed, there's some nuance based on usage. My experience is that people use the dedicated autocomplete/search appearance (they're aliases) for filtering like you describe.

I don't feel too strongly about this one but I do feel like the case below is important to address. Maybe an issue to file?

lognaturel · 2024-03-07T18:36:50Z

packages/odk-web-forms/src/lib/xform/state/select/SelectState.ts

+	 *
+	 * It is also assumed, but not yet implemented, that any user initiated
+	 * selection change should discard `selected` values which have been
+	 * subsequently filterered from `items`, i.e. in lieu of the above step 4:


Yes, I agree with this. Feels like it might be simpler to understand if, when a choice stops being visible, it's always unselected?

Moved to #57, as we discussed on Slack.

lognaturel

Couple small things to fix but after interactive review, we're aligned and ready to merge!

lognaturel · 2024-03-08T01:58:05Z

packages/odk-web-forms/src/lib/xform/XFormDefinition.ts

+ * {@link https://www.w3.org/TR/xforms11/#rpm-processing-recalc-mddg | algorithm as described in XForms 1.0},
+ * in that:
+ *
+ * - it performs a single sort up-front upon parsing the form; handling of


I think that’s what the spec says? Enketo computes it over time in various places but my read of the spec is its intended to be done once.

The spec discusses applying the DFS algorithm after a change, to identify and order the subgraph dependencies affected by that change. The intent of this part of the comment is to call out that this logic is not used for that purpose (although it’s roughly the same algorithm, notwithstanding counting vertices), and instead defers to the reactive subscription graph to track dependencies on change.

I see! I had to reread the section starting with recalculate.

I think there’s also a hint there about why constraints are not in the graph: recomputation stops at the first constraint violation.

This was referenced Feb 22, 2024

Initial read interface design between engine and UI #45

Closed

Initial support for itext translations #13

Merged

eyelidlessness added 4 commits February 28, 2024 09:31

First pass on selects (select/select1, item/itemset)

1bdedbd

eyelidlessness force-pushed the features/items branch from 91a659a to 1bdedbd Compare February 28, 2024 17:34

lognaturel reviewed Mar 7, 2024

View reviewed changes

lognaturel approved these changes Mar 7, 2024

View reviewed changes

eyelidlessness mentioned this pull request Mar 8, 2024

What is the expected behavior when a selected select value is temporarily removed? #57

Open

eyelidlessness added 2 commits March 7, 2024 16:34

Fix typo in outputs fixture

11cfa09

Notes on toposort, relative to algorithm described in XForms spec

52b2205

lognaturel approved these changes Mar 8, 2024

View reviewed changes

eyelidlessness mentioned this pull request Mar 8, 2024

Briefly address FAQ in top level readme #28

Closed

13 tasks

eyelidlessness merged commit 60c902f into main Mar 8, 2024
42 checks passed

eyelidlessness deleted the features/items branch March 8, 2024 18:11

eyelidlessness mentioned this pull request Apr 1, 2024

Refactor: engine <-> client interface implementation #67

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial selects support: (select, select1, item, itemset) #14

Initial selects support: (select, select1, item, itemset) #14

eyelidlessness commented Dec 7, 2023 •

edited

Loading

lognaturel left a comment

lognaturel Mar 7, 2024

eyelidlessness Mar 8, 2024

lognaturel Mar 7, 2024

lognaturel Mar 7, 2024

lognaturel Mar 7, 2024

eyelidlessness Mar 8, 2024

lognaturel Mar 7, 2024

lognaturel Mar 7, 2024

lognaturel Mar 7, 2024

lognaturel Mar 7, 2024

lognaturel Mar 7, 2024

eyelidlessness Mar 8, 2024

lognaturel Mar 7, 2024

eyelidlessness Mar 7, 2024

lognaturel Mar 7, 2024

lognaturel Mar 7, 2024

eyelidlessness Mar 8, 2024

lognaturel left a comment

lognaturel Mar 8, 2024

eyelidlessness Mar 8, 2024

lognaturel Mar 8, 2024


		this.dependencyReferences = dependencyReferences;

		const isTranslated = semanticDependencies.translations && isItextFunctionCalled(expression);

Initial selects support: (select, select1, item, itemset) #14

Initial selects support: (select, select1, item, itemset) #14

Conversation

eyelidlessness commented Dec 7, 2023 • edited Loading

Expression/dependency generalization

instance() XPath support

Selects proper

What's missing

lognaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyelidlessness commented Dec 7, 2023 •

edited

Loading

`instance()` XPath support