luci-base: form.js: fix stripTags NotFoundError and modify source node - #8936
luci-base: form.js: fix stripTags NotFoundError and modify source node#8936jjm2473 wants to merge 2 commits into
Conversation
b8ff99d to
da2a65c
Compare
da2a65c to
61436bb
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; one inline simplification suggested. The commit message accurately describes the change, and the underlying NotFoundError diagnosis is correct.
Generated by Claude Code
| if (br.parentElement) | ||
| br.parentElement.replaceChild(document.createTextNode('\n'), br); | ||
| else | ||
| x.replaceChild(document.createTextNode('\n'), br); |
There was a problem hiding this comment.
The branch is unnecessary — br.parentNode is the correct target in every case and makes the fallback redundant.
br always comes from x.querySelectorAll('br'), so it is always a descendant of x and always has a parent node. Two situations exist:
xis an element (thedom.parse(${s})path, or an element passed in viadom.elem(s)): the parent is an element, soparentElement === parentNode.xis aDocumentFragmentand the<br>is a direct child of it:parentElementisnull— this is the only case theelsereaches — andparentNodeis the fragment, i.e. exactly whatx.replaceChild()uses. Notedom.elem()only tests fornodeType, so a fragment does reach here.
Using parentNode unconditionally covers both, and matches how the rest of the tree does node replacement (e.g. cbi.js:366, tools/network.js:1383).``
| if (br.parentElement) | |
| br.parentElement.replaceChild(document.createTextNode('\n'), br); | |
| else | |
| x.replaceChild(document.createTextNode('\n'), br); | |
| br.parentNode.replaceChild(document.createTextNode('\n'), br); |
Generated by Claude Code
For example `s='<a><br></a>'`, then `x=dom.parse('<div><a><br></a></div>')`,
this `br` is not a direct child node of `x`,
the subsequent call `x.replaceChild(document.createTextNode('\n'), br)` will result in an error:
```
Uncaught (in promise) NotFoundError: Failed to execute 'replaceChild' on 'Node': The node to be replaced is not a child of this node.
at eval (form.js:312:6)
at NodeList.forEach (<anonymous>)
at ClassConstructor.stripTags (form.js:311:28)
at ClassConstructor.renderFrame (form.js:4434:30)
```
Signed-off-by: Liangbin Lian <jjm2473@gmail.com>
61436bb to
f02fc9d
Compare
`stripTags` modifies the source node, causing switch port status missing `<br>` on first render. Clone source node as AI suggests: openwrt#8937 (comment) Signed-off-by: Liangbin Lian <jjm2473@gmail.com>
f02fc9d to
619ce76
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits (the branch was force-pushed, so 61436bb was rewritten as 19a5954, which adopts the br.parentNode simplification from the previous review).
Both changes look correct:
- 19a5954 —
br.parentNode.replaceChild()is right for every case, sincebralways comes fromx.querySelectorAll('br')and therefore always has a parent, whetherxis an element or aDocumentFragment. - 619ce76 —
cloneNode(true)on thedom.elem(s)branch is the right scope for the mutation fix: only that branch receives a caller-owned node, while thedom.parse()branch already builds a throwaway tree.textContenton the detached clone is unaffected by the clone.
Both commit messages match their diffs. One nit inline about the PR description.
Generated by Claude Code
Pull request details
Description
This PR addresses two issues:
For example
s='<a><br></a>', thenx=dom.parse('<div><a><br></a></div>'), thisbris not a direct child node ofx, the subsequent callx.replaceChild(document.createTextNode('\n'), br)will result in an error:stripTagsmodifies the source node, causing switch port status missing<br>on first render.Clone source node as AI suggests: luci-mod-network: switch: fix port status missing
<br>on first render #8937 (comment)Screenshot or video of changes (if applicable)
Maintainer (preferred)
@systemcrash
Tested on
OpenWrt version: iStoreOS 25.12.5 2026081215
LuCI version: LuCI istoreos-25.12 branch 26.195.42598~4a1ef8c
Web browser(s): Chrome (151.0.7922.76)
Checklist