Keep an image's alternative text and intrinsic size in attachment content - #1230
Keep an image's alternative text and intrinsic size in attachment content#1230jeremy wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Preserves image alternative text and intrinsic dimensions through attachment sanitization and Action Text round trips.
Changes:
- Allows
altglobally andwidth/heightspecifically on permitted<img>elements. - Adds unit, Action Text, and system coverage.
- Adds an avatar to the dummy mention attachment fixture.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/config/dom_purify.js |
Extends sanitized image attributes. |
test/javascript/unit/dom_purify_image_attributes.test.js |
Tests attribute preservation and scoping. |
test/actiontext/image_attributes_test.rb |
Checks server-side attribute preservation. |
test/system/attachment_content_images_test.rb |
Tests the complete persistence round trip. |
test/dummy/app/views/people/_person.html.erb |
Adds the test avatar markup. |
test/helpers/lexxy/tag_helper_test.rb |
Makes mention assertions structure-based. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Verification
Nothing here needed a browser beyond what CI already runs — the widening is decided entirely by CI: green (9/9). |
|
Handoff note for the whole campaign, including QA results and what still needs a human decision: #1234 |
350ebc7 to
e520212
Compare
…tent An <img> inside a custom attachment is sanitized by Lexxy every time the attachment renders in the editor, so what it may carry is Lexxy's decision rather than the app's. alt and width/height were both being dropped: alt is what a screen reader reads, and width/height are what stop the line reflowing while the image loads. Neither can execute anything. alt goes in the blanket list, since it is inert on every element it can appear on. srcset stays out — it carries URLs, so it belongs to a consumer that declares it. width/height are scoped to img instead, through a new per-tag mechanism, because ALLOWED_ATTR is not per-tag: putting them in the blanket list would also permit `<table width="100000">` and `<td height="500">` in attachment content, which is layout the editor previously stripped. The widening only ever applies to tags the caller already permits, so it never admits an element. Covered on both sides of the round trip. The Action Text test reads Lexxy's allowlist out of source rather than copying it, so widening one side without the other fails there; the system test drives the dummy app end to end — editor DOM, submitted value, rendered page, re-edited document — which needed the mention partial to carry an avatar, as the consumer shape this exists for does. A data: URI keeps that self-contained and off the network. The tag helper test asserts the mention's parts rather than the partial's exact markup, so adding the avatar doesn't make it brittle.
Review catch. The agreement check intersected the blanket allowlist with a hardcoded `%w[ alt ]`, so a newly blanket-allowed attribute sat outside it — while the header claimed "widening one side without the other fails here". It would not have. Mutation-checked: adding `longdesc` to ALLOWED_HTML_ATTRIBUTES now fails, and did not before. It now takes the whole blanket list minus one documented exemption, and pins the exemption too, so NOT_PERSISTED cannot quietly become a place to hide real disagreements. That exemption is `contenteditable` alone. The old comment said Action Text drops `contenteditable` and `style` "by design"; it keeps `style`. Both sides agree on it, so it belongs in the check rather than in an exception list. On reading the constants out of dom_purify.js, which STYLE.md:319 would otherwise argue against: kept, with the reasoning written into the file. The property is that two allowlists in two languages agree, and Ruby has no public interface onto the JavaScript one — copying the list across converts a drift this test catches into one it cannot see, since the Ruby copy would keep passing while Lexxy widened underneath it. The coupling is bounded by the vacuity guard, which fails loudly on a rename instead of passing on an empty list.
e520212 to
340d4b5
Compare
Consumer action: none. This widens what an allowed element may carry; it
never admits an element.
An
<img>inside a custom attachment is sanitized by Lexxy every time theattachment renders in the editor, so what it may carry is Lexxy's decision rather
than the host app's.
altandwidth/heightwere all being dropped.altiswhat a screen reader reads;
width/heightare what stop the line reflowingwhile the image loads. None of the three can execute anything.
altgoes in the blanket list, since it is inert on every element it can appearon.
srcsetdeliberately stays out — it carries URLs, so it belongs to a consumerthat declares it.
width/heightare scoped toimginstead, via a new per-tag mechanism, becauseALLOWED_ATTRis not per-tag: putting them in the blanket list would also permit<table width="100000">and<td height="500">in attachment content, which islayout the editor previously stripped. The widening only ever applies to tags the
caller already permits.
Verification
Both sides of the round trip, because neither catches the other:
test/actiontext/image_attributes_test.rbreads Lexxy's allowlist out ofsource rather than copying it, so widening one side without the other fails
there.
test/system/attachment_content_images_test.rbdrives the dummy app end to end— editor DOM, submitted value, rendered page, re-edited document. That needed
the dummy mention partial to carry an avatar, since the consumer shape this
exists for does; a
data:URI keeps it self-contained and off the network.alt/sizeand the tag-scoping cases fail.
The tag helper test now asserts the mention's parts rather than the partial's
exact markup, so adding an avatar doesn't make it brittle.
Part of a series re-filing #1227 at reviewable scope, after #1227 was reverted
from
mainin 8c64aa4. Merge order: #1228 → #1229 → this → #instance →#trusted-types → #1226. Nothing here is released.
Draft: needs human review and a soak period before merging.