Fix: multiple "static" itemset datalists in the same repeat (#995)

Author: eyelidlessness

src/js/itemset.js

@@ -21,9 +21,16 @@ import events from './event';
 /**
  * This function tries to determine whether an XPath expression for a nodeset from an external instance is static.
  * Hopefully in the future it can do this properly, but for now it considers any expression
- * with a non-numeric (position) predicate to be dynamic.
+ * it determines to have a non-numeric (position) predicate to be dynamic.
  * This function relies on external instances themselves to be static.
  *
+ * Known issues:
+ *
+ * - Broadly, this function uses regular expressions to attempt static analysis on an XPath expression. This is prone to false positives *and* negatives, particularly concerning string sub-expressions.
+ * - The check for a reference to an instance does not handle [non-`instance`] absolute or relative path expressions.
+ * - The check for a reference to an instance does not account for expressions where that reference may *itself* appear as a sub-expression (e.g. in a predicate, or as a function parameter).
+ * - At least the numeric predicate does not account for whitespace.
+ *
  * @static
  * @param {string} expr - XPath expression to analyze
  * @return {boolean} Whether expression contains a predicate
@@ -132,8 +139,31 @@ export default {
                 return;
             }
 
-            const shared =
-                template.parentElement.parentElement.matches('.or-repeat-info');
+            const templateParent = template.parentElement;
+            const isShared =
+                // Shared itemset datalists and their related DOM elements were
+                // previously reparented directly under `repeat-info`. They're
+                // now reparented to a container within `repeat-info` to fix a
+                // bug when two or more such itemsets are present in the same
+                // repeat.
+                //
+                // The original check for this condition was tightly coupled to
+                // the previous structure, leading to errors even after the root
+                // cause had been fixed. This has been revised to check for a
+                // class explicitly describing the condition it's checking.
+                //
+                // TODO (2023-08-16): This continues to add to the view's role
+                // as a (the) source of truth about both form state and form
+                // definition. While expedient, it must be acknowledged as
+                // additional technical debt.
+                templateParent.classList.contains(
+                    'repeat-shared-datalist-itemset'
+                ) ||
+                // It's currently unclear whether there are other cases this
+                // would still handle. It's currently preserved in case its
+                // removal might cause unknown regressions. See
+                // https://en.wiktionary.org/wiki/Chesterton%27s_fence
+                templateParent.parentElement.matches('.or-repeat-info');
             const inputAttributes = {};
 
             const newItems = {};
@@ -162,7 +192,7 @@ export default {
                     inputAttributes.disabled = 'disabled';
                 }
             } else if (list && list.nodeName.toLowerCase() === 'datalist') {
-                if (shared) {
+                if (isShared) {
                     // only the first input, is that okay?
                     input = that.form.view.html.querySelector(
                         `input[name="${list.dataset.name}"]`
@@ -195,7 +225,7 @@ export default {
              * Determining the index is expensive, so we only do this when the itemset is inside a cloned repeat and not shared.
              * It can be safely set to 0 for other branches.
              */
-            const index = !shared ? that.form.input.getIndex(input) : 0;
+            const index = !isShared ? that.form.input.getIndex(input) : 0;
             const safeToTryNative = true;
             // Caching has no advantage here. This is a very quick query
             // (natively).

src/js/repeat.js

@@ -3,14 +3,104 @@
  *
  * Two important concepts are used:
  * 1. The first XLST-added repeat view is cloned to serve as a template of that repeat.
- * 2. Each repeat series has a sibling .or-repeat-info element that stores info that is relevant to that series.
+ * 2. Each repeat series has a sibling .or-repeat-info element that stores info that is relevant to that series. (More details below)
  *
  * Note that with nested repeats you may have many more series of repeats than templates, because a nested repeat
  * may have multiple series.
  *
  * @module repeat
  */
 
+/**
+ * "Repeat info" elements are used to convey and/or contain several sorts of
+ * metadata, optimization-focused state, and interactive behavior. The following
+ * should be regarded as non-exhaustive, but the intent is to update it as any
+ * further usage becomes known.
+ *
+ * Metadata:
+ *
+ * - The "repeat info" element itself serves as a footer for a series of zero or
+ *   more repeat instances in the view, i.e. a marker designating where a given
+ *   series of repeat instances *does or may* precede.
+ *
+ *   ```html
+ *   <section class="or-repeat" name="/root/repeat-name">...</section>
+ *   <div class="or-repeat" data-name="/root/repeat-name">...</div>
+ *   ```
+ *
+ *   This element is produced by Enketo Transformer.
+ *
+ * - Its `data-name` attribute is the nodeset referenced by its associated
+ *   repeat instances (if any).
+ *
+ *   This attribute is produced by Enketo Transformer.
+ *
+ * - Its `data-repeat-count` attribute is the repeat's `jr:count` expression, if
+ *   defined in the corresponding XForm.
+ *
+ *    ```html
+ *    <div class="or-repeat" data-repeat-count="/data/repeat-count" ...>
+ *    ```
+ *
+ *   This attribute is produced by Enketo Transformer.
+ *
+ * - Its `data-repeat-fixed` attribute, if defined in the corresponding XForm
+ *   with `jr:noAddRemove="true()"`.
+ *
+ *    ```html
+ *    <div class="or-repeat" data-repeat-fixed ...>
+ *    ```
+ *
+ *   This attribute is produced by Enketo Transformer.
+ *
+ * Optimization-focused state:
+ *
+ * - "Shared", "static" itemsets—when rendered as `datalist`s—along with their
+ *   associated translation definitions, and the current state of their
+ *   translated label elements. A minimal (seriously!) example:
+ *
+ *    ```html
+ *    <div class="repeat-shared-datalist-itemset-elements" style="display: none;">
+ *      <datalist id="datarepitem0" data-name="/data/rep/item-0" class="repeat-shared-datalist-itemset">
+ *        <option class="itemset-template" value="" data-items-path="instance('items-0')/item">...</option>
+ *        <option value="items-0-0" data-value="items 0 0"></option>
+ *      </datalist>
+ *
+ *      <span class="or-option-translations" style="display:none;" data-name="/data/rep/item-0"> </span>
+ *
+ *      <span class="itemset-labels" data-value-ref="name" data-label-type="itext" data-label-ref="itextId" data-name="/data/rep/item-0">
+ *        <span lang="en" class="option-label active" data-itext-id="items-0-0">items-0-0</span>
+ *      </span>
+ *    </div>
+ *    ```
+ *
+ *    The child elements are first produced by Enketo Transformer. They are then
+ *    identified (itemset.js), augmented and reparented (repeat.js) by Enketo
+ *    Core to the outer element created during form initialization.
+ *
+ * Interactive behavior:
+ *
+ * - The button used to add new repeat user-controlled instances (i.e. when
+ *   instances are not controlled by `jr:count` or `jr:noAddRemove`):
+ *
+ *    ```html
+ *    <button type="button" class="btn btn-default add-repeat-btn">...</button>
+ *    ```
+ *
+ *    This element is created and appended in Enketo Core, with requisite event
+ *    handler(s) for user interaction when adding repeat instances.
+ *
+ *    Each user-controlled repeat instance's corresponding removal button is
+ *    contained by its respective repeat instance, under a `.repeat-buttons`
+ *    element (also added by Enketo Core; no other buttons are added besides the
+ *    removal button).
+ * @typedef {HTMLDivElement} EnketoRepeatInfo
+ * @property {`${string}or-repeat-info${string}`} className - This isn't the
+ * best! It just ensures `EnketoRepeatInfo` is a distinct type (according to
+ * TypeScript and its language server), rather than an indistinguishable alias
+ * to `HTMLDivElement`.
+ */
+
 import $ from 'jquery';
 import { t } from 'enketo/translator';
 import dialog from 'enketo/dialog';
@@ -576,29 +666,59 @@ export default {
                     if (this.staticLists.includes(id)) {
                         datalist.remove();
                     } else {
-                        // Let all identical input[list] questions amongst all repeat instances use the same
-                        // datalist by moving it under repeatInfo.
-                        // It will survive removal of all repeat instances.
+                        // Let all identical input[list] questions amongst all
+                        // repeat instances use the same datalist by moving it
+                        // under repeatInfo. It will survive removal of all
+                        // repeat instances.
+
                         const parent = datalist.parentElement;
                         const { name } = input;
 
                         const dl = parent.querySelector('datalist');
                         const detachedList = parent.removeChild(dl);
                         detachedList.setAttribute('data-name', name);
-                        repeatInfo.appendChild(detachedList);
 
                         const translations = parent.querySelector(
                             '.or-option-translations'
                         );
                         const detachedTranslations =
                             parent.removeChild(translations);
                         detachedTranslations.setAttribute('data-name', name);
-                        repeatInfo.appendChild(detachedTranslations);
 
                         const labels = parent.querySelector('.itemset-labels');
                         const detachedLabels = parent.removeChild(labels);
                         detachedLabels.setAttribute('data-name', name);
-                        repeatInfo.appendChild(detachedLabels);
+
+                        // Each of these supporting elements are nested in a
+                        // containing element, so any subsequent DOM queries for
+                        // their various sibling elements don't mistakenly match
+                        // those from a previous itemset in the same repeat.
+                        const sharedItemsetContainer =
+                            document.createElement('div');
+
+                        sharedItemsetContainer.style.display = 'none';
+                        sharedItemsetContainer.append(
+                            detachedList,
+                            detachedTranslations,
+                            detachedLabels
+                        );
+                        repeatInfo.append(sharedItemsetContainer);
+
+                        // Add explicit class which can be used to determine
+                        // this condition elsewhere. See its usage and
+                        // commentary in `itemset.js`
+                        datalist.classList.add(
+                            'repeat-shared-datalist-itemset'
+                        );
+                        // This class currently serves no functional purpose
+                        // (please do not use it for new functional purposes
+                        // either). It's included specifically so that the
+                        // resulting DOM structure has some indication of why
+                        // it's the way it is, and some way to trace back to
+                        // this code producing that structure.
+                        sharedItemsetContainer.classList.add(
+                            'repeat-shared-datalist-itemset-elements'
+                        );
 
                         this.staticLists.push(id);
                         // input.classList.add( 'shared' );