Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

Commit

Permalink
[repeat-multiple-shared-datalist-itemsets] fix, with gobs of commentary
Browse files Browse the repository at this point in the history
  • Loading branch information
@eyelidlessness
eyelidlessness committed Aug 16, 2023
1 parent c15e8da commit ed481ed
Show file tree
Hide file tree
Showing 2 changed files with 162 additions and 12 deletions.
40 changes: 35 additions & 5 deletions src/js/itemset.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 = {};
Expand Down Expand Up @@ -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}"]`
Expand Down Expand Up @@ -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).
Expand Down
134 changes: 127 additions & 7 deletions src/js/repeat.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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' );
Expand Down

0 comments on commit ed481ed

Please sign in to comment.