From 6bebe25bd35f2fed4620db46f44252437c0fe760 Mon Sep 17 00:00:00 2001 From: eyelidlessness Date: Thu, 17 Aug 2023 14:42:02 -0700 Subject: [PATCH] Fix: multiple "static" itemset datalists in the same repeat (#995) --- src/js/itemset.js | 40 ++++- src/js/repeat.js | 134 ++++++++++++++- ...peat-multiple-shared-datalist-itemsets.xml | 158 ++++++++++++++++++ test/spec/itemset.spec.js | 9 + 4 files changed, 329 insertions(+), 12 deletions(-) create mode 100644 test/forms/repeat-multiple-shared-datalist-itemsets.xml 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 + * + * ``` + * + * 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