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.

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

Conversation

MichaelWest22
Copy link

@MichaelWest22 MichaelWest22 commented Dec 18, 2024

during the work testing two-step we found that it is soft matching when an id is swapped around to a different location during the morph. Even though the id exists in both the old and new content it can be moved to a different location and when this happens it can detect two elements as softMatches and morph them in place which can transfer hidden state like intermediate checkbox state or focus to an item with a different ID which is not ideal.

So to resolve this I've created a new persistenIds global set with a new routine that finds all id's that match between old and new content. When checking for soft Matches it can then use this set of id's and exclude from soft matching when the id's are not exactly the same and either of the id's is in the persistent id set.

To test this works as expected I pulled in all the two-pass tests and code as well and tested to make sure it passed the tests without needing workarounds.

I also found core tests were accidently left disabled so removed the only. But removed the fix for this so it can be done in another PR

Finally I found while testing that populateIdMapForNode was not working as you would expect as sometimes the oldContent node passed in can contain an ID and it only processed this nodes children via the querySelectorAll leaving possible bugs as the parent node's ID is never added into the set. the documentation for createIdMap() lower down says it should be inclusive of the Node. So I wrote a quick helper funciton to get all Nodes with ID's including self which is used 3 times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant