From c15e8dad8d2dd0b2fc14321faa3aad4488c44ca6 Mon Sep 17 00:00:00 2001 From: eyelidlessness Date: Wed, 16 Aug 2023 10:32:18 -0700 Subject: [PATCH 1/3] [repeat-multiple-shared-datalist-itemsets] add failing test --- ...peat-multiple-shared-datalist-itemsets.xml | 165 ++++++++++++++++++ test/spec/itemset.spec.js | 9 + 2 files changed, 174 insertions(+) create mode 100644 test/forms/repeat-multiple-shared-datalist-itemsets.xml 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..1c49b48a --- /dev/null +++ b/test/forms/repeat-multiple-shared-datalist-itemsets.xml @@ -0,0 +1,165 @@ + + + + + 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 From ed481ed342bf73163cc4ed766b16f5c5652367d8 Mon Sep 17 00:00:00 2001 From: eyelidlessness Date: Wed, 16 Aug 2023 11:35:55 -0700 Subject: [PATCH 2/3] [repeat-multiple-shared-datalist-itemsets] fix, with gobs of commentary --- src/js/itemset.js | 40 ++++++++++++-- src/js/repeat.js | 134 +++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 162 insertions(+), 12 deletions(-) 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' ); From 24c71048b4421d89270f6a61ef56abaed331959d Mon Sep 17 00:00:00 2001 From: eyelidlessness Date: Thu, 17 Aug 2023 13:58:16 -0700 Subject: [PATCH 3/3] Remove knowledge base commentary --- test/forms/repeat-multiple-shared-datalist-itemsets.xml | 7 ------- 1 file changed, 7 deletions(-) diff --git a/test/forms/repeat-multiple-shared-datalist-itemsets.xml b/test/forms/repeat-multiple-shared-datalist-itemsets.xml index 1c49b48a..2cc7a802 100644 --- a/test/forms/repeat-multiple-shared-datalist-itemsets.xml +++ b/test/forms/repeat-multiple-shared-datalist-itemsets.xml @@ -18,13 +18,6 @@ - consolidate the explanation into a single place, as the various contributing factors are spread across multiple files/modules, across multiple projects - - Such information is probably better suited for, perhaps, a knowledge base - and some systematic method of linking the related implementation details in - code, so these details can be traced **and kept up to date** as any aspect - changes. In the absence of such a system, and in the interest of expediency, - the commentary is included here, paired with the form particulars exercising - the bug. -->