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.

Sign up for GitHub

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

Jump to bottom

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

Open
eyelidlessness opened this issue Mar 8, 2024 · 0 comments
Open

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

eyelidlessness opened this issue Mar 8, 2024 · 0 comments
Labels
question Further information is requested

Comments

@eyelidlessness
Copy link
Member

Breaking out an issue to track this discussion thread on #14.

Current (albeit incomplete) behavior

The intent of the behavior in #14 (which is incomplete, as called out in the comment) is this:

Given a form like:

<instance>
  <data>
    <sel />
    <fil />
  </data>
</instance>
<instance id="sel-opts">
  <root>
    <opt><val>option a</val></opt>
    <opt><val>option b</val></opt>
  </root>
</instance>
<!-- ... -->
<select ref="/data/sel">
  <itemset nodeset="instance('sel-opts')/root/opt[contains(val, /data/fil)]">
    <value ref="val" />
  </itemset>
</select>
<input ref="/data/fil" />

If a user...

  1. Answers /data/sel, selecting "option a"
  2. Answers /data/fil, entering "b" (causing "option a" to be removed from /data/sel's available items)

Then the form state is updated so that the prior selection of "option a" is:

  • Cleared from /data/sel
  • Retained in memory until1 another selection is performed with the filtered options for /data/sel

At this point, the form's instance state is correct, but it allows either:

Next (A), if a user...

  1. Answers /data/sel again, selecting "option b"

    Then the form state is updated so that:

    • /data/sel is set to "option b"
    • The previous selection of "option a" will1 no longer be retained

  2. Clears /data/fil (restoring "option a" as an available option for /data/sel)

    Then the form state for /data/sel is unchanged from step 3

... OR ...

Next (B), if a user...

  1. Clears /data/fil (restoring "option a" as an available option for /data/sel) without any further user action on /data/sel following step 1's affirmative selection of "option a"

    Then the form state is restored to its state after step 1 (i.e. the value of /data/sel is again set to "option a")

Alternative behavior

Per the discussion thread, the alternative would be not to retain any memory that "option a" had been chosen (step 1), once that option is filtered out (step 2). Also per the discussion thread, this is the behavior in JavaRosa. Furthermore, it would be fair to say it is the obvious default behavior. It is unquestionably the behavior we'd implement if we skip asking the question posed by this issue.

Footnotes

  1. This aspect is currently unhandled, but will be handled the same way regardless of the broader answer to the question posed by this issue. ↩ ↩2
@eyelidlessness eyelidlessness added the question Further information is requested label Mar 8, 2024
@github-project-automation github-project-automation bot moved this to Todo in Web Forms Mar 8, 2024
@eyelidlessness eyelidlessness mentioned this issue Mar 8, 2024
Merged
@eyelidlessness eyelidlessness mentioned this issue Apr 1, 2024
Merged
12 tasks
@eyelidlessness eyelidlessness mentioned this issue May 30, 2024
Merged
eyelidlessness added a commit that referenced this issue Oct 31, 2024
@eyelidlessness
- - - POSSIBLE REVIEW CUTOFF POINT - - -
301e04d 
For the purposes of this refactor, the commits up to this point represent all but one outstanding external-facing change to the `@getodk/xpath` package.

That remaining change will put the project into a temporarily broken state. It has been intentionally held until after this commit, so we can clearly identify where this temporarily broken state begins.

**What's next**

The bulk of the remaining work on this refactor branch involves **integrating** the `@getodk/xpath` changes before this commit into `@getodk/xforms-engine`.

My intent is to keep the aforementioned broken state as short lived as reasonably possible, and to introduce another potential review cutoff when it is resolved.

There will also be a handful of test changes in `@getodk/scenario`. The child-vaccination smoke test's only remaining blocker will be eliminated by this integration work. There is also an open question around select state/itemset filtering (#57) which we've intentionally left broken to keep that question open; some of the integration work necessitates picking one of the options available to answer that question (albeit also temporarily). The select test suite will be updated to reflect that fact, and expanded to more clearly demonstrate the choices available in finally resolving the open question.

Those test changes will likely lag behind the engine integration changes which make them necessary. If I can (relatively) easily identify where these test changes become pertinent, I will try to commit them at the earliest point possible. This will minimize **test/CI breakage** at intermediate commits. But the intent is that, regardless of test/CI state, the project's **user-facing behavior** will continue to work after resolution of the incoming broken state.
eyelidlessness added a commit that referenced this issue Nov 4, 2024
@eyelidlessness
END BROKEN PROJECT STATE! Integrate engineDOMAdapter
08019cc 
(Note

This commit involves two sets of changes which are inherently tightly coupled:

1. Value state is no longer persisted to a WHAT Working Group DOM backing store
2. Concrete implementations of InstanceNode now act as their own node representations for the purposes of evaluating XPath evaluations, XForms spec computations

**Value state:**

This constitutes the most substantial change in the commit, but it is pretty much a drastic simplification of value state logic. This pretty much comes “for free” by eliminating the need for a WHAT DOM backing store.

I took the opportunity to break up some other logic which with more incidental coupling than I’d consider ideal, and updated JSDoc to reflect both the changes in storage mechanism as well as general clarification where it made sense.

**InstanceNode as EvaluationContext.contextNode:**

For most concrete InstanceNode subclasses, there is very little change beyond updating `contextNode` to reference `this` (with some type refinements where makes sense or necessary to satisfy the internal interfaces).

The more substantial changes are all concerned with eliminating WHAT DOM usage where any such logic still remains _in PrimaryInstance_.

To be clear: as of this commit, the engine no longer uses the WHAT/browser DOM for any aspect of instance state. Remaining WHAT DOM usage in the engine is now restricted to:

- parsing XForm XML into the engine’s internal `*Definition` data model, which is then used to initialize `PrimaryInstance`
- tests

- - -

We are VERY near the end of this refactor! What’s left:

- An outstanding regression in select behavior. Addressing this will involve a judgment call around this open question: #57. The `@getodk/scenario` select.test.ts suite will have updates to reflect these changes.
- Eliminate a few odd repeat-specific workarounds which are now redundant or moot.
- Eliminate most current usage of parse-stage dependency analysis. The underlying dependency analysis **logic** will remain in the codebase, as it will still have value for other work in the future.
- Removal of subscription interfaces and logic which depended on current usage of dependency analysis. As of this commit, both the affected dependency analysis and subscription logic are obsolete. Their responsibilities are now handled **by evaluation of XPath expressions** with the now-integrated engineDOMAdapter!
- Complete porting child-vaccination.test.ts, which will pass once complete!
- Odd assorted cleanup and loose ends, if any remain
eyelidlessness added a commit that referenced this issue Nov 4, 2024
@eyelidlessness
SelectField: selected values are **filtered** from value state on ite…
d55ce0c 
…mset change

This addresses a temporary regression in an earlier commit laying groundwork for engineDOMAdapter integration. The nature of the change is best described by #57

Some previously failing scenario tests now pass. And some new tests are added to clarify the change in behavior.

**NOTE:** this change is not intended to press for a decision on #57. It is primarily meant to ensure that, to the best of my knowledge, this refactor does not have any known regressions (excepting some edge cases discussed in previous commit notes). Secondarily, it’s meant to leave some better breadcrumbs for us to make an informed decision on that issue when we do prioritize it.
eyelidlessness added a commit that referenced this issue Nov 19, 2024
@eyelidlessness
END BROKEN PROJECT STATE! Integrate engineDOMAdapter
5a9b08e 
(Note

This commit involves two sets of changes which are inherently tightly coupled:

1. Value state is no longer persisted to a WHAT Working Group DOM backing store
2. Concrete implementations of InstanceNode now act as their own node representations for the purposes of evaluating XPath evaluations, XForms spec computations

**Value state:**

This constitutes the most substantial change in the commit, but it is pretty much a drastic simplification of value state logic. This pretty much comes “for free” by eliminating the need for a WHAT DOM backing store.

I took the opportunity to break up some other logic which with more incidental coupling than I’d consider ideal, and updated JSDoc to reflect both the changes in storage mechanism as well as general clarification where it made sense.

**InstanceNode as EvaluationContext.contextNode:**

For most concrete InstanceNode subclasses, there is very little change beyond updating `contextNode` to reference `this` (with some type refinements where makes sense or necessary to satisfy the internal interfaces).

The more substantial changes are all concerned with eliminating WHAT DOM usage where any such logic still remains _in PrimaryInstance_.

To be clear: as of this commit, the engine no longer uses the WHAT/browser DOM for any aspect of instance state. Remaining WHAT DOM usage in the engine is now restricted to:

- parsing XForm XML into the engine’s internal `*Definition` data model, which is then used to initialize `PrimaryInstance`
- tests

- - -

We are VERY near the end of this refactor! What’s left:

- An outstanding regression in select behavior. Addressing this will involve a judgment call around this open question: #57. The `@getodk/scenario` select.test.ts suite will have updates to reflect these changes.
- Eliminate a few odd repeat-specific workarounds which are now redundant or moot.
- Eliminate most current usage of parse-stage dependency analysis. The underlying dependency analysis **logic** will remain in the codebase, as it will still have value for other work in the future.
- Removal of subscription interfaces and logic which depended on current usage of dependency analysis. As of this commit, both the affected dependency analysis and subscription logic are obsolete. Their responsibilities are now handled **by evaluation of XPath expressions** with the now-integrated engineDOMAdapter!
- Complete porting child-vaccination.test.ts, which will pass once complete!
- Odd assorted cleanup and loose ends, if any remain
eyelidlessness added a commit that referenced this issue Nov 19, 2024
@eyelidlessness
SelectField: selected values are **filtered** from value state on ite…
23f84f3 
…mset change

This addresses a temporary regression in an earlier commit laying groundwork for engineDOMAdapter integration. The nature of the change is best described by #57

Some previously failing scenario tests now pass. And some new tests are added to clarify the change in behavior.

**NOTE:** this change is not intended to press for a decision on #57. It is primarily meant to ensure that, to the best of my knowledge, this refactor does not have any known regressions (excepting some edge cases discussed in previous commit notes). Secondarily, it’s meant to leave some better breadcrumbs for us to make an informed decision on that issue when we do prioritize it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question Further information is requested
Projects
Web Forms
Status: Todo
Milestone
No milestone
Development

No branches or pull requests
1 participant
@eyelidlessness