Keep width and height on SafeHTML images

[rtibblesbot]

Summary

• <img> keeps width/height; other elements still lose them.
• Height-only images derive width from the natural ratio, as Studio's editor does.
• Percentage heights are dropped.
• Match/associate chips keep their fixed box.
• Carried inline styles on images and tables apply through $attrs; the image border and table width move to CSS, and parseStyleString goes.

References

Fixes #15378. First seen: learningequality/studio#6189.

Reviewer guidance

• Unit tests: pnpm test-jest packages/kolibri-common/components/SafeHTML.
• Open question: an authored size lifts the 80vh cap — should it hold for tall images?
• Open question: with both attributes, height follows the natural ratio, not a stretched W×H — is distortion ever intended?

QA steps

Setup: pnpm devserver, a learner, a channel imported via Device > Channels with Safe HTML5 articles holding each step's HTML; QTI steps use /en/learn/#/qti_sandbox/. Images over 400×300.

1. Learn > Library, open an article with images sized W+H, width only, height only, height="50%": authored size, natural ratio, 1px border.
2. Same article at 360px: images shrink undistorted; Expand image shows only on downscaled ones, opening a closable lightbox.
3. Open an article with allowlisted (align, float, colour) and disallowed image styles: only allowlisted ones apply.
4. Open an article with 2-, 3- and 5+-column tables, some sized and styled: 640px up to 3 columns, then 200px each; styles apply; sizes ignored; scrolls at 360px.
5. In the QTI sandbox, load a choice item with sized images in prompt, choices and hint: authored size; selecting, Submit and the hint work.
6. In the QTI sandbox, load three items with an image in the same place — styled, unstyled, restyled: each shows only its own style.

Evidence

Sized images

Step	Screenshot
W+H, width only
Height only; 50% dropped

Phone viewport and lightbox

Step	Screenshot
360px
Chip on downscaled image
No chip at natural size
Lightbox open
Closed

s2-phone-lightbox-open-close.webm

Styled images

Step	Screenshot
Allowlisted apply, disallowed stripped
Content swapped, none stale

Tables

Step	Screenshot
2, 3 columns
5, 7 columns
360px scroll

QTI sized images

Step	Screenshot
Initial
Answered, hint open

s5-qti-select-submit-hint.webm

QTI consecutive items

Step	Screenshot
Item 1: styled
Item 2: unstyled
Item 3: styled

s6-qti-step-styled-unstyled-styled.webm

QTI then Safe HTML5

Step	Screenshot
QTI
Article
QTI again, unchanged

Interaction chips

Step	Screenshot
Match, 1280px
Match, 360px
Associate, 1280px
Associate, 360px
Associate selected

AI usage

Used Claude Code to plan and implement the sanitizer, layout and chip changes with their tests. Verified with the SafeHTML, QTI viewer and Safe HTML5 viewer Jest suites and prek.

Deviations from the issue spec

Criterion	Why not
width/height on <img> kept "including when only one is set"	Percentage height dropped: it collapses the image.
width/height applied by SafeHTML	Match/associate chips keep their fixed box.

---

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

• Ran pre-flight CI checks (lint, format, tests) and verified all pass
• Rebased onto the target branch and resolved any conflicts
• Reorganized commit history into clean, logical commits
• Audited the diff to ensure only issue-relevant files are changed
• Built PR body from the repository's PR template with evidence blocks

🟡 Waiting for feedback Last updated: 2026-09-26 05:04 UTC

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.20.0a2.dev999_gdd0f3fa05.pex
Windows Installer (EXE)	kolibri-setup-0.20.0a2.dev999+gdd0f3fa05-unsigned.exe
Debian Package	kolibri-server_0.20.0~a2.dev999+gdd0f3fa05-0ubuntu1_all.deb
Debian Package	kolibri_0.20.0a2.dev999+gdd0f3fa05-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.20.0a2.dev999+gdd0f3fa05.dmg
Android Package (APK)	kolibri-0.20.0a2.dev999+gdd0f3fa05-debug.apk
Flatpak bundle (GNOME)	org.learningequality.Kolibri.Devel.flatpak
Raspberry Pi Image	kolibri-pi-image-0.20.0a2.dev999+gdd0f3fa05.zip
TAR file	kolibri-0.20.0a2.dev999+gdd0f3fa05.tar.gz
WHL file	kolibri-0.20.0a2.dev999+gdd0f3fa05-py3-none-any.whl

Smoke test screenshot

Download screenshot

[rtibbles]

This broadly makes sense, and you've properly compensated for why the width and height were originally stripped - but why wouldn't we just add them as props on the image component (and then shouldn't that allow the attributes to stay even if they're forbidden?)

Props alone won't keep them: DOMPurify strips FORBID_ATTR before mapNode reads props, and attributeNameCheck only covers custom elements.

• Have SafeHTML keep any attribute declared as a prop by the component rendering that tag (HTMLComponents and customComponents), then declare width/height on SafeHtmlImage and drop the img-only hook.
• Enforce the forbid list in an uponSanitizeAttribute hook, not with forceKeepAttr, which skips _isValidAttribute and would let the src prop bypass ALLOWED_URI_REGEXP.

The style handling can be simpler too:

• The carried style already reaches the img via v-bind="$attrs"; parseStyleString and the instance.proxy.$attrs workaround only exist to merge it into the image's own :style a second time.
• Move the border into the scoped style as border: 1px solid var(--tokens-fineLine) and drop :style. Same for SafeHtmlTable, after which parseStyleString.js can go.

[rtibbles]

Can you not just add width and height to props?

[rtibblesbot]

Done in 0859fd2: width/height are now props, read in place of $attrs. Searched SafeHTML/*.vue for other $attrs reads: 4 matches; the 2 size reads changed, the 2 style reads (image, table) stay since Vue reserves style as a prop name.

Props alone would not keep them:

• FORBID_ATTR is absolute in DOMPurify: _isValidAttribute returns false before any custom-element check.
• validProps only feeds attributeNameCheck for customComponents tags; img goes through HTMLComponents, which it never reads.
• Sanitising runs before mapNode builds props, so a removed attribute never reaches the component.

So index.js still drops FORBID_ATTR and strips the size attributes from non-img elements in a hook.

[rtibbles]

Only the inline props thread was addressed; the review body still stands:

• Keep any attribute declared as a prop by the tag's component (HTMLComponents and customComponents) via an uponSanitizeAttribute hook, not forceKeepAttr, and drop the img-only hook.
• Drop the $attrs style read, contentStyle and getCurrentInstance from SafeHtmlImage; v-bind="$attrs" already carries the style.
• Move the border into the scoped style as border: 1px solid var(--tokens-fineLine) and drop :style, in both SafeHtmlImage and SafeHtmlTable.
• Delete parseStyleString.js.

[rtibblesbot]

Done in f92ed0c and 282f127.

Prop allowlist:

• Each createSafeHTML builds its own purifier; a forbidden attribute survives only when the component for its tag (HTMLComponents or customComponents) declares it as a prop.
• Enforced in an uponSanitizeAttribute hook; no forceKeepAttr, so src still goes through ALLOWED_URI_REGEXP.
• The img-only hook is gone. New test: a custom element keeps a declared width and loses an undeclared height.

Styles:

• Searched SafeHTML/ for :style merges and parseStyleString imports: 2 each (image, table). Changed all.
• Image border is now 1px solid var(--tokens-fineLine) in the scoped style; :style, contentStyle and the $attrs workaround are gone.
• The table width is still computed from its column count, so it moves to --table-width on the container, where the other table variables already live.
• parseStyleString.js and its spec are deleted.

In the capture, the carried style, the border and the 800px table width (4 columns) all come through without :style:

[rtibbles]

Manually QA'd in the QTI sandbox: all six QA steps pass, including associate chips with sized images. No console errors.