From 74f085c395bbbd749b99fed2165be7b461ea822f Mon Sep 17 00:00:00 2001 From: Martijn van de Rijdt Date: Fri, 1 Oct 2021 14:21:23 -0600 Subject: [PATCH] Fixed: itemset inside first repeat is not evaluated correctly when multiple repeat instances are present, closes #818 --- src/js/dom-utils.js | 36 ++++- .../itemset-repeat-relevant-current-new.xml | 123 ++++++++++++++++++ ...> itemset-repeat-relevant-current-old.xml} | 18 +-- test/spec/dom-utils.spec.js | 78 ++++++++++- test/spec/itemset.spec.js | 30 ++++- 5 files changed, 270 insertions(+), 15 deletions(-) create mode 100644 test/forms/itemset-repeat-relevant-current-new.xml rename test/forms/{reprelcur1.xml => itemset-repeat-relevant-current-old.xml} (91%) diff --git a/src/js/dom-utils.js b/src/js/dom-utils.js index aaa0c8c95..b06d988bf 100644 --- a/src/js/dom-utils.js +++ b/src/js/dom-utils.js @@ -153,7 +153,7 @@ function empty( element ) { /** * @param {Element} el - Target node - * @return {boolean} Whether previous sibling has same node name + * @return {boolean} Whether previous sibling has the same node name */ function hasPreviousSiblingElementSameName( el ) { let found = false; @@ -173,6 +173,36 @@ function hasPreviousSiblingElementSameName( el ) { return found; } +/** + * @param {Element} el - Target node + * @return {boolean} Whether next sibling has the same node name + */ +function hasNextSiblingElementSameName( el ) { + let found = false; + const nodeName = el.nodeName; + el = el.nextSibling; + + while ( el ) { + // Ignore any sibling text and comment nodes (e.g. whitespace with a newline character) + // also deal with repeats that have non-repeat siblings in between them, event though that would be a bug. + if ( el.nodeName && el.nodeName === nodeName ) { + found = true; + break; + } + el = el.nextSibling; + } + + return found; +} + +/** + * @param {Element} el - Target node + * @return {boolean} Whether a sibling has the same node name + */ +function hasSiblingElementSameName( el ) { + return hasNextSiblingElementSameName( el ) || hasPreviousSiblingElementSameName( el ); +} + /** * @param {Element} node - Target node * @param {string} content - Text content to look for @@ -225,7 +255,7 @@ function getXPath( node, rootNodeName = '#document', includePosition = false ) { while ( parent && parentName !== rootNodeName && parentName !== '#document' ) { if ( includePosition ) { index = getRepeatIndex( parent ); - position = ( index > 0 ) ? `[${index + 1}]` : ''; + position = hasSiblingElementSameName( parent ) ? `[${index + 1}]` : ''; } steps.push( parentName + position ); parent = parent.parentElement; @@ -417,6 +447,8 @@ export { getXPath, hasPreviousCommentSiblingWithContent, hasPreviousSiblingElementSameName, + hasNextSiblingElementSameName, + hasSiblingElementSameName, closestAncestorUntil, empty, MutationsTracker diff --git a/test/forms/itemset-repeat-relevant-current-new.xml b/test/forms/itemset-repeat-relevant-current-new.xml new file mode 100644 index 000000000..b6285a4b2 --- /dev/null +++ b/test/forms/itemset-repeat-relevant-current-new.xml @@ -0,0 +1,123 @@ + + + + repeat-relative-current + + + + + Beans + + + Banana + + + Coffee + + + Cacao + + + Coffee + + + Banana + + + Beans + + + Cacao + + + + + + + + + + + + + + + + + + + + + static_instance-crop_list-0 + banana + + + static_instance-crop_list-1 + beans + + + static_instance-crop_list-2 + cacao + + + static_instance-crop_list-3 + coffee + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/forms/reprelcur1.xml b/test/forms/itemset-repeat-relevant-current-old.xml similarity index 91% rename from test/forms/reprelcur1.xml rename to test/forms/itemset-repeat-relevant-current-old.xml index 85c3cb791..2a9dd110c 100644 --- a/test/forms/reprelcur1.xml +++ b/test/forms/itemset-repeat-relevant-current-old.xml @@ -1,11 +1,11 @@ - repeat-relative-current @@ -73,7 +73,7 @@ - + @@ -120,4 +120,4 @@ - \ No newline at end of file + diff --git a/test/spec/dom-utils.spec.js b/test/spec/dom-utils.spec.js index c31fc05b9..a6aab564e 100644 --- a/test/spec/dom-utils.spec.js +++ b/test/spec/dom-utils.spec.js @@ -1,4 +1,4 @@ -import { getSiblingElements, getSiblingElementsAndSelf, getSiblingElement, getAncestors, closestAncestorUntil, getChildren, getChild, getXPath } from '../../src/js/dom-utils'; +import { getSiblingElements, getSiblingElementsAndSelf, getSiblingElement, getAncestors, closestAncestorUntil, getChildren, getChild, getXPath, hasNextSiblingElementSameName, hasSiblingElementSameName, hasPreviousSiblingElementSameName } from '../../src/js/dom-utils'; function getFragment( htmlStr ) { return document.createRange().createContextualFragment( htmlStr ); @@ -247,12 +247,15 @@ describe( 'DOM utils', () => { + + + @@ -269,24 +272,97 @@ describe( 'DOM utils', () => { expect( getXPath( node, null ) ).to.equal( '/root/path/to/node' ); } ); + it( 'returns same /root/path/to/node if first parameter is null and second parameter is true', () => { + const node = xml.querySelector( 'node' ); + expect( getXPath( node, null ) ).to.equal( '/root/path/to/node' ); + } ); + it( 'returns path from context first node provided as parameter', () => { const node = xml.querySelector( 'node' ); expect( getXPath( node, 'root' ) ).to.equal( '/path/to/node' ); } ); + it( 'returned path includes no positions if there are no siblings with the same name along the path', () => { const node = xml.querySelector( 'node' ); expect( getXPath( node, 'root', true ) ).to.equal( '/path/to/node' ); } ); + it( 'returned path includes positions when asked', () => { const node = xml.querySelectorAll( 'number' )[ 1 ]; expect( getXPath( node, 'root', true ) ).to.equal( '/path/to/repeat[2]/number' ); } ); + + it( 'returned path includes position of first repeat if sibling repeat exists', () => { + const node = xml.querySelector( 'number' ); + expect( getXPath( node, 'root', true ) ).to.equal( '/path/to/repeat[1]/number' ); + } ); + it( 'returned path includes positions when asked (multiple levels)', () => { const node = xml.querySelectorAll( 'number' )[ 2 ]; expect( getXPath( node, 'root', true ) ).to.equal( '/path/to/repeat[2]/number[2]' ); } ); } ); + describe( 'siblingElement functions', () => { + const xmlStr = ` + + + + + + + + + + + + + + + + + + `; + const xml = new DOMParser().parseFromString( xmlStr, 'text/xml' ); + const repeats = xml.querySelectorAll( 'repeat' ); + const node = xml.querySelector( 'node' ); + + describe( 'hasPreviousSiblingElementSameName', () => { + [ + [ repeats[0], false ], + [ repeats[1], true ], + [ node, false ] + ].forEach( ( [ el, expected ] ) => { + it( 'evaluates correctly', () => { + expect( hasPreviousSiblingElementSameName( el ) ).to.equal( expected ); + } ); + } ); + } ); + describe( 'hasNextSiblingElementSameName', () => { + [ + [ repeats[0], true ], + [ repeats[1], false ], + [ node, false ] + ].forEach( ( [ el, expected ] ) => { + it( 'evaluates correctly', () => { + expect( hasNextSiblingElementSameName( el ) ).to.equal( expected ); + } ); + } ); + } ); + + describe( 'hasSiblingElementSameName', () => { + [ + [ repeats[0], true ], + [ repeats[1], true ], + [ node, false ] + ].forEach( ( [ el, expected ] ) => { + it( 'evaluates correctly', () => { + expect( hasSiblingElementSameName( el ) ).to.equal( expected ); + } ); + } ); + } ); + + } ); } ); diff --git a/test/spec/itemset.spec.js b/test/spec/itemset.spec.js index c3870b478..e1082fcfe 100644 --- a/test/spec/itemset.spec.js +++ b/test/spec/itemset.spec.js @@ -290,11 +290,11 @@ describe( 'Itemset functionality', () => { } ); } ); - describe( ' in a cloned repeat with a predicate including current()/../', () => { - it( 'works', () => { + describe( 'with repeats and a predicate referring to current()/../', () => { + it( 'works in a cloned repeat in an old form that does not contain odk:xforms-version="1.0.0"', () => { // This test is added to show that once the makeBugCompliant function has been removed // itemsets with relative predicates still work. - const form = loadForm( 'reprelcur1.xml' ); + const form = loadForm( 'itemset-repeat-relevant-current-old.xml' ); form.init(); form.view.$.find( '[data-name="/repeat-relative-current/rep/crop"][value="banana"]' ).prop( 'checked', true ).trigger( 'change' ); form.view.$.find( '.add-repeat-btn' ).click(); @@ -306,6 +306,30 @@ describe( 'Itemset functionality', () => { expect( form.view.$.find( sel1 ).eq( 1 ).val() ).to.equal( 'beans' ); expect( form.view.$.find( sel2 ).eq( 1 ).val() ).to.equal( 'beans' ); } ); + + describe( 'works in a first repeat if a second repeat exists and an itemset update is triggered', () => { + + [ + [ 'in an old form that does not contain odk:xforms-version="1.0.0"', 'itemset-repeat-relevant-current-old.xml' ], + [ 'in a form that contains odk:xforms-version="1.0.0"','itemset-repeat-relevant-current-new.xml' ] + ].forEach( ( [ itText, filename ] ) => { + it( itText, ()=> { + const form = loadForm( filename ); + form.init(); + form.view.html.querySelector( '.add-repeat-btn' ).click(); + const repeats = form.view.html.querySelectorAll( '.or-repeat' ); + const crop1 = repeats[0].querySelector( '[data-name="/repeat-relative-current/rep/crop"][value="banana"]' ); + crop1.checked = true; + crop1.dispatchEvent( events.Change() ); + const crop2 = repeats[1].querySelector( '[data-name="/repeat-relative-current/rep/crop"][value="beans"]' ); + crop2.checked = true; + crop2.dispatchEvent( events.Change() ); + const selectorA = 'label:not(.itemset-template) > input[data-name="/repeat-relative-current/rep/sel_a"]'; + expect( [ ...repeats[0].querySelectorAll( selectorA ) ].map( el => el.value ) ).to.eql( [ 'banana' ] ); + } ); + } ); + + } ); } ); describe( 'with a rank widget', () => {