dom#equalizeNode fails to process textarea / input combo
kptdobe opened this issue · 10 comments
Failing test: https://app.circleci.com/pipelines/github/adobe/helix-publish/2472/workflows/20a43bdb-d537-4d61-a65d-8d10a5a6b2fb/jobs/3991
I could isolate the piece of the DOM causes the issue:
<form>
<textarea id="rating-comments" name="rating-comments"></textarea>
<input type="submit" value="Send">
</form>
Performing the dom#equalizeNode method on that node throws the following exception:
TypeError: Cannot read property 'insertAdjacentHTML' of undefined
at /Users/acapt/work/dev/helix/helix-shared/src/dom.js:303:17
at /Users/acapt/work/dev/helix/helix-shared/node_modules/ferrum/src/sequence.js:604:5
at each [CURRY] (node_modules/ferrum/src/functional.js:175:12)
at collapseTextAcrossInlineNodes (src/dom.js:244:5)
at Function.equalizeNode.impl (src/dom.js:392:5)
at /Users/acapt/work/dev/helix/helix-shared/src/dom.js:364:20
at /Users/acapt/work/dev/helix/helix-shared/node_modules/ferrum/src/sequence.js:604:5
at each [CURRY] (node_modules/ferrum/src/functional.js:175:12)
at Function.equalizeNode.impl (src/dom.js:357:3)
at /Users/acapt/work/dev/helix/helix-shared/src/dom.js:364:20
at /Users/acapt/work/dev/helix/helix-shared/node_modules/ferrum/src/sequence.js:604:5
at each [CURRY] (node_modules/ferrum/src/functional.js:175:12)
at Function.equalizeNode.impl (src/dom.js:357:3)
at equalizeNode (src/dom.js:180:45)
at Function.equalizeNode.impl (src/dom.js:207:12)
at equalizeNode (src/dom.js:180:45)
at Context.<anonymous> (test/dom.test.js:93:9)
at processImmediate (internal/timers.js:439:21)
In the case of the failing test, the entry point is dom#dumpDOM method which catch the exception but does not expose it. Even worst, it continues to execute while one of the node (nodeA or nodeB) has been partially processed already:
https://github.com/adobe/helix-shared/blob/main/src/dom.js#L687-L693
Wow, great find!
I think it's unlikely that this is caused by the input being non-closed. equalizeNode operates on the dom tree and both alternate forms (, ) normalize to during the dom parsing stage.
insertAdjacentHTML is not defined on all nodes, only elements, so I suspect this is caused by after in line 303 being a text node instead of an element…how this happens is unclear.
What sort of browser environment are the tests executed in?
I am kind of proud it took so long for this function to have a bug…this is complex as heck…
This bug is part of the cross element space normalization feature of the normalizer (cases like <b>foo <b>bar turning into <b>foo</b> bar.
It might be possible to fix this by adding an if statement testing whether after is a text node and changing the text node directly instead of adding a space directly to the dom node…however I don't think this situation was ever intended so I'll have to wrap my brain around this for a while…
What does the test fail with? The exception?
I am not sure the test is entirely correct; it discards whitespaces instead of collapsing them…the normalizer explicitly handles whitespace between elements…
New minimal example:
<span style="white-space: pre"></span>`,
The issue has nothing to do with form elements; it just happens to be about the handling of elements that are both inline (like span) and use <pre> style rendering. display: inline; white-space: pre in CSS terms.
This is handled as a special case because a <pre> </pre> b must collapse to a <pre> </pre> b not a<pre> </pre>b. This is a special case and must be handled as such.
I will add a selection of tests covering elements that are both inline and pre.
There's a fix in the branch :)
Thanks @koraa !
Even if the root cause is not the form element, the form element fired an exception, I would still keep it as a test case to make sure the test is correct. The test output can be adjusted, I could never validate the output since it failed before.
Sure! Feel free to re-add the test; optimally with a pointer to the issue so people know it's a regression test!
🎉 This issue has been resolved in version 7.19.1 🎉
The release is available on:
Your semantic-release bot 📦🚀