Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Numeric controls: decimal, int, appearance=numbers #261

Merged
merged 33 commits into from
Dec 13, 2024

Conversation

eyelidlessness
Copy link
Member

@eyelidlessness eyelidlessness commented Dec 10, 2024

Closes #251.
Closes #252.
Closes #260.

I have verified this PR works in these browsers (latest versions):

  • Chrome
  • Firefox
  • Safari (macOS) See "Notable limitations/quirky behaviors"
  • Safari (iOS) See "Notable limitations/quirky behaviors"
  • [ ] Not applicable

What else has been done to verify that this works as intended?

Expanded test coverage, largely around bind types. Lots of manual testing of weird edge cases and nuances in the UI.

Why is this the best possible solution? Were any other approaches considered?

I considered handling the UI and engine portions of this separately. I think that would have risked splitting responsibilities for integrity of per-type values in ways that could bite us longer term. This also sets us up for much more rapid development of the remaining data types (which that shorter term approach would not have done).

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

I think the greatest areas of risk are anywhere that new branchy logic should have more nuance than I've accounted for. But I expect if that's the case, those nuances are lurking in other pending feature work.

Do we need any specific form for testing your changes? If so, please attach one.

A new fixture is added, which I used for testing in the UI.

What's changed

Engine: <bind type> parsing

  • <bind type> is now parsed into a BindTypeDefinition with:

    • resolved: the effective type determined for the node's binding
    • source: the literal value of the type attribute, if any

    It may not be valuable to preserve source longer term, but it was useful during development, especially around some existing or newly introduced nuances...

  • Namespaced types specified in the http://www.w3.org/2001/XMLSchema namespace are resolved to their LocalPart. Resolution applies the following logic (applicable for namespace declarations on each respective <bind> element, or any of its ancestors):

    • _If the XML Schema namespace URI is associated with a namespace declaration: normal XML namespace resolution applies. I believe this is completely consistent with all applicable specs. By convention, the namespace is typically declared for the xsd prefix.

    • Iff the XML Schema namespace URI is NOT associated with any namespace declaration: The xsd prefix is used as a default. This exceeds spec. I went with this because there are some form fixtures from JavaRosa which would be affected, and at least at a glance this appears to be the intent/expected behavior for those fixtures.

    • If any other URI is declared for the xsd prefix: the xsd default is not applied.

  • integer is resolved as an alias to int. We may decide to revert that, depending on this corresponding JavaRosa change. I left it in since it was in flight, and would allow us to discuss both in tandem.

    My take: If we are confident that we're the only ones using the alias, removing it in both projects makes sense! If we believe users may be using it, I'd like to make sure we have a plan for communicating the breaking change to them.

  • Other found aliases are implicitly resolved to string (implicitly because it is the default type). Those I've found in existing fixtures:

    • select1
    • odk:rank
    • rank (unprefixed)

Engine/client interface: ModelValueNode is generic over additive valueType property

Note: This change isn't strictly required for the features in this PR. I chose to do this as a testbed for the approach, which I then expanded to input controls. I used this testbed to evaluate both the notion of generic value types overall, and the impact on client usability.

This introduces the following (technically breaking) changes to ModelValueNode:

  • A valueType property is added, associated with a new client-exported union type ValueType (effectively the same union of <bind type>s).

  • The ModelValueNode interface has a ValueType type parameter ("generic"), i.e. ModelValueNode<V extends ValueType>

  • Each ValueType is associated with a representation of its runtime value, and the type for that runtime value is then associated with ModelValueNode<V>.currentState.value. In this PR, those representations are as follows:

    • ModelValueNode<'string'>.currentState.value: string
    • ModelValueNode<'int'>.currentState.value: bigint | null
    • ModelValueNode<'decimal'>.currentState.value: number | null
    • ModelValueNode<AnyOtherValueType>.currentState.value: string (as a default, until we immplement those value types)
  • The union of node types we currently call GeneralChildNode (which a client iterates over for the children of a corresponding GeneralParentNode) is expanded to include the union of all ModelValueNode<V>s. An alias to that sub-union is also exported as AnyModelValueNode (which is generally how clients will consume these now).

  • Each ModelValueNode also exposes its current value as a string, regardless of its valueType:

    • AnyModelValueNode.currentState.instanceValue: string

    This is mostly an affordance for scenario, which has many tests exercising the values of model-only nodes. The same state property will be exposed for any other "value node" as we expand the approach. In terms of spec semantics, this is effectively the node's "XPath value", and its pre-serialized "submission value" (i.e. it is NOT escaped for XML).

Engine/client interface: StringNode -> InputNode<V extends ValueType>

The StringNode client interface has now been renamed InputNode to reflect the fact that it is no longer a string-only node, and to better reflect the fact that it is strictly associated with <input> controls in the form definition.

All of the notes for ModelValueNode apply. Additionally, the setValue method's value parameter is now also generic over valueType:

  • InputNode<'string'>.setValue: (value: string) => RootNode
  • InputNode<'int'>.setValue: (value: bigint | number | string | null) => RootNode
  • InputNode<'decimal'>.setValue: (value: bigint | number | string | null) => RootNode

Note: both of the numeric types accept a superset of their runtime value type for writes. This is entirely an accommodation for ergonomics in client integrations (which paid off a bit!).

As with ModelValueNode, the engine exports all of the InputNode<V> sub-union as part of the GeneralChildNode union. And the engine exports a catch all sub-union as AnyInputNode.

Unlike ModelValueNode (where clients are generally not doing more than accessing the nodes values, if that), it also made sense to provide named representations of the newly introduced input nodes and their value types. So the engine also now exports:

  • StringInputNode
  • IntInputNode
  • DecimalInputNode

Lastly, the engine exports another catch all for the remaining value types which fall back to a string value representation pending implementation of those value types. This is exported as TemporaryStringValueInputNode, hopefully providing some clarity that this is not a stable type we expect to keep around forever.

Engine-internal: ValueNode<V extends ValueType> and its subclasses

Internally, the engine now has a new abstract ValueNode base class, which handles nearly all of the responsibilities of each affected value node type. The following have been updated to be subclasses of it:

  • ModelValue (implementation of ModelValueNode)
  • StringField -> InputControl (implementation of InputNode)

I anticipate moving nearly all of the other value node implementations to be subclasses of ValueNode, with the possible exception of selects, and the pending implementation of odk:rank. (Currently the generalization does not account for select/rank options.)

This base class largely takes over responsibilities for existing implementation details common to value nodes:

  • Initializing the node's value state as a primary instance value:

    • Creation of a reactive signal to store the value
    • Deriving the initial value (currently from the form definition; from edited submission still anticipated/pending)
    • Ensuring non-relevant values are treated as blank
    • Initializing reactive calculate computations
  • Mapping that primary instance value to/from a runtime representation (more on this in the next section on ValueCodec)

  • Integration with XPath (most pertinent now being reactive access to its primary instance state)

  • Initialization of node validation (constraint, required)

Engine-internal value representation: ValueCodec<V extends ValueType>

Each ValueNode expects a new engine-internal implementation of a ValueCodec which is:

  • Implemented for that node's valueType
  • Implementing decodeValue, to decode the node's primary instance value state into its runtime value representation
  • Implementing encodeValue, to encode the node's runtime input value representation to its primary instance value

This basically breaks apart the same responsibilities from an earlier engine-internal interface (ValueContext). It's what allows value nodes to be generic over their value type independent of other value node responsibilities.

This abstraction is also designed so that different node types may have different runtime representations of the same value types. The intent here is that we can use the same ValueNode and ValueCodec to serve node type-specific nuances. A couple examples which are top of mind:

  • Note (implementation of NoteNode): it will probably best serve clients if a note's value representation is always string | null regardless of its valueType. (And at least for some valueTypes, that string representation will probably differ from its instanceValue!)

  • TriggerControl (implementation of TriggerNode): its runtime value representation will likely continue to be boolean, but its encoded primary instance value will be serialized differently from a 'boolean' model/input node: '' | 'OK' (versus '0' | '1').

Numeric input UI

The existing InputText component has now been broken up into:

  • InputControl: a wrapper for typed inputs, handling responsibilities common to all of them (e.g. label, hint, required status, validation)

  • InputNumeric: a composable base component for each numeric input type, handling various nuances of integration with PrimeVue's InputNumber

  • InputDecimal composing InputNumeric with props specific to the decimal value type

  • InputInt composing InputNumeric with props specific to the int value type. This includes min/max bounds which differ from Collect (but match what I understand to be the intent of collect's bounds)

  • InputNumbersAppearance composing InputNumeric (for type="string" if specified + <input appearance="numbers">). The behavior is roughly a superset of InputDecimalandInputInt: decimal values are allowed, and the min/max` bounds are relaxed.

  • InputText: just the control portion, as extracted from the component of the same name on main

For all numeric inputs, the thousands-sep appearance determines whether to use "grouping" on the PrimeVue base component. I assume there will be some localization-specific nuances to address as well. For now I deferred to PrimeVue's defaults for that.

Notable limitations/quirky behaviors

I've made a reasonable effort toward reasonable behavior for these inputs. There are some caveats worth noting.

First, some which are browser-specific:

  • Safari, iOS: cannot enter negative or decimal (fixed) values, apparently this is optional. I believe this can be addressed by setting inputmode="text" type="number" and then adding type-specific pattern attributes. The downside is that a bunch of non-numeric keys are present on this keyboard variant (for reasons I still don't understand). I'd like to make this change, but would be happy to put it off to discuss nuances/look for better alternatives. Just kidding! This breaks the increment/decrement buttons (causing a runtime error on click/tap), also for reasons I don't understand! As far as I can tell, the best we can do with PrimeVue's InputNumber component is inputmode="text", which will use the default software keyboard. Otherwise, negative number input on iOS is not possible.

  • Safari, macOS: numeric inputs swallow all keyboard commands (at least anything with cmd key that I tested)

  • All tested desktop browsers: numeric inputs swallow cut keyboard commands (i.e. cmd x); cut by other means (e.g. menu, touch popup) does still work

The PrimeVue component also has a variety of general quirks, which I've tried to minimize as much as possible. Worth noting here:

  • By default, the DOM and reactive state can easily get out of sync. This is true for any keyboard input that doesn't trigger increment/decrement behavior (i.e. typing a value!). Unlike PrimeVue's base InputText (which is the base component for its InputNumber!), state changes are only applied on change (which is only dispatched once focus leaves the control). This is exacerbated by enforcing bounds (either with the min/max props, or the engine's slightly more permissive bounds).

    This is the reason for the very weird (IMO) use of customRef. By using state and focusing entirely on numeric values to address these issues, we avoid having to deal with parsing and re-formatting the input's DOM text value, which could add quite a bit of complexity! One important downside to this worth noting is that every time we override the value state it causes the user's cursor to jump to the end of the input. In my testing this is usually close enough to what I'd expect, but it might be jarring in some cases.

  • The base presentation in the theme we currently use is terrible. Here I've opted for something more subtle/closer to a common native control.

Setting the stage for pending feature work

The design of engine internals in this PR is positioned to expedite implementation of the remaining data types, and to allow most of that work to focus on those types' UI/UX concerns. In particular, the ValueNode and ValueCodec abstractions are designed so that we can rapidly add support for encoding/decoding each data type, without making further substantive changes to any other aspect of the affected nodes (apart from assigning them named exports as appropriate).

Copy link

changeset-bot bot commented Dec 10, 2024

🦋 Changeset detected

Latest commit: 91c6617

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

This PR includes changesets to release 3 packages
Name Type
@getodk/web-forms Minor
@getodk/xforms-engine Minor
@getodk/ui-solid Patch

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 eyelidlessness force-pushed the features/decimal-int-controls branch from 6ae5bfe to 5fecfb8 Compare December 10, 2024 17:36
@eyelidlessness eyelidlessness force-pushed the features/decimal-int-controls branch 2 times, most recently from 1762289 to c0e7a7b Compare December 12, 2024 18:00
This interface is not intended to be consumed directly by clients. Instead, it narrows (and slightly expands) the `BaseNode` definition with types common to all leaf (i.e. value) node types.

This interface will be used to establish a corresponding abstract base class implementation, which **most** value node implementations can extend. The interface (and the corresponding abstract base implementation) will also be expanded in subsequent commits.

Note that, at least for now, this interface is intended to become a basis for nodes whose values are not chosen from a set of options (i.e. excluding nodes for `<select>`, `<select1>` and `<odk:rank>` controls). Currently, this applies to nodes with the following types:

- model-value
- string (to be updated in this change, as described below)
- note (to be updated at our convenience)
- trigger (to be updated at our convenience)
- range (to be updated when we support this control type)
- upload (to be updated when we support this control type)

Note that the naming of the `string` node type (and its corresponding `StringNode`, `StringField`) is not consistent with this anticipated data type parameterization. Those will likely be renamed to reflect their relationship to the XForms `<input>` control.

Also note that several of the other node types are associated with a subset of the available XForms data types.
…value types

This effectively breaks out the `encodeValue` and `decodeValue` concepts which are currently defined by the `ValueContext` engine-internal interface. It will allow value node implementations to be generic over their `ValueType`, where a `ValueCodec` implementation is responsible for:

- mapping its instance state value (always a string, as the engine’s representation of the node’s value for XPath and XML-serialization purposes) to a `RuntimeValue`

- for value nodes with write semantics, mapping a corresponding `RuntimeInputValue` back to that node’s instance state value representation

- - -

Note that `ValueCodec` is unopinionated about what a given `ValueType`’s representation of `RuntimeValue` and `RuntimeInputValue` actually are. This is intentional, acknowledging that some nodes will have different value representations for the same `ValueType`. For some nodes this is by design (e.g. note nodes will likely always have a string representation for all value types). For node types which are also pending implementation, this intentionally also leaves design of their respective value representations unaddressed.
…tation

As of this commit, all value types will still be treated as strings. This shared representation will be integrated into at least one affected node implementation, where we can then implement and validate support for specific value types.
The following are now generic over their value type:

- `ModelValueNode` (client interface)
- `ModelValueDefinition` (shared client/engine sub-interface)
- `LeafNodeDefinition` (base interface)
- `ModelValue` (implemenation of `ModelValueNode`)

This node type was chosen as the first to exercise the new generic value type concepts as it will cause the most minimal change for clients. As of this change, it should require no client changes at all. Some minimal change is incoming as value codecs are implemented for certain value types.

- - -

As described in the commit introducing `ValueCodec`, this continues to break apart concepts already present in `ValueContext` and its usage.

- `InstanceValueContext` is `ValueContext` without the codec responsibilities
- `createInstanceValueState` isolates the core instance state logic from any runtime representation as seen in `createValueState`, where those core concerns are:
  - storage of instance values (ultimate source of truth)
  - reactive updates to treat those values as blank when the node is non-relevant
  - computation and reactive recomputation of `calculate` expressions for affected nodes
- `ClientReactiveSubmittableValueNode` is ClientReactiveSubmittableLeafNode` without value encoding responsibilities
- `createValueNodeSubmissionState` is `createLeafNodeSubmissionState`, adapted to consume the node’s `instanceValue` state, which is always a string, rather than consuming and encoding the node’s runtime value state

- - -

In theory, this should also have some marginal performance benefits for nodes with non-string value types, as it avoids several places we would redundantly perform decode/encode round trips.
Note that since this is the first value type variant of `ModelValueNode` with a non-string runtime value, this also introduces an expanded `AnyModelValueNode` union type. This type allows clients to treat any node with `nodeType: ‘model-value’` as a discriminated union over their `valueType` property. (This isn’t particularly useful in its own right, but it should be a hint of what’s to come for the treatment of value types for nodes associated with `<input>` controls.)

Updated client integration will follow in subsequent commits.
- Type parameter is updated to reflect that model value nodes from the engine are now a union over their value type (as `AnyModelValueNode`)
- Reference to those nodes’ string value now go through the dedicated string-only `instanceValue` state property
@eyelidlessness eyelidlessness force-pushed the features/decimal-int-controls branch 2 times, most recently from 5902bb7 to 4fc57fb Compare December 13, 2024 16:57
@eyelidlessness eyelidlessness changed the title (WIP) Decimal/Int controls Numeric controls: decimal, int, appearance=numbers Dec 13, 2024
@eyelidlessness eyelidlessness marked this pull request as ready for review December 13, 2024 20:10
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Firefox 132.0.2 (aarch64), macOS, every browser and device I've tried the string with numbers appearance turns all characters after the 17th digit into 0s.

I made an inline node about the bounds for decimal. For ints, Collect limits the entry to 9 characters, including the -.

I think all of these could be addressed in follow-up work.

let displayMessage: string | null = null;

const unexpectedNodeErrorMessage = (node: UnexpectedNode): string => {
return `Expected mode-only node. Got node with type: ${node.nodeType}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

model-only

});
});
it('casts a calculated value to an int', async () => {
// const scenario = await Scenario.init(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this one commented out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should've been deleted! 🙃 It was one test which became the basis for the three above.

if (props.isDecimal) {
fractionalDigits = {
min: 0,
max: 18,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this is minimum conformance defined by XML. Collect caps at 15 total characters (including the decimal). Let's track this difference for possible follow-up work.

- `QuestionList` now uses an explicit check for control `nodeType`s in order to render them
- The added `ExpectModelNode` component functions sort of like an exhaustiveness check, to ensure that our explicit check _as type guard_ matches the actual set of `nodeType`s received from the engine, producing an error if any non-model node is unhandled. (The error is currently displayed inline in dev mode, and logged with `console.error` in builds)

This doesn’t necessarily need to be quite so involved, but it’ll be much more resilient to any future breaking changes to the union of node types the engine produces.
…ntation)

This is committed here to demonstrate that since `ModelValueNode` and `ModelValue` are now parameterized (type and implementation) to use the shared set of value type codecs, implementing and referencing a decimal codec will be sufficient to update the node’s value types and runtime state representation.
Note: as described in prior commit, types and runtime representation are updated for the affected node without needing to make a direct change to `ModelValueNode` (the type-parameterized client interface) or `ModelValue` (the codec-parameterized implementation of that interface).
This is preparation for renaming `StringNode` -> `InputNode` to reflect it becoming generic over its value type (as `ModelValueNode` is now).
This reflects the fact that `InputNode` will be generic over its value type (as `ModelValueNode` is now).

- Downstream packages will be updated in subsequent commits. (Project is in a temporarily broken state as of this commit.)
- Engine-internal implementation of `StringNode` will be similarly renamed as it is made generic over its value type
This was done separately from the previous commit, in hopes the file rename in that commit will be preserved in git history.
The following are now generic over their value type:

- `InputNode` (client interface)
- `InputDefinition` (shared client/engine sub-interface)
- `InputControl` (implemenation of `InputNode`, replacing `StringField`)

This change largely follows the same work for `ModelValueNode`, with the following exceptions:

- Unlike `ModelValueNode`, we expose explicit named types for the typed input nodes (`StringInputNode`, `IntInputNode`, `DecimalInputNode`). It's expected that these will be more ergonomic for clients to use, as at least the UI client will branch behavior depending on the node's `valueType`.

-  We likewise expose an explicit named type for the current value types pending implementation (`TemporaryStringValueInputNode`). This is intentionally distinct from `StringInputNode` as the subset of `valueType`s will shrink as we implement those value types, but `StringInputNode` is expected to be (comparatively) stable.

- `InputNode` exposes a `setValue` method, which is also now generic over the node's `valueType`.

Note that on this last point, support for this is deferred entirely to the node's parameterized `ValueCodec` and its associated input types.

- - -

Note that there are some edge cases to fix in IntValueCodec, and likely in DecimalValueCodec as well. (Probably should unit test those directly! But it does actually seem reasonable to test at this boundary or even at the `scenario` integration level too.)

- - -

As with the corresponding change for `ModelValueNode`, clients will be updated in subsequent commits.
This commit does not incorporate any logic specific to `InputNode.valueType`, that will be handled in a subsequent commit (i.e. the actual _feature_ of supporting `int` and `decimal` node types!)
Some liberties were taken around visual styling.
…swer`

This helps to ensure accurate results from tests exercising casting logic.
Fixing some int casting edge cases revealed in CI that a previously ported JavaRosa test now passes. We’d already intended to derive more tests more directly exercising the affected logic, so those are added here.

As the setup and test logic very closely mirrors that of the previously added value-types unit tests, it felt more reasonable to move those tests into `scenario` as well.

Note: some of the expanded int casting tests are failing as of this commit. A fix is incoming in the next commit.
Note: this negates some of the optimizations anticipated by breaking up the logic of `createValueState`, but still leaves open the possibility of more targeted optimization.
Discussed with @lognaturel yesterday, we do plan to eliminate this package rather than continue updating it. I felt removing it on this branch/in this PR could be confusing, so we decided to remove it from CI first. We can remove the package entirely in a followup PR.
… types

This applies the same clamping logic whether:

- `min`/`max` are controlled by props (as with `InputInt`) or by the engine
- the input value is changed by increment/decrement, direct keyboard entry, paste

Note: This does not handle cut events, which PrimeVue’s component seems to swalow. No clue why that is!
This pretty much settles the matter in my mind: unless PrimeVue 4’s equivalent component is vastly better, we should look into other options. This should be either the default behavior, or the default when anything decimal is implicated.
@eyelidlessness eyelidlessness force-pushed the features/decimal-int-controls branch from 4cdc794 to 91c6617 Compare December 13, 2024 23:06
@eyelidlessness
Copy link
Member Author

In Firefox 132.0.2 (aarch64), macOS, the string with numbers appearance turns all characters after the 17th digit into 0s.

I'm pretty sure this is because the component's value is represented as a number. A 17-digit integer exceeds Number.MAX_SAFE_INTEGER.

@eyelidlessness eyelidlessness merged commit d59094f into main Dec 13, 2024
74 checks passed
@lognaturel
Copy link
Member

I'm pretty sure this is because the component's value is represented as a number.

Ah, got it. The intent is for it to be represented as a string throughout so the only restriction on it is that the input is limited to digits, a single optional decimal marker and a single optional prefix -.

eyelidlessness added a commit that referenced this pull request Dec 17, 2024
This package has outlived its usefulness. We removed it from CI in #261, anticipating full removal of the package in a more appropriately scoped PR.

Note: the `yarn.lock` change is a result of running `yarn install` immediately after all of the other changes in this commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support string type with number appearance Support decimal inputs Support integer inputs
2 participants