diff --git a/src/js/itemset.js b/src/js/itemset.js
index be4df9c1..3cf92e03 100644
--- a/src/js/itemset.js
+++ b/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).
diff --git a/src/js/repeat.js b/src/js/repeat.js
index c55bce61..84a8dca5 100644
--- a/src/js/repeat.js
+++ b/src/js/repeat.js
@@ -3,7 +3,7 @@
*
* 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.
@@ -11,6 +11,96 @@
* @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
+ * ...
+ *
...
+ * ```
+ *
+ * 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
+ *
+ * ```
+ *
+ * This attribute is produced by Enketo Transformer.
+ *
+ * - Its `data-repeat-fixed` attribute, if defined in the corresponding XForm
+ * with `jr:noAddRemove="true()"`.
+ *
+ * ```html
+ *
+ * ```
+ *
+ * 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
+ *
+ *
+ *
+ *
+ *
+ *
+ * items-0-0
+ *
+ *
+ * ```
+ *
+ * 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
+ *
+ * ```
+ *
+ * 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,16 +666,17 @@ 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'
@@ -593,12 +684,41 @@ export default {
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' );
diff --git a/test/forms/repeat-multiple-shared-datalist-itemsets.xml b/test/forms/repeat-multiple-shared-datalist-itemsets.xml
new file mode 100644
index 00000000..2cc7a802
--- /dev/null
+++ b/test/forms/repeat-multiple-shared-datalist-itemsets.xml
@@ -0,0 +1,158 @@
+
+
+
+
+ repeat-multiple-shared-datalist-itemsets
+
+
+
+
+
+ items-0-0
+
+
+ items-1-0
+
+
+
+
+
+
+
+
+
+
+
+
+
+ items-0-0
+ items 0 0
+
+
+
+
+ items-1-0
+ items 1 0
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/test/spec/itemset.spec.js b/test/spec/itemset.spec.js
index f5c3ec1c..a44b1a77 100644
--- a/test/spec/itemset.spec.js
+++ b/test/spec/itemset.spec.js
@@ -888,6 +888,15 @@ describe('Itemset functionality', () => {
);
});
});
+
+ it('loads form with multiple "shared static" itemsets presented as `datalist`s in a repeat', () => {
+ const form = loadForm(
+ 'repeat-multiple-shared-datalist-itemsets.xml'
+ );
+ const loadErrors = form.init();
+
+ expect(loadErrors.length).to.equal(0, loadErrors.join('\n'));
+ });
});
// Radiobutton itemset set cache problem: https://github.com/OpenClinica/enketo-express-oc/issues/423