Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix persistent ids being softMatched when they shouldn't #75

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix persistent ids being softMatched when they shouldn't #75

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
18fbd86
handle persistent id's not softMatching
MichaelWest22 Dec 18, 2024
3a276a1
Improve parent node handling
MichaelWest22 Dec 18, 2024
1f18f87
Fix Type error
MichaelWest22 Dec 18, 2024
35c06a1
Added test
MichaelWest22 Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 44 additions & 9 deletions src/idiomorph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ var Idiomorph = (function () {
* @property {ConfigInternal['ignoreActive']} ignoreActive
* @property {ConfigInternal['ignoreActiveValue']} ignoreActiveValue
* @property {Map<Node, Set<string>>} idMap
* @property {Set<string>} persistentIds
* @property {Set<string>} deadIds
* @property {ConfigInternal['callbacks']} callbacks
* @property {ConfigInternal['head']} head
Expand Down Expand Up @@ -259,7 +260,7 @@ var Idiomorph = (function () {
oldNode.parentNode?.removeChild(oldNode);
ctx.callbacks.afterNodeRemoved(oldNode);
return null;
} else if (!isSoftMatch(oldNode, newContent)) {
} else if (!isSoftMatch(oldNode, newContent, ctx)) {
if (ctx.callbacks.beforeNodeRemoved(oldNode) === false) return oldNode;
if (ctx.callbacks.beforeNodeAdded(newContent) === false) return oldNode;

Expand Down Expand Up @@ -712,6 +713,7 @@ var Idiomorph = (function () {
ignoreActive: mergedConfig.ignoreActive,
ignoreActiveValue: mergedConfig.ignoreActiveValue,
idMap: createIdMap(oldNode, newContent),
persistentIds: persistentIdSet(oldNode, newContent),
deadIds: new Set(),
callbacks: mergedConfig.callbacks,
head: mergedConfig.head
Expand Down Expand Up @@ -746,12 +748,18 @@ var Idiomorph = (function () {
*
* @param {Node | null} node1
* @param {Node | null} node2
* @param {MorphContext} ctx
* @returns {boolean}
*/
function isSoftMatch(node1, node2) {
function isSoftMatch(node1, node2, ctx) {
if (node1 == null || node2 == null) {
return false;
}
// If the id's do not match and either of the id's are persisted through the morph then they can't be soft matches
if ( /** @type {Element} */ (node1).id !== /** @type {Element} */ (node2).id
&& (ctx.persistentIds.has(/** @type {Element} */ (node1).id) || ctx.persistentIds.has(/** @type {Element} */ (node2).id))) {
return false;
}
return node1.nodeType === node2.nodeType &&
// ok to cast: if one is not element, `tagName` will be undefined and we'll compare that
/** @type {Element} */ (node1).tagName === /** @type {Element} */ (node2).tagName
Expand Down Expand Up @@ -871,11 +879,11 @@ var Idiomorph = (function () {
}

// if we have a soft match with the current node, return it
if (isSoftMatch(newChild, potentialSoftMatch)) {
if (isSoftMatch(newChild, potentialSoftMatch, ctx)) {
return potentialSoftMatch;
}

if (isSoftMatch(nextSibling, potentialSoftMatch)) {
if (isSoftMatch(nextSibling, potentialSoftMatch, ctx)) {
// the next new node has a soft match with this node, so
// increment the count of future soft matches
siblingSoftMatchCount++;
Expand Down Expand Up @@ -1046,7 +1054,7 @@ var Idiomorph = (function () {
// TODO: The function handles node1 and node2 as if they are Elements but the function is
// called in places where node1 and node2 may be just Nodes, not Elements
function scoreElement(node1, node2, ctx) {
if (isSoftMatch(node1, node2)) {
if (isSoftMatch(node1, node2, ctx)) {
// ok to cast: isSoftMatch performs a null check
return .5 + getIdIntersectionCount(ctx, /** @type {Node} */ (node1), node2);
}
Expand Down Expand Up @@ -1128,7 +1136,19 @@ var Idiomorph = (function () {
}

/**
* A bottom up algorithm that finds all elements with ids inside of the node
* @param {Element} Content Content containing id's
* @returns {Element[]} all nodes that have id's
*/
function nodesWithIds(Content) {
let Nodes = Array.from(Content.querySelectorAll('[id]'));
if(Content.id) {
Nodes.push(Content);
}
return Nodes;
}

/**
* A bottom up algorithm that finds all elements with ids in the node
* argument and populates id sets for those nodes and all their parents, generating
* a set of ids contained within all nodes for the entire hierarchy in the DOM
*
Expand All @@ -1137,9 +1157,7 @@ var Idiomorph = (function () {
*/
function populateIdMapForNode(node, idMap) {
let nodeParent = node.parentElement;
// find all elements with an id property
let idElements = node.querySelectorAll('[id]');
for (const elt of idElements) {
for (const elt of nodesWithIds(node)) {
/**
* @type {Element|null}
*/
Expand Down Expand Up @@ -1180,6 +1198,23 @@ var Idiomorph = (function () {
return idMap;
}

/**
* @param {Element} oldContent the old content that will be morphed
* @param {Element} newContent the new content to morph to
* @returns {Set<string>} the id set of all persistent nodes that exist in both old and new content
*/
function persistentIdSet(oldContent, newContent) {
let idSet = new Set();
for (const oldNode of nodesWithIds(oldContent)) {
for (const newNode of nodesWithIds(newContent)) {
if (oldNode.id === newNode.id) {
idSet.add(oldNode.id);
}
}
}
return idSet;
}

//=============================================================================
// This is what ends up becoming the Idiomorph global object
//=============================================================================
Expand Down
31 changes: 31 additions & 0 deletions test/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,4 +474,35 @@ describe("Core morphing tests", function(){
}
});

it('non-attribute state of element with id is not transfered to other elements when moved between different containers', function()
{
getWorkArea().append(make(`
<div>
<div id="left">
<input type="checkbox" id="first">
</div>
<div id="right">
<input type="checkbox" id="second">
</div>
</div>
`));
document.getElementById("first").indeterminate = true

let finalSrc = `
<div>
<div id="left">
<input type="checkbox" id="second">
</div>
<div id="right">
<input type="checkbox" id="first">
</div>
</div>
`;
Idiomorph.morph(getWorkArea(), finalSrc, {morphStyle:'innerHTML'});

getWorkArea().innerHTML.should.equal(finalSrc);
// second checkbox is now in firsts place and we don't want firsts indeterminate state to be retained
// on what is now the second element so we need to prevent softMatch mroph from matching persistent id's
document.getElementById("second").indeterminate.should.eql(false)
});
})