Initial support for appearances, body classes #127
Conversation
eyelidlessness
force-pushed
the
features/appearances-init
branch
from
2807d7e
to
b0ec3d4
Compare
Note that this introduces an in-memory `<label form-definition-source=“repeat-group”>` to distinguish outer/inner labels for such structures.
Example: for this form definition structure…
```xml
<group ref="/root/rep">
<label>Repeat/group label</label>
<repeat nodeset="/root/rep">
<label>Repeat label</label>
</repeat>
</group>
```
… this would be the normalized structure:
```xml
<repeat nodeset="/root/rep">
<label form-definition-source=“repeat-group”>Repeat/group label</label>
<label>Repeat label</label>
</repeat>
```
Note that `RepeatGroupDefinition` previously had two responsibilities: 1. To provide access to its `repeat` 2. To provide access to a label defined in the repeat’s containing group The first is no longer necessary because the repeat is accessed directly. The second is accommodated by defining the `RepeatElementDefinition`’s label from the special-case `<label form-definition-source=“repeat-group”>` as produced in the previous commit. Also note that this still does not deal with the unhandled repeat labeling engine responsibility: <repeat><group><label/></group></repeat>. A TODO is added specifically so it can be traced back to this commit, as it will likely help shorten the path to reabsorbing the pertinent code/implementation details back into active brain memory.
Automated testing is pending. Some manual validation has been done to verify that this likely works as intended. The intent is to make this first pass available for client iteration as quickly as possible. While it’s possible to include unit tests for `TokenListParser`, it seems more likely we’ll want to add integration tests in the `scenario` package. Given there’s a ton of unaddressed feedback in #110, it seems most prudent to get this into draft first, and bring in integration tests once that lands.
eyelidlessness
force-pushed
the
features/appearances-init
branch
from
b0ec3d4
to
fc776a9
Compare
eyelidlessness
changed the title
|
Somehow I forgot to mention in the initial writeup that this also adds support for body classes. The functionality is so conceptually similar that it made sense to me to generalize it and tackle both together. It feels a bit awkward at the |
Yes, that sounds right to me.
I think starting with an error is a good idea because it will allow us to know whether it actually happens. As far as I know, the only appearance that can apply to groups or repeats is
I never registered that |
|
Re |
👍
The other case I found is The design is such that anything defined will be passed through, so anything that Enketo supports will be present, it just won't necessarily be explicit in the type-driven suggestions.
Enketo uses body |
Right, that's perfect.
Ah, right, of course. |
|
One thing that just came to mind is that labels cannot be directly nested in repeats according to the spec: https://getodk.github.io/xforms-spec/#repeats That’s why repeats are generally nested in a group bound to the same node. I think this just means that some of the work in the first commit won’t be used in practice. Seems ok to leave in for now, though. |
| }; | ||
|
|
||
| const normalizeRepeatGroupLabel = (group: Element, repeat: Element): void => { | ||
| const groupLabel = Array.from(group.children).find((child) => { |
There was a problem hiding this comment.
❓ should we throw an error if there are more than one label nodes?
There was a problem hiding this comment.
Probably warn, and probably everywhere we look for 0-1 nodes (all label lookups, hints also come to mind). My understanding of the spec is that only one is “properly supported”, which to me strongly implies ignoring >1.
Feels like a good issue to file, and/or category of things to add to a general discussion of how we’ll convey errors/warnings/notices about form definitions to clients.
| 'quickcompact', | ||
| 'map', | ||
| // "quick map" | ||
| ], |
There was a problem hiding this comment.
❓I am assuming that appearance (W1, W2... Wn) related to theme-grid of Enketo will be handled separately later on.
There was a problem hiding this comment.
I mean, they’re handled here already. They’ll be passed through if present. They’re not included in the types, but they can added be if/whenever we want to make them first class.
| 'label', | ||
| 'list-nolabel', | ||
| 'minimal', | ||
|
|
There was a problem hiding this comment.
I see following appearances from XLSForm template missing here:
- no-buttons
- list
- image-map
There was a problem hiding this comment.
Ah yes, compact is deprecated. It was split up into horizontal-compact and no-buttons which can be combined if desired.
There was a problem hiding this comment.
If there are any known/canonical appearances (or aliases to them) that we want to add right away, please feel free to add them in! My hope is that this approach makes that trivial. I’d be happy to add these when my power is back, but you’re welcome to add them at your convenience as well.
Ah I thought this was more common than it is. Turns out the only case I could find in Enketo was one I added myself 🙃, presumably when I was less familiar with these details of the spec. |
lognaturel
force-pushed
the
features/appearances-init
branch
from
e3649cf
to
1ac6b10
Compare
|
Have added changeset (that was fun), over to you @sadiqkhoja to either merge or align select appearances, up to you. 😊 |
Closes #56.
This is primarily intended to introduce feature support for
appearance, from parse through to the engine's client interface, represented by anappearancesproperty on each node. Nodes which do not support appearances are assignedappearances: nullfor object shape consistency (as is typically the case across node types, with some exceptions forRootNode; more on that below).Nodes that do support appearances currently include:
GroupNodeRepeatInstanceNode*RepeatRangeNode*SelectNodeStringNode***: Presently both repeat node types share the same appearances definition. There are some JSDoc comments and commit notes discussing the details of this.
**: A
StringNode's appearance will be present if the node'sdefinitionhas a non-nullbodyElement(presently anInputDefinition). This may be less confusing if we pursue additional node types, as discussed in #73 (i.e. we could better distinguish between a "string node" that's associated with an<input>, versus a "string node" that's only associated with a model node and/or<bind>).The value for each node's
appearancesare produced by a node-specificTokenListParserinto aTokenList1 with roughly the same behavior:For each of that node's appearances (as defined as a whitespace separated list in an
appearanceattribute, on that node's body element),node.appearances.$appearanceNamewill producetrue(and will producefalsefor any*** appearance not present on the element in the form definition). For clients which are concerned with specific appearances (i.e. branching on those specific appearances to produce differences in rendering, presentation, interactive behavior), this is likely to be the most convenient way to access a node's appearances.*** Excepting aliases, see the last bullet point in this list.
The
appearancesobject is also anIterable, yielding each such appearance from its definition in the form (e.g.for (const appearance of node.appearances) { ... }). It's likely this will have more utility for testing purposes.There is some variance in the
appearancestypes for each respective node type. These are primarily intended to provide additional developer convenience in clients. For each underlying body element type, there are a set of "canonical" (known, first class, explicitly documented, and so on). These are specified as part of body parsing logic, and those appearances are promoted in the nodeappearancestype to provide additional editor support (e.g. autosuggest on dot access and/or when referencing an iterated appearance).All form-defined appearances are passed through in the runtime value, even if they are not declared as "canonical". These are not included in type suggestions, because they're not known upfront. But if present in the form definition, non-canonical/unknown appearances will also produce
trueon dot access and will be yielded on iteration.Node specific aliases: mostly included as a proof of concept (for now), each node type's
TokenListParsermay specify analiasesoption, mapping any particular appearance to another "canonical" appearance. The behavior of aliases is additive: if a form definition specifies an appearance which is aliased to a "canonical" appearance, both the specified appearance and its corresponding "canonical" appearance will producetrueon dot access, and both will be yielded on iteration (with the "canonical" appearance yielded first). This is currently only used to map a single select appearance (search->autocomplete), but is intended to demonstrate the generalization. Ideally we will expand on these so that the engine can act as a source of truth for all known/supported appearance types, without pushing that responsibility separately onto every client.Minor tech debt cleanup, related open question
Elimination of "repeat group"/"repeat" pairing in the parsed body representation
There has been some lingering awkwardness in the form parsing logic around the concept of "repeat groups". Earlier development introduced a pre-parse, in-memory normalization to ensure that every
<repeat nodeset>element is wrapped in a corresponding<group ref>(where thenodeset/refare the same. This is the most common production from XLSForms as produced by pyxform. The intent was to simplify the parsed data model by producing a consistent structure for repeats regardless of whether the form itself specifies a<group ref><repeat nodeset>pair. Even at the time, it was noted that the resulting data model was somewhat awkward, with the parsed representation of a repeat's body always including a corresponding pair of objects for what is ultimately treated as a singular thing.Starting on the work to support appearances, this awkwardness resurfaced. I had also recognized that the awkwardness overlaps with other coming feature work (support for
jr:countandjr:noAddRemovecome to mind, and from recent prototyping on those I anticipate this change will bring some meaningful simplicity).Rename remaining references to repeat "sequence" to "range"
Before #67, the concept of a set of zero-or-more contiguous repeat instances was called a "sequence". In that PR, we settled on the name "range" (
RepeatRange&co). Since I was already doing some minor refactoring on the above "repeat group" concept, it felt appropriate to tackle this lingering naming inconsistency. So all references to the same "bag or vat of repeat instances" are now consistently referenced with the "range" terminology.Open question regarding repeat appearances
As I understand the spec, appearances may be specified only on form controls and groups. In practice, I have noted many form fixtures (particularly in Enketo) define appearances directly on
<repeat>elements. With the aforementioned change to normalize "repeat groups" into a singular concept, both of these would be treated as interchangeable:My instinct is that this is the most likely expectation, although that can also be framed as a question here. So: is that what users would most likely expect?
Perhaps more ambiguous in intent: what would users expect if both nodes specify appearances? I.e.
This is presently treated as an error. It's conceivable that:
<label>(this is currently a presentational difference in theweb-formsVue UI client)Landing sequence
I'm opening in earnest, as I understand it will unblock quite a bit of UI client work. I'm opening it as draft, because I would like to land these other PRs first:
Scenariotest suites #110 - so we can also add integration tests for this functionalityFootnotes
As discussed in more detail in the JSDoc for
TokenListParser/TokenList, these names are a reference to a web standardDOMTokenList, as it is very similar conceptually. But I'm totally open to bikeshedding these names if they don't feel right. ↩