feat(svg): recognise an svg by reading it - #680
Merged
Conversation
andiwand
force-pushed
the
feat/svg-inline-render
branch
from
August 9, 2026 20:56
e20c94f to
a9bc473
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e20c94feb5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
andiwand
force-pushed
the
feat/svg-inline-render
branch
from
August 10, 2026 05:37
a9bc473 to
9b63cf3
Compare
`FileType::scalable_vector_graphics` was a label the generic image wrapper would put on any bytes at all, and an svg rendered as a base64 data url in an `<img>` — fixed size, no selectable text, nothing to inspect. It now opens as `svg::SvgFile`, an image over an `xml::XmlFile`: the xml module parses it, resolves its encoding from the declaration and records the root element name, and svg answers the one question left. So `open` as an svg rejects what is not one, and detection costs one parse rather than two — `XmlFile::root_name()` is what the second probe reads. The markup goes into the page, so the drawing fits the screen and its text can be selected. That makes it live markup, which behind an `<img>` it was not, so it arrives scrubbed — script and embedding elements, event handlers, references that leave the document, animation aimed at any of those, and css that reaches outside — under a page that declares `script-src 'none'`. An svg *inside* a document keeps the data url: there it is one image in a layout, and `<img>` renders svg in secure static mode. Reasoning in `src/odr/internal/svg/AGENTS.md`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1PApAFAK7q2yN7Rd2UUpr
…refix what is written Two ways the inline rendering read a document differently from the browser that gets it. `attributeName` was looked up through pugixml's exact-match `attribute()`, so a `<set attributename="href">` was left in place — and the html parser lowercases attribute names in foreign content and maps that one straight back, so the animation still aimed at the href the scrub had just taken off. The lookup now goes by local name, case-insensitively, like every other name here. A document that binds the svg namespace to a prefix was written into the page as `<s:svg>`, and the html parser enters foreign content on `svg` alone: a file that drew fine as an `<img>` data url drew nothing at all. The prefixes bound to the svg namespace come off before the markup is written — only those, because a bare `span` or `div` in foreign content is what ends the svg early. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ER7HcXMJZ1Q8azxx4wJRS
andiwand
force-pushed
the
feat/svg-inline-render
branch
from
August 10, 2026 09:55
9b63cf3 to
7db8d40
Compare
`static_cast<char>(std::tolower(static_cast<unsigned char>(c)))` was written out in five places — `pdf_afm`, `text_encoding_table`, `media_file`, `pdf_file` and a test — each rebuilding a lowercased copy to compare or look something up, and `svg_util` had grown a whole private family around it. `util::string` now carries `to_lower` for a char and for a string, plus `equals_ignore_case`, `starts_with_ignore_case` and `find_ignore_case`. The header records that the fold is ascii-only, so a utf-8 sequence compares byte for byte rather than being folded into something else. Each of the five sites goes through it. `is_safe_uri` loses its mutation of the scheme it just built and asks the allowlist directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBhKvrRnEC3m55CCFnEy2y
An svg goes back into the page as the `<img>` data url every other image is, until there is a reason to make one scalable and selectable. Detection stays: `open` as an svg still reads the bytes and rejects what is not one. Inside an `<img>` a browser renders svg in secure static mode — no scripts, no fetches, animation frozen. Inlining the markup gives that up and is the only path here where the input file authors our output DOM, in a WebView with a bridge to native, so it had to be earned back by hand: a scrub of script and embedding elements, event handlers, references that leave the document, SMIL aimed at any of those, and css that reaches outside, plus a `script-src 'none'` page. That is a lot of machinery, and it interferes with the file — a legitimate external image or a webfont went too. It all goes: `html/svg_file.*`, `svg/svg_util.*`, the svg stylesheet, `write_header_meta_equiv`, `ImageFile::impl` and the nine tests that pinned the scrub. `check_svg_file` becomes `is_svg_file`, a predicate — `open_strategy` asks the question without wanting the file, and hands the `XmlFile` on unparsed a second time when the answer is no. `parse_source` goes back to being file-local, as `xml/AGENTS.md` says it is; nothing outside the xml module parses. Each layer now reaches the one below — `SvgFile::xml_file`, `text_file`, `file`, with `text` and `document` forwarded — so a reader downstream needs neither a second parse nor a second decode. The reasoning is on record in `svg/AGENTS.md`, including what inlining would cost if it comes back, and the root `AGENTS.md` states the rule it follows from: the input file never authors the output markup. Which is where the one exception is written down too — a pdf `/URI` is filtered to navigable schemes while a document hyperlink is only escaped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBhKvrRnEC3m55CCFnEy2y
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Generated with Claude Code
FileType::scalable_vector_graphicswas a label the generic image wrapper would put on any bytes at all — nothing read the file to see whether it was one. Detection now reads it, and it costs no parse of its own.What is in here
internal/svg/svg_file.{hpp,cpp}—SvgFile, anabstract::ImageFileover anxml::XmlFile. The xml module parses, rejects what is not well formed and resolves the encoding from the declaration; svg answers the one question left,is_svg_file, againstXmlFile::root_name(). Soopen(file, scalable_vector_graphics)throwsNoSvgFileon anything that is not an svg, and the probe reads the xml parse instead of repeating it.internal/svg/svg_util.{hpp,cpp}is gone. It held the oldcheck_svg_file(std::istream &), which parsed the bytes a second time to look at one element name.is_svg_file(const xml::XmlFile &)replaces it — a predicate rather than a throwing check, becauseopen_strategyasks the question without wanting the file: an xml that is not an svg is handed on as theXmlFilealready built.SvgFile::xml_file(),text_file(),file(), withtext()anddocument()forwarded, andXmlFile::text_file()under that. A reader downstream needs neither a second parse nor a second decode. One trap is documented on the accessor:text_file()->text()decodes with the encoding detected over the bytes,text()with the one the declaration names.NoSvgFileinexceptions.hpp, and thefile_type_tablerow.util::stringgainsto_lower,equals_ignore_case,starts_with_ignore_caseandfind_ignore_case(second commit).static_cast<char>(std::tolower(static_cast<unsigned char>(c)))was written out in five places; all five now go through it, andis_safe_uriloses the mutation of the scheme it had just built.Rendering is unchanged
An svg still goes into the page as
<img src="data:image/svg+xml;base64,…">, the same path as png and jpeg, and the same path an svg inside a document already took.The commit list will look odd, so: this branch first inlined the markup — so the drawing would scale to the viewport and its text be selectable — and that is reverted in
refactor(svg): keep the detection, drop the drawing. Inside an<img>a browser renders svg in secure static mode: no scripts, no external fetches, animation frozen. Inlined, the markup is live, and it is the only path in the library where the input file authors our output DOM — in a WebView with a bridge to native. Earning secure static mode back by hand meant a scrub (script and embedding elements, event handlers, references that leave the document, SMIL aimed at any of those, css reaching outside) plus ascript-src 'none'page — a lot of machinery, and it interferes with the file: a legitimate external image or a webfont went too. Not worth it until something actually needs a scalable, selectable svg; if that comes, isolation in a sandboxed iframe beats modifying the file.That reasoning is recorded in
internal/svg/AGENTS.mdrather than only in the git history, together with the rule it follows from, now in the rootAGENTS.md: the input file never authors the output markup. Which is where the one place we are inconsistent is written down too —html/pdf_file.cppfilters a PDF/URIaction to an allowlist of navigable schemes, while a document hyperlink is onlyescape_attributed, so ajavascript:href in an odt reaches the page. Not touched here; worth deciding separately.Test plan
./test/odr_test --gtest_filter='SvgFile*'— 6 tests: an svg opens as an image that knows it is one, only an svg opens as one (NoSvgFile/NoXmlFile/UnknownFileType), a prefixed root element is still an svg, every layer it was read through is reachable, the declared encoding is what it is decoded with, and it renders as an<img>data url../test/odr_test --gtest_filter='string_util*'— 8 tests, four of them new, covering the char and string overloads, the non-ascii passthrough, a prefix longer than the string, andfrompast the end.