Skip to content

Conversation

@matz3
Copy link
Member

@matz3 matz3 commented Feb 28, 2025

The stack of XML namespaces was not being properly maintained which
resulted into wrong assumptions about controls in some scenarios when
multiple default namespaces were present in the XML.

@matz3 matz3 requested a review from a team February 28, 2025 11:14
@matz3 matz3 force-pushed the fix-xml-namespace-handling branch from aeb0223 to d78c225 Compare February 28, 2025 11:18
codeworrior
codeworrior previously approved these changes Feb 28, 2025
Copy link
Member

@codeworrior codeworrior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

localName: null,
namespace: attrValue,
}, this.#nodeStack.length);
}, this.#nodeStack.length + 1); // +1 as the currently created node is not yet on the stack
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the idea is that level counts from 1, I'm fine with this change.- Alternatively, if level should correspond to the node's position in the #nodeStack, popTag could just remove level - 1(or swap L171 and L172 and adapt the constant in L178).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That's a way better solution.

The stack of XML namespaces was not being properly maintained which
resulted into wrong assumptions about controls in some scenarios when
multiple default namespaces were present in the XML.
Copy link
Member

@codeworrior codeworrior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@matz3 matz3 merged commit 4be9fde into main Feb 28, 2025
18 checks passed
@matz3 matz3 deleted the fix-xml-namespace-handling branch February 28, 2025 13:25
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.

3 participants