Repository navigation
fix: reflect HTML attributes that browsers expose as properties - #54
Conversation
alan-agius4
left a comment
There was a problem hiding this comment.
Thanks for this PR! Reflecting modern attributes like inert, popover, fetchPriority, decoding, and inputMode fixes an important SSR issue where Angular bindings to these DOM properties were not serialized to HTML attributes. Also untangling formAssociatedProps fixes the bug where <fieldset>.type returned 'submit'.
The main concern is that renaming existing properties (autoFocus, autoCapitalize, nomodule) creates a silent breaking change for existing Angular SSR apps and build tools (especially script.nomodule, which could cause browsers to run both modern and legacy bundles).
We should keep backwards-compatible aliases for these. Details inline.
c2e36b9 to
6e5dfb7
Compare
alan-agius4
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous review feedback! Left a few small follow-up notes inline.
| var autocorrectInheritingElements = Object.assign(Object.create(null), { | ||
| "BUTTON": true, "FIELDSET": true, "INPUT": true, "OUTPUT": true, | ||
| "SELECT": true, "TEXTAREA": true | ||
| }); | ||
|
|
||
| function AUTOCORRECT(attr) { | ||
| return { | ||
| get: function() { | ||
| if (this.localName === 'input' && | ||
| (this.type === 'url' || this.type === 'email' || this.type === 'password')) { | ||
| return false; | ||
| } | ||
| var v = this._getattr(attr); | ||
| if (v !== null) { | ||
| v = v.toLowerCase(); | ||
| if (v === 'off') { return false; } | ||
| if (v === 'on' || v === '') { return true; } | ||
| } | ||
| if (this.tagName in autocorrectInheritingElements && this.form) { | ||
| return this.form.autocorrect; | ||
| } |
There was a problem hiding this comment.
Two small things here:
- Per the WHATWG HTML spec, the invalid value default of
autocorrectis theOnstate (true), and step 2 of computing the used autocorrection state returns the state of the element's own attribute whenever the attribute is present. Form inheritance (step 3) only applies whenautocorrectis missing (v === null). Currently,<form autocorrect="off"><input autocorrect="bogus"></form>falls out ofif (v !== null)and inheritsfalsefrom the form instead of returningtrue. We can simplify thev !== nullcheck toreturn v.toLowerCase() !== 'off';. - Nit: Line 63 checks
this.localName === 'input'while line 73 checksthis.tagName in autocorrectInheritingElements. Using lowercase tag names inautocorrectInheritingElementsand checkingthis.localName in autocorrectInheritingElementsavoids computing_tagNameand stays consistent with line 63:
var autocorrectInheritingElements = Object.assign(Object.create(null), {
"button": true, "fieldset": true, "input": true, "output": true,
"select": true, "textarea": true
});
function AUTOCORRECT(attr) {
return {
get: function() {
if (this.localName === 'input' &&
(this.type === 'url' || this.type === 'email' || this.type === 'password')) {
return false;
}
var v = this._getattr(attr);
if (v !== null) {
return v.toLowerCase() !== 'off';
}
if (this.localName in autocorrectInheritingElements && this.form) {
return this.form.autocorrect;
}
return true;
},| name: String, | ||
| value: String, | ||
| disabled: Boolean, | ||
| autofocus: Boolean, |
There was a problem hiding this comment.
Nit: Now that autofocus: Boolean is defined on HTMLElement (line 205), the per-element autofocus: Boolean declarations here on button and on input (line 661), keygen (line 713), select (line 1107), and textarea (line 1308) are redundant and can be removed (similar to how inputMode was removed from input and textarea).
| props: { | ||
| form: formAssociatedProps.form, | ||
| type: { get: function() { return 'output'; } }, | ||
| }, |
There was a problem hiding this comment.
Previously <output> inherited value from <button> via formAssociatedProps. After isolating formAssociatedProps, 'value' in doc.createElement('output') is now false.
Per the HTML spec (and Angular's DomElementSchemaRegistry), HTMLOutputElement exposes value and defaultValue reflecting textContent (just like HTMLTextAreaElement on lines 1288 to 1301). Can we add defaultValue and value here as well so <output [value]="..."> bindings continue to work in SSR?
props: {
form: formAssociatedProps.form,
type: { get: function() { return 'output'; } },
defaultValue: {
get: function() { return this.textContent; },
set: function(v) { this.textContent = v; },
},
value: {
get: function() { return this.defaultValue; },
set: function(v) { this.defaultValue = v; },
},
},| domino.createDocument('<form autocorrect="off"><input></form>') | ||
| .querySelector('input').autocorrect.should.equal(false); |
There was a problem hiding this comment.
Can we also add a test assertion here for <form autocorrect="off"><input autocorrect="bogus"></form> returning true (and a test in formAssociatedElementsDoNotShareReflectedAttributes for output.value)?
The reflected properties were defined as `autoFocus`, `autoCapitalize` and `nomodule`, so setting `el.autofocus`, `el.autocapitalize` or `script.noModule` only created a plain JavaScript property and the attribute was missing from the serialized HTML. The old names are kept as aliases of the standard ones, so code that sets them still reflects the attribute. `autocapitalize` now also returns the same values as browsers: `off` maps to `none`, `on` to `sentences`, and an invalid value to `sentences`. The `autofocus` declarations on `button`, `input`, `keygen`, `select` and `textarea` are removed, since `HTMLElement` now defines it.
`button`, `fieldset`, `keygen`, `label`, `meter`, `object`, `output` and `progress` all passed the same `formAssociatedProps` object to `define()`, which adds each element's reflected attributes to it. Every element defined later inherited the attributes of the earlier ones, so `progress` exposed `formAction`, `data` and `useMap`, `label` had `value` and `disabled`, and `fieldset.type` returned `submit`. Each of these elements now gets its own property object, like `input`, `select` and `textarea` already do. The properties that only worked through the leak are now declared on the element that owns them: `value` on `meter` and `progress`, `htmlFor` on `output`, and the read-only `type` of `fieldset` and `output`. `output` also gets `value` and `defaultValue`, which set its text content like on `textarea`, instead of the `value` attribute it used to get from `button`.
Several properties that browsers reflect to attributes were missing, so
setting them only created a plain JavaScript property and the attribute was
missing from the serialized HTML. Angular sets DOM property bindings this
way during server-side rendering, so for example `[popover]`, `[inert]` or
`[max]` on `<meter>` were missing from the server output.
This adds:
- `autocorrect`, `inert`, `inputMode` and `popover` on all HTML elements
(`inputMode` replaces the obsolete keywords that `input` and `textarea`
had)
- `decoding` and `fetchPriority` on `img`, `fetchPriority` on `link` and
`script`
- `command` and `popoverTargetAction` on `button`, `formAction`,
`popoverTargetAction` and `webkitdirectory` on `input`
- `name` on `details`, `rel` on `form`, `headers` on `td` and `th`
- `playsInline` and `disablePictureInPicture` on `video`,
`disableRemotePlayback` on `audio` and `video`
- `min`, `max`, `low`, `high` and `optimum` on `meter`
Enumerated attributes can now be nullable, so that `el.popover = null`
removes the attribute. Their `{value, alias}` entries are now keyed by their
value: `val.value || val` stored `{value: "", alias: "auto"}` under
"[object Object]", so an empty `popover` attribute returned the invalid
value default instead of `auto`.
6e5dfb7 to
1cacfd3
Compare
Angular applies DOM property bindings during SSR by setting the property on the domino element. Several properties that
browsers reflect to attributes are missing or misnamed here, so their value never reaches the serialized HTML. For
example,
[popover],[inert]and[max]on<meter>are missing from the server output, which the browser showsuntil hydration, and which content in
@defer (hydrate never)keeps.There are three commits:
autofocus,autocapitalizeandnoModulewere defined asautoFocus,autoCapitalizeandnomodule, so thestandard names didn't reflect anything. The old names stay as aliases, so code that sets them keeps working.
autocapitalizealso gets the browser getter (off→none,on→sentences).button,fieldset,keygen,label,meter,object,outputandprogresspassed the sameformAssociatedPropsobject todefine(), which adds each element's attributes to it, so every element inheritedthe attributes of the ones defined before it (
fieldset.typewas"submit",progresshadformActionanddata). Each one now gets its own object, likeinput,selectandtextarea. The properties that only workedthrough that leak are declared where they belong:
valueonmeterandprogress,htmlForonoutput, andthe read-only
typeoffieldsetandoutput.outputalso getsvalueanddefaultValue, which set its textcontent like on
textarea, instead of thevalueattribute it got frombutton. Without this, adding attributes tobuttonin the next commitwould have leaked them to six other elements.
autocorrect,inert,inputModeandpopoveron all HTML elements,decodingandfetchPriorityonimg,fetchPriorityonlinkandscript,commandandpopoverTargetActiononbutton,formAction,popoverTargetActionandwebkitdirectoryoninput,nameondetails,relonform,headerson table cells,
playsInlineanddisablePictureInPictureonvideo,disableRemotePlaybackon media elements,and
min/max/low/high/optimumonmeter. Most of them are plain reflections.popoveris nullable(setting it to
nullremoves the attribute), which also needed enumerated values like{value: "", alias: ...}to be keyed by their value rather than by the object, and
autocorrectis a boolean over theon/offattribute.
inputModereplaces the obsolete keywords thatinputandtextareahad.I left out
contentEditable(its setter validates and throws),slot,partand the token lists (relList,controlsList), to keep this to plain reflected attributes.The new cases are in
test/domino.js. I didn't touchweb-platform-blocklist.json, since the WPT harness isn't runanymore.
Fixes angular/angular#71603