Skip to content

Commit 76328d2

Browse files
committed
fix(html-interactive-content): use classifyAttribute for controls (cross-attr m6/d3 analog)
`controls` on `<audio>`/`<video>` is an HTML boolean attribute, so per the cross-attribute observation in docs/glimmer-attribute-behavior.md, bare `{{false}}` / `{{null}}` / `{{undefined}}` cause Glimmer to omit the attribute at runtime. The helper's previous AST-presence check (hasAttribute) treated `<video controls={{false}}>` as still having controls — a false positive that propagated to every rule using isHtmlInteractiveContent: nested-interactive, no-noninteractive-tabindex, interactive-supports-focus, click-events-have-key-events, etc. After: - `controls` flows through classifyAttribute. Bare-mustache falsy literals now correctly classify as 'absent' → element is NOT interactive at runtime. - `href` and `usemap` continue to use AST-presence — those are plain string attributes that don't falsy-coerce (i4 analog: bare `{{false}}` on a plain string renders as the literal `"false"`, attribute kept). Concat forms (`controls="{{X}}"`) still classify as 'present' because concat is never falsy at runtime — so the existing pass-through behavior for concat is preserved. Tests: - New: `<video controls={{false}}>` → not interactive (regression-locking). - New: `<audio controls={{null}}>` → not interactive. - New: `<video controls="{{false}}">` → IS interactive (concat sets IDL true regardless of inner literal). - All 9555 existing tests pass — no rule was relying on the buggy behavior, so this fix lands cleanly across all consumers.
1 parent d7feb44 commit 76328d2

2 files changed

Lines changed: 68 additions & 2 deletions

File tree

lib/utils/html-interactive-content.js

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
'use strict';
22

3+
const { classifyAttribute } = require('./glimmer-attr-presence');
4+
35
/**
46
* HTML "interactive content" classification, authoritative per
57
* [HTML Living Standard §3.2.5.2.7 Interactive content]
@@ -79,9 +81,18 @@ function isHtmlInteractiveContent(node, getTextAttrValue, options = {}) {
7981
return hasAttribute(node, 'usemap');
8082
}
8183

82-
// audio / video — interactive only when controls is present
84+
// audio / video — interactive only when controls is present.
85+
//
86+
// `controls` is an HTML boolean attribute, so `controls={{false}}` /
87+
// `{{null}}` / `{{undefined}}` cause Glimmer to omit the attribute at
88+
// runtime (rows m6, m9, m10 by attribute-kind analogy + cross-attribute
89+
// observation in docs/glimmer-attribute-behavior.md). AST-presence is
90+
// wrong here — use classifyAttribute so the runtime presence drives the
91+
// answer. `href` / `usemap` are plain string attrs that don't falsy-coerce
92+
// (i4 analog), so AST-presence remains correct for them.
8393
if (tag === 'audio' || tag === 'video') {
84-
return hasAttribute(node, 'controls');
94+
const controlsAttr = node.attributes?.find((a) => a.name === 'controls');
95+
return classifyAttribute(controlsAttr).presence === 'present';
8596
}
8697

8798
return false;

tests/lib/utils/html-interactive-content-test.js

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,61 @@ describe('isHtmlInteractiveContent', () => {
111111
it('<video> without controls is NOT interactive', () => {
112112
expect(isHtmlInteractiveContent(makeNode('video'), getTextAttrValue)).toBe(false);
113113
});
114+
115+
// Bare-mustache falsy on `controls` (per cross-attribute observation in
116+
// docs/glimmer-attribute-behavior.md) causes Glimmer to omit the attribute
117+
// at runtime — so `<video controls={{false}}>` has NO controls and is not
118+
// interactive. Was previously a false positive: AST-presence treated it
119+
// as having controls.
120+
it('<video controls={{false}}> is NOT interactive (Glimmer omits the attribute)', () => {
121+
const node = {
122+
tag: 'video',
123+
attributes: [
124+
{
125+
name: 'controls',
126+
value: {
127+
type: 'GlimmerMustacheStatement',
128+
path: { type: 'GlimmerBooleanLiteral', value: false },
129+
},
130+
},
131+
],
132+
};
133+
expect(isHtmlInteractiveContent(node, getTextAttrValue)).toBe(false);
134+
});
135+
136+
it('<audio controls={{null}}> is NOT interactive', () => {
137+
const node = {
138+
tag: 'audio',
139+
attributes: [
140+
{
141+
name: 'controls',
142+
value: { type: 'GlimmerMustacheStatement', path: { type: 'GlimmerNullLiteral' } },
143+
},
144+
],
145+
};
146+
expect(isHtmlInteractiveContent(node, getTextAttrValue)).toBe(false);
147+
});
148+
149+
it('<video controls="{{false}}"> IS interactive (concat sets IDL true regardless)', () => {
150+
const node = {
151+
tag: 'video',
152+
attributes: [
153+
{
154+
name: 'controls',
155+
value: {
156+
type: 'GlimmerConcatStatement',
157+
parts: [
158+
{
159+
type: 'GlimmerMustacheStatement',
160+
path: { type: 'GlimmerBooleanLiteral', value: false },
161+
},
162+
],
163+
},
164+
},
165+
],
166+
};
167+
expect(isHtmlInteractiveContent(node, getTextAttrValue)).toBe(true);
168+
});
114169
});
115170

116171
describe('elements NOT in §3.2.5.2.7', () => {

0 commit comments

Comments
 (0)