Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions blank.ts
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ interface Context {
renames: Array<[number, number, string]>;
fullyBlankedRanges: Range[];
dynamicContentOffsets: number[];
imgSplatOffsets: number[];
effectiveComponentAttrMap?: Map<string, ComponentAttrs>;
// When set, `handleBlockStatement` uses the selection from this map
// (keyed by the BlockStatement's source-start offset) instead of the
Expand Down Expand Up @@ -667,6 +668,35 @@ function substituteSelfClosingVoidComponent(
//
// No-op when no suitable attr area is found — `no-implicit-input-type`
// will still fire in that case, but the user can silence per-site.

// Symmetric to tryInjectInputType but for void natives whose required attrs
// can be supplied by the consumer via `...attributes`. Without injection,
// `<img ...attributes>` blanks to an attribute-less `<img>` and html-validate
// FP-fires `element-required-attributes` (src) and `wcag/h37` (alt) even
// though both come from the splat at runtime.
//
// We don't rewrite the source — the minimal `...attributes` slot is 13
// chars and html-validate accepts no two-attr form that fits (bare
// `src alt` triggers `attribute-allowed-values`-missing-value, empty
// quoted `src=''` triggers `attribute-allowed-values`-invalid-value,
// and the wide whitespace form `src=' ' alt=' '` is 19+ chars).
//
// Instead, record the element's start offset; the transformer's
// `processElement` hook reads this list and calls `setAttribute` with
// a DynamicValue at parse time, sidestepping source-side slot width
// entirely. Skipped when the consumer already wrote `src=` / `alt=`
// explicitly (no FP to suppress).
//
// Currently scoped to <img>; the same shape applies to
// <source>/<track>/<area>/<iframe> if real-world FPs surface there.
function tryInjectImgRequiredAttrs(node: AST.ElementNode, ctx: Context): void {
const hasSplat = (node.attributes ?? []).some((a) => a.name === '...attributes');
if (!hasSplat) return;
const present = new Set((node.attributes ?? []).map((a) => a.name));
if (present.has('src') && present.has('alt')) return;
ctx.imgSplatOffsets.push(startOffset(node));
}

function tryInjectInputType(node: AST.ElementNode, ctx: Context): void {
const literalType = lookupComponentAttr(node, ctx, 'type');
// Build the injected text. Prefer a literal value when known and
Expand Down Expand Up @@ -1018,6 +1048,12 @@ function handleElementNode(node: AST.ElementNode, ctx: Context): void {
if (elementHasDynamicContent(node)) {
ctx.dynamicContentOffsets.push(start);
}
// For void natives whose required attrs can come from `...attributes`,
// inject placeholders before the splat is blanked. Currently <img>; see
// tryInjectImgRequiredAttrs for the rationale and scope.
if (effectiveTag === 'img') {
tryInjectImgRequiredAttrs(node, ctx);
}
for (const attr of node.attributes ?? []) {
emitAttribute(attr, ctx, effectiveTag);
}
Expand Down Expand Up @@ -1170,12 +1206,21 @@ export interface BlankResult {
content: string;
error: Error | null;
dynamicContentOffsets: number[];
// Offsets of `<img ...attributes>` elements (start of `<` byte) where
// the consumer-side `...attributes` is expected to provide required
// attrs (src/alt) at runtime. The transformer's processElement hook
// synthesizes these attrs as DynamicValue at parse-time so html-
// validate sees them as "present, value unknowable" — sidesteps the
// narrow-slot problem where source-side rewrite can't fit two 9-char
// `attr=' '` placeholders into a 13-char `...attributes` slot.
imgSplatOffsets: number[];
}

export interface BlankErrorResult {
content: string;
error: Error;
dynamicContentOffsets?: undefined;
imgSplatOffsets?: undefined;
}

function blankTemplateContent(
Expand Down Expand Up @@ -1213,6 +1258,7 @@ function blankTemplateContent(
renames: [],
fullyBlankedRanges: [],
dynamicContentOffsets: [],
imgSplatOffsets: [],
branchSelections,
inFullyBlankedRange(offset: number): boolean {
for (const [s, e] of ctx.fullyBlankedRanges) {
Expand Down Expand Up @@ -1275,6 +1321,7 @@ function blankTemplateContent(
content: buf.join(''),
error: null,
dynamicContentOffsets: ctx.dynamicContentOffsets,
imgSplatOffsets: ctx.imgSplatOffsets,
};
}

Expand Down
13 changes: 13 additions & 0 deletions examples/img-splat-thin-wrapper.gjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Thin <img> wrapper: parent provides src + alt via `...attributes`.
// Mirrors the super-rentals `rental/image.gjs` pattern that surfaced
// the narrow-slot FP — a single `...attributes` slot can't fit two
// 9-char `attr=' '` placeholders, so prior versions only synthesized
// `src` source-side and wcag/h37 / element-required-attributes (alt)
// FP-fired. The hook-time injection sidesteps the slot-width problem.
import Component from '@glimmer/component';

export default class ThinImg extends Component {
<template>
<img ...attributes>
</template>
}
55 changes: 55 additions & 0 deletions test/blank.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,61 @@ describe('mustache blanking', () => {
expect(out.content).toHaveLength(src.length);
expect(out.content).not.toContain('...attributes');
});

it('records `<img ...attributes>` start offset for hook-time src/alt injection', () => {
// `<img ...attributes>` in a thin wrapper component (parent supplies
// src AND alt via splat). The blanker erases `...attributes` to
// whitespace; the transformer's `processElement` hook is responsible
// for synthesizing src and alt as DynamicValue at parse time. Here
// we just assert that the offset is recorded so the hook knows
// which `<img>` to augment.
//
// Why hook-time (not source-rewrite): the minimal `...attributes`
// slot is 13 chars, but no two-attr form html-validate accepts fits
// (bare `src alt` triggers attribute-allowed-values-missing-value;
// empty quoted `src=''` triggers attribute-allowed-values-invalid;
// wide `src=' ' alt=' '` is 19+ chars). Hook-time setAttribute
// bypasses the slot-width problem.
const src = '<img ...attributes>';
const out = blank(src);
expect(out.content).toHaveLength(src.length);
expect(out.content).not.toContain('...attributes');
expect(
out.imgSplatOffsets,
`expected the <img ...attributes> offset to be recorded for hook-time injection`,
).toEqual([0]);
});
Comment thread
johanrd marked this conversation as resolved.

it('records the offset even when there are multiple Glimmer-only slots', () => {
// Real-world `<img>` invocations often have @args / {{on …}} alongside
// `...attributes`. The hook still needs to know about this img so it
// can synthesize src/alt — same mechanism regardless of slot count.
const src = '<img @loading="lazy" {{on "load" this.h}} ...attributes>';
const out = blank(src);
expect(out.content).toHaveLength(src.length);
expect(out.imgSplatOffsets).toEqual([0]);
});

it('does not record offset when the consumer wrote both src and alt explicitly', () => {
// When src AND alt are both already on the element, no injection is
// needed — leave the offset out of the set so the hook skips it.
const src = '<img src="/foo.png" alt="bar" {{on "load" this.h}} ...attributes>';
const out = blank(src);
expect(out.content).toHaveLength(src.length);
// Original literal values survive.
expect(out.content).toContain('src="/foo.png"');
expect(out.content).toContain('alt="bar"');
expect(out.imgSplatOffsets).toEqual([]);
});

it('records offset when only ONE of src/alt is consumer-written (other still synthesized)', () => {
// When the consumer wrote only `src=` but not `alt=`, alt still needs
// synthesis — record the offset; the hook checks per-attr presence
// and synthesizes only the missing one.
const src = '<img src="/foo.png" {{on "load" this.h}} ...attributes>';
const out = blank(src);
expect(out.imgSplatOffsets).toEqual([0]);
});
Comment thread
johanrd marked this conversation as resolved.
});

describe('component substitution (transparent fallback)', () => {
Expand Down
18 changes: 18 additions & 0 deletions test/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,24 @@ describe('end-to-end fixtures', () => {
expect(r.valid).toBe(true);
});

it('img-splat-thin-wrapper: `<img ...attributes>` does not FP-fire wcag/h37 or element-required-attributes', async () => {
// Thin <img> wrapper component — parent provides src + alt via the
// splat. The `...attributes` slot is 13 chars, too narrow to source-
// rewrite both 9-char `attr=' '` placeholders. Hook-time setAttribute
// synthesizes src + alt as DynamicValue at parse time so wcag/h37
// (missing alt) and element-required-attributes (missing src) don't
// fire on what the consumer actually fills in. Mirrors super-rentals'
// `rental/image.gjs`.
const r = await validate('img-splat-thin-wrapper.gjs', { 'void-style': 'off' });
const offenders = r.messages.filter(
(m) => m.rule === 'wcag/h37' || m.rule === 'element-required-attributes',
);
expect(
offenders,
`wcag/h37 / element-required-attributes must not fire on <img ...attributes>; got: ${JSON.stringify(r.messages)}`,
).toHaveLength(0);
});

it('linkto-aria-label: aria-label on <LinkTo> does not fire aria-label-misuse', async () => {
// <LinkTo> is substituted to <a> via the built-in components map,
// and block-form substitution injects an href placeholder so the
Expand Down
44 changes: 41 additions & 3 deletions transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,11 @@ function offsetToLineCol(source: string, offset: number): { line: number; column
return { line, column };
}

function makeHooks(dynamicSet: ReadonlySet<number>, startOffset: number): SourceHooks {
function makeHooks(
dynamicSet: ReadonlySet<number>,
imgSplatSet: ReadonlySet<number>,
startOffset: number,
): SourceHooks {
const processAttribute: ProcessAttributeCallback = (attr: AttributeData) => {
// Bare-mustache attribute values (`id={{x}}`) are emitted as
// `id="<spaces>"` (see blank.ts). Detect that pattern and replace
Expand Down Expand Up @@ -150,6 +154,32 @@ function makeHooks(dynamicSet: ReadonlySet<number>, startOffset: number): Source
location,
);
}
// Inject required attrs synthetically for `<img ...attributes>`.
// The blanker erases `...attributes` from the source but records
// the element's start offset; we honor that here by adding a
// DynamicValue-backed `src` and/or `alt` if the consumer didn't
// already write them. Sidesteps the source-side slot-width
// problem (a 13-char `...attributes` slot can't fit two valid
// `attr=value` placeholders that html-validate accepts).
if (
imgSplatSet.has(templateRelativeOffset) &&
(el as unknown as { tagName?: string }).tagName === 'img'
) {
const elWithAttrs = el as unknown as {
hasAttribute(name: string): boolean;
setAttribute(
name: string,
value: unknown,
keyLocation: unknown,
valueLocation: unknown,
): void;
};
for (const name of ['src', 'alt']) {
if (!elWithAttrs.hasAttribute(name)) {
elWithAttrs.setAttribute(name, new DynamicValue(''), location, location);
}
}
}
};

return { processAttribute, processElement };
Expand Down Expand Up @@ -192,7 +222,11 @@ function* transformGlimmer(source: Source): Generator<Source, void, unknown> {
column: 1,
offset: 0,
originalData,
hooks: makeHooks(new Set(result.dynamicContentOffsets ?? []), 0),
hooks: makeHooks(
new Set(result.dynamicContentOffsets ?? []),
new Set(result.imgSplatOffsets ?? []),
0,
),
};
return;
}
Expand Down Expand Up @@ -311,7 +345,11 @@ function* transformGlimmer(source: Source): Generator<Source, void, unknown> {
column: sourceColumn,
offset: sourceOffset,
originalData,
hooks: makeHooks(new Set(result.dynamicContentOffsets ?? []), startOffset),
hooks: makeHooks(
new Set(result.dynamicContentOffsets ?? []),
new Set(result.imgSplatOffsets ?? []),
startOffset,
),
};
}
}
Expand Down
Loading