fix(DqElement): avoid calling constructors with cloneNode#5013
fix(DqElement): avoid calling constructors with cloneNode#5013WilcoFiers merged 3 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #4996 by refactoring how element HTML source is retrieved, avoiding calls to cloneNode() which can trigger custom element constructors. The solution extracts the element source generation logic from DqElement into a new standalone utility function getElementSource that works independently of the virtual tree and uses string manipulation instead of node cloning.
Changes:
- Created new
getElementSourceutility function that generates truncated HTML source without callingcloneNode() - Refactored DqElement to use the new utility function instead of the old
truncateElementimplementation - Improved attribute truncation logic to check each attribute individually instead of stopping at the first that doesn't fit
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/core/utils/get-element-source.js | New utility function that generates element HTML source using outerHTML/XMLSerializer instead of cloneNode, with support for truncation and various node types |
| lib/core/utils/dq-element.js | Removed old truncateElement implementation and updated to use the new getElementSource utility |
| lib/core/utils/index.js | Added export for the new getElementSource utility |
| axe.d.ts | Added TypeScript definitions for the new getElementSource utility function |
| test/core/utils/get-element-source.js | Comprehensive test suite for the new utility covering elements, non-element nodes, namespaces, and truncation scenarios |
| test/core/utils/dq-element.js | Removed source-related tests that were moved to the new test file, keeping only DqElement-specific behavior tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| ({ name, value }) => ({ name, value }) | ||
| ); | ||
| const attrsLength = nodeAttrs.reduce((acc, { name, value }) => { | ||
| return acc + name.length + value.length + 4; |
There was a problem hiding this comment.
I'm guessing what the 4 represents but we should comment the number
| return acc + name.length + value.length + 4; | |
| // 4 = whitespace + equal sign + start and end quote | |
| return acc + name.length + value.length + 4; |
| return acc + name.length + value.length + 4; | ||
| }, 0); | ||
|
|
||
| if (2 + nodeName.length + attrsLength > maxLength) { |
There was a problem hiding this comment.
Same here for the 2
| if (2 + nodeName.length + attrsLength > maxLength) { | |
| // 2 = whitespace before attrs + ? | |
| if (2 + nodeName.length + attrsLength > maxLength) { |
| const attrStr = ` ${attr.name}="${attr.value}"`; | ||
| if (source.length + attrStr.length > maxLength - truncateEnd.length) { | ||
| tagEnd = truncateEnd; | ||
| continue; |
There was a problem hiding this comment.
This should probably break since we've reached the max length. Otherwise this will continue to process attrs we don't need to.
Edit: I see the last test tests that the function can add later attrs that do fit when priors don't. This just means we will always loop over every attribute even if none could fit. Not sure if that's a performance problem, but something to keep in mind.
| source += attrStr; | ||
| } | ||
|
|
||
| return source + tagEnd; |
There was a problem hiding this comment.
One thing to note with this approach is the case where the full outerHTML is too long but the outerHTML of just the element without children is not too long. As far as I am aware node.attributes is not guaranteed to be the same order as the element source. E.g.
<a id="link" href="#" style="color: blue" class="thing"></a>
<script>
link.attributes // { 0: style, 1: href, 2: class, 3: id }
</script>Not having the same order would mean that combining them back into a string could change the attribute order from what the DOM is showing. Not sure if that will be a problem or not, but something to keep in mind. Probably worth the performance gain over doing what we thought before.
There was a problem hiding this comment.
Fair, but that's also the case for the current implementation. Even if we detect that it it'll fit we use node.attributes to generate the string.
| '</svg>' | ||
| ); | ||
| const result = getElementSource(vNode.actualNode, { maxLength: 50 }); | ||
| assert.include(result, 'a'); |
There was a problem hiding this comment.
Just looking for the letter a may not be exactly what we want to test as id="target" also includes the letter a. So not sure if this is testing that the a node is in the string or something else.
Closes: #4996 - **Avoid calling cloneNode when retrieving source** - Add a new utils.getElementSource (which works even if the tree is not constructed - Made sure getElementSource works with namespaces and non-node elements - Check each attribute if it fits in the truncated source, instead of stopping at the first that doesn't fit --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Closes: #4996 - **Avoid calling cloneNode when retrieving source** - Add a new utils.getElementSource (which works even if the tree is not constructed - Made sure getElementSource works with namespaces and non-node elements - Check each attribute if it fits in the truncated source, instead of stopping at the first that doesn't fit --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
### [4.11.2](v4.11.1...v4.11.2) (2026-03-30) ### Bug Fixes - **aria-valid-attr-value:** handle multiple aria-errormessage IDs ([#4973](#4973)) ([9322148](9322148)) - **aria:** prevent getOwnedVirtual from returning duplicate nodes ([#4987](#4987)) ([99d1e77](99d1e77)), closes [#4840](#4840) - **DqElement:** avoid calling constructors with cloneNode ([#5013](#5013)) ([88bc57f](88bc57f)) - **existing-rule:** aria-busy now shows an error message for a use with unallowed children ([#5017](#5017)) ([dded75a](dded75a)) - **scrollable-region-focusable:** clarify the issue is in safari ([#4995](#4995)) ([2567afd](2567afd)), closes [WebKit#190870](https://github.com/dequelabs/WebKit/issues/190870) [WebKit#277290](https://github.com/dequelabs/WebKit/issues/277290) - **scrollable-region-focusable:** do not fail scroll areas when all content is visible without scrolling ([#4993](#4993)) ([240f8b5](240f8b5)) - **target-size:** determine offset using clientRects if target is display:inline ([#5012](#5012)) ([69d81c1](69d81c1)) - **target-size:** ignore widgets that are inline with other inline elements ([#5000](#5000)) ([cf8a3c0](cf8a3c0))
Closes: #4996