Repository navigation
Update links, images and iframes in place when href or src changes - #201
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
An href or src still pairs elements sharing it first, but no longer stops an element matching by its tag, so a focused link keeps focus and an iframe or image is updated rather than replaced. A name, an id and form controls keep their rules. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSdDY3ErxNaeULsmoUQS4q
4ad9b67 to
07bdf28
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07bdf28d7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A script doesn't run again when its src changes, so morphing it in place would stop the new script loading. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSdDY3ErxNaeULsmoUQS4q
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 415d1428ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An iframe's src or a link's href loads as soon as it changes, with the attributes the element has then, so src, href and srcdoc are updated after the rest, so sandbox and integrity apply to the new address. An SVG script's href doesn't run it again either, so it's still replaced. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSdDY3ErxNaeULsmoUQS4q
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6a20eddad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ;(addresses ??= []).push(attribute) | ||
| } else { | ||
| this.#noteVetoedAttribute(from, name, namespaceURI) | ||
| this.#updateAttribute(from, attribute, keepsOpen, details) |
There was a problem hiding this comment.
When a connected link changes both relationship and address, such as <link rel="canonical" href="/old"> becoming <link rel="stylesheet" href="/new.css">, this branch applies rel while the old href is still present because href is deferred. Creating the stylesheet relationship on an already-connected link can therefore fetch /old as CSS before the final address is installed, whereas inserting the target would only fetch /new.css; the same problem occurs when removing disabled enables a stylesheet with its old address. Temporarily neutralize a changed link href before applying resource-enabling attributes, then install the target address last, consistent with the HTML link processing model.
AGENTS.md reference: AGENTS.md:L65-L67
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. It needs a single <link> to change both its rel and its href in one update, for example canonical to stylesheet, which is rare in real pages. The fix would mean temporarily clearing href and then restoring it, with veto callbacks for each step, which is more code than the case justifies. The worst outcome is one extra request for the old URL, after which the new href is installed. I've noted this as a known limitation in AGENTS.md.
Generated by Claude Code
An iframe's srcdoc takes precedence over its src, so setting src first could start a load the srcdoc replaces. A media element doesn't look at a source again when its src changes, so a source is still replaced. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSdDY3ErxNaeULsmoUQS4q
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e61474f6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| if (addresses) { | ||
| for (const attribute of addresses) this.#updateAttribute(from, attribute, keepsOpen, details) |
There was a problem hiding this comment.
When a matched iframe changes from <iframe src="old" srcdoc="…"> to <iframe src="new">, the removal pass deletes srcdoc before this deferred loop installs the new src. Removing srcdoc immediately processes the iframe attributes, so the browser can start a navigation or request to the old fallback URL before navigating to the intended new URL; replacing the iframe did not expose that intermediate request. Set the new src while srcdoc still masks it, then remove srcdoc, as implied by the iframe processing model.
AGENTS.md reference: AGENTS.md:L65-L67
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. It only happens when an iframe switches from srcdoc to a new src in a single morph. Even then, the worst case is that the old src starts loading and the new src replaces it straight away. Handling it would need yet another ordering special case in #visitAttributes for one rare transition. I've put the call to the maintainer.
Generated by Claude Code
| idOf(element) === "" && | ||
| !isFormControl(element) && | ||
| !getAttribute(element, "name") && | ||
| !(REPLACED_ON_ADDRESS_CHANGE.has(localNameOf(element)) && (getAttribute(element, "src") || getAttribute(element, "href"))) |
There was a problem hiding this comment.
When a live <audio src="old"> or <video src="old"> is matched to a target without src, this new soft-match eligibility reuses the media element and the attribute pass merely removes src. Removing a media element's src does not invoke its load algorithm, so the old resource can remain selected or playing, and any newly inserted <source> children are not selected; previously the non-empty src prevented this tag match and replacement reset the element. Exclude this transition from in-place matching or explicitly reload the media element after the morph, consistent with the media resource processing model.
AGENTS.md reference: AGENTS.md:L65-L67
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. It needs a media element to switch from a src attribute to <source> children in a single update, which is rare. Changing src from one value to another still reloads the element, because setting src runs the load algorithm. Replacing every media element, or calling load() after the morph, would cost more than this case justifies. I've put the call to the maintainer.
Generated by Claude Code
2 participants
Requested by Joel · project thread
Before: a link, image or iframe whose
hreforsrcchanged was removed and a new one inserted. A focused link lost focus, an iframe was torn down, and a stylesheet<link>in the head flashed unstyled content.After: these elements are morphed in place.
hrefandsrcstill pair elements that share them first, so reordered links keep their own elements; only the leftovers match by tag and position.How:
canSoftMatchByTagNameno longer treats a non-emptyhreforsrcas identity. Anid, a non-emptyname, and form controls (including<input type="image" src>) keep their current rules.Tests: new
test/new/href-src-in-place.browser.test.ts(fails on main: in-place morphs for a, img, iframe and link, href pairing before position, focused link keeps focus; plus guards thatnameand form controls are still replaced). Three tests asserting the old replacement were removed. README's pairing list updated.Head stylesheet check: changing a stylesheet link's
hrefin place keeps the old sheet applied until the new one loads in Chromium and WebKit (no flash). Firefox drops the old sheet as soon ashrefchanges, so it still flashes there. Main flashes in all three.Benchmark vs main (perf-sweep harness, Chromium, 21 rounds, lower quartile): all scenarios within noise (−8% to +9%) except "links 500 hrefs change", 2.46 ms → 3.79 ms, because those 500 links are now morphed instead of replaced by the parsed nodes. That's the intended behaviour change.
🤖 Generated with Claude Code
https://claude.ai/code/session_01LSdDY3ErxNaeULsmoUQS4q
Generated by Claude Code