Repository navigation
fix(compiler): add missing Baseline DOM properties to the schema - #71470
aminesbdev wants to merge 1 commit into
Conversation
0f16517 to
ffe1b39
Compare
ffe1b39 to
8ba00a6
Compare
|
Thanks for the detailed review! I pushed an update that covers most of it:
For a few points I wanted to check before changing anything, so I put together a small demo: ngc 22.2.2 with the schema of main, the one of this PR, and this PR with your suggestions applied. I ended up somewhere else on these. I tried the const host = document.createElement('div');
const template = document.createElement('template');
template.shadowRootMode = 'open';
host.append(template);
host.shadowRoot; // nullSo the binding would compile and do nothing. I listed them in the description with the other exclusions instead. About NG0303, the JIT check doesn't read this schema, it looks at On the support window, you're right about For The SVG/MathML inheritance is real for Same for camelCase ARIA in SSR: on main, Last thing, |
|
That what's Gemini doing the review for me, let's smooth out the details :
Keeping the rest seem reasonable. |
8ba00a6 to
7787284
Compare
0a7b09c to
8ccd316
Compare
Binding to a DOM property that is missing from the schema fails with NG8002, even when the property is Baseline. This adds the Baseline properties that were never added to the schema: `command` and `commandForElement` on `button` (invoker commands), `name` on `details`, `autocorrect`, `webkitdirectory` on `input`, `fetchPriority` on `img`, `link` and `script`, `rel` on `form` and SVG `a`, `crossOrigin` on SVG `image`, and the `aria-braillelabel`/`aria-brailleroledescription` attributes. The `q`/`blockquote` entry also listed `cite` as an element instead of a property, so `[cite]` never compiled on those elements. It also adds attribute-to-property mappings for `fetchpriority` and the aria-braille attributes (`aria-braillelabel` and `aria-brailleroledescription`). It also adds the missing events (`animationcancel`, `beforeinput`, `beforematch`, `beforetoggle`, `command`, the composition events, `contentvisibilityautostatechange`, `focusin`, `focusout` and `scrollend`) so the language service can suggest them.
8ccd316 to
aad9b08
Compare
alan-agius4
left a comment
There was a problem hiding this comment.
AGENT: Thanks for putting this together and for the thorough Baseline breakdown. The schema additions, q/blockquote cite fix, and security posture look good overall, though there are a couple of issues to address before we can merge:
- Removing
'fetchpriority'from_ATTR_TO_PROP, sincereify.tsand runtimemapPropNamedon't remap it and[fetchpriority]silently sets a dead expando property at runtime. - Restoring the dropped
language-servicecompletion fix (either here or in a prerequisite PR) so*commandon[HTMLElement]doesn't clobber[command]completions on<button>. - Restoring
!autocorrectso the schema reflects the boolean IDL property.
| export const _ATTR_TO_PROP = new Map( | ||
| Object.entries({ | ||
| 'class': 'className', | ||
| 'fetchpriority': 'fetchPriority', |
There was a problem hiding this comment.
AGENT: Adding 'fetchpriority': 'fetchPriority' here lets <img [fetchpriority]="..."> pass template typechecking (REGISTRY.getMappedPropName), however codegen keeps the raw binding name (mapPropertyName is false in r3_template_transform.ts). Unlike aria-* attributes, entries in _ATTR_TO_PROP also need to exist in DOM_PROPERTY_REMAPPING (packages/compiler/src/template/pipeline/src/phases/reify.ts) and mapPropName (packages/core/src/render3/instructions/shared.ts), neither of which remaps 'fetchpriority'.
As a result, <img [fetchpriority]="'high'"> compiles without NG8002 in AOT and silently writes a dead img.fetchpriority expando property at runtime instead of img.fetchPriority, while logging NG0303 in JIT dev mode.
Other camelCase properties like crossOrigin and referrerPolicy are intentionally omitted here so they are bound as [crossOrigin] or [attr.fetchpriority] without adding extra branches to mapPropName in @angular/core. Let's drop 'fetchpriority' here (and update the commit message accordingly):
| 'fetchpriority': 'fetchPriority', |
| 'base^[HTMLElement]|href,target', | ||
| 'body^[HTMLElement]|aLink,background,bgColor,link,*afterprint,*beforeprint,*beforeunload,*blur,*error,*focus,*hashchange,*languagechange,*load,*message,*messageerror,*offline,*online,*pagehide,*pageshow,*popstate,*rejectionhandled,*resize,*scroll,*storage,*unhandledrejection,*unload,text,vLink', | ||
| 'button^[HTMLElement]|!disabled,formAction,formEnctype,formMethod,!formNoValidate,formTarget,name,popoverTargetAction,%popoverTargetElement,type,value', | ||
| 'button^[HTMLElement]|command,%commandForElement,!disabled,formAction,formEnctype,formMethod,!formNoValidate,formTarget,name,popoverTargetAction,%popoverTargetElement,type,value', |
There was a problem hiding this comment.
AGENT: It looks like the language-service commit (22fb98a668) mentioned in your earlier comment was dropped in the latest force push, and it hasn't landed on main yet.
Because *command is still on [HTMLElement] (line 88), buildAttributeCompletionTable (packages/language-service/src/attribute_completions.ts:417-422) inserts 'command' as DomAttribute for <button> and then overwrites it with DomEvent. That prevents <button [com|]> from suggesting [command] and also lets the 12 new events overwrite same named directive @Input() completions.
If you'd prefer to keep the language-service fix in a separate PR, let's get that landed first (ideally keying DomEvent entries distinctly such as (${event}) so neither [command] nor (command) hides the other), or restore the fix into this PR.
| /* added manually to avoid breaking changes */ | ||
| ',*message,*mozfullscreenchange,*mozfullscreenerror,*mozpointerlockchange,*mozpointerlockerror,*webglcontextcreationerror,*webglcontextlost,*webglcontextrestored', | ||
| '[HTMLElement]^[Element]|accessKey,autocapitalize,!autofocus,contentEditable,dir,!draggable,enterKeyHint,!hidden,!inert,innerText,inputMode,lang,nonce,*abort,*animationend,*animationiteration,*animationstart,*auxclick,*beforexrselect,*blur,*cancel,*canplay,*canplaythrough,*change,*click,*close,*contextmenu,*copy,*cuechange,*cut,*dblclick,*drag,*dragend,*dragenter,*dragleave,*dragover,*dragstart,*drop,*durationchange,*emptied,*ended,*error,*focus,*formdata,*gotpointercapture,*input,*invalid,*keydown,*keypress,*keyup,*load,*loadeddata,*loadedmetadata,*loadstart,*lostpointercapture,*mousedown,*mouseenter,*mouseleave,*mousemove,*mouseout,*mouseover,*mouseup,*mousewheel,*paste,*pause,*play,*playing,*pointercancel,*pointerdown,*pointerenter,*pointerleave,*pointermove,*pointerout,*pointerover,*pointerrawupdate,*pointerup,*progress,*ratechange,*reset,*resize,*scroll,*securitypolicyviolation,*seeked,*seeking,*select,*selectionchange,*selectstart,*slotchange,*stalled,*submit,*suspend,*timeupdate,*toggle,*transitioncancel,*transitionend,*transitionrun,*transitionstart,*volumechange,*waiting,*webkitanimationend,*webkitanimationiteration,*webkitanimationstart,*webkittransitionend,*wheel,outerText,popover,!spellcheck,%style,#tabIndex,title,!translate,virtualKeyboardPolicy', | ||
| '[HTMLElement]^[Element]|accessKey,autocapitalize,autocorrect,!autofocus,contentEditable,dir,!draggable,enterKeyHint,!hidden,!inert,innerText,inputMode,lang,nonce,*abort,*animationcancel,*animationend,*animationiteration,*animationstart,*auxclick,*beforeinput,*beforematch,*beforetoggle,*beforexrselect,*blur,*cancel,*canplay,*canplaythrough,*change,*click,*close,*command,*compositionend,*compositionstart,*compositionupdate,*contentvisibilityautostatechange,*contextmenu,*copy,*cuechange,*cut,*dblclick,*drag,*dragend,*dragenter,*dragleave,*dragover,*dragstart,*drop,*durationchange,*emptied,*ended,*error,*focus,*focusin,*focusout,*formdata,*gotpointercapture,*input,*invalid,*keydown,*keypress,*keyup,*load,*loadeddata,*loadedmetadata,*loadstart,*lostpointercapture,*mousedown,*mouseenter,*mouseleave,*mousemove,*mouseout,*mouseover,*mouseup,*mousewheel,*paste,*pause,*play,*playing,*pointercancel,*pointerdown,*pointerenter,*pointerleave,*pointermove,*pointerout,*pointerover,*pointerrawupdate,*pointerup,*progress,*ratechange,*reset,*resize,*scroll,*scrollend,*securitypolicyviolation,*seeked,*seeking,*select,*selectionchange,*selectstart,*slotchange,*stalled,*submit,*suspend,*timeupdate,*toggle,*transitioncancel,*transitionend,*transitionrun,*transitionstart,*volumechange,*waiting,*webkitanimationend,*webkitanimationiteration,*webkitanimationstart,*webkittransitionend,*wheel,outerText,popover,!spellcheck,%style,#tabIndex,title,!translate,virtualKeyboardPolicy', |
There was a problem hiding this comment.
AGENT: Small catch on autocorrect: even though the HTML attribute takes "on" or "off", SCHEMA models the DOM IDL property type, and HTMLElement.prototype.autocorrect is a boolean in both the HTML spec and TypeScript's lib.dom.d.ts (matching !spellcheck and !translate on this line). Assigning "off" in JS coerces to true, so we should restore the ! prefix here:
| '[HTMLElement]^[Element]|accessKey,autocapitalize,autocorrect,!autofocus,contentEditable,dir,!draggable,enterKeyHint,!hidden,!inert,innerText,inputMode,lang,nonce,*abort,*animationcancel,*animationend,*animationiteration,*animationstart,*auxclick,*beforeinput,*beforematch,*beforetoggle,*beforexrselect,*blur,*cancel,*canplay,*canplaythrough,*change,*click,*close,*command,*compositionend,*compositionstart,*compositionupdate,*contentvisibilityautostatechange,*contextmenu,*copy,*cuechange,*cut,*dblclick,*drag,*dragend,*dragenter,*dragleave,*dragover,*dragstart,*drop,*durationchange,*emptied,*ended,*error,*focus,*focusin,*focusout,*formdata,*gotpointercapture,*input,*invalid,*keydown,*keypress,*keyup,*load,*loadeddata,*loadedmetadata,*loadstart,*lostpointercapture,*mousedown,*mouseenter,*mouseleave,*mousemove,*mouseout,*mouseover,*mouseup,*mousewheel,*paste,*pause,*play,*playing,*pointercancel,*pointerdown,*pointerenter,*pointerleave,*pointermove,*pointerout,*pointerover,*pointerrawupdate,*pointerup,*progress,*ratechange,*reset,*resize,*scroll,*scrollend,*securitypolicyviolation,*seeked,*seeking,*select,*selectionchange,*selectstart,*slotchange,*stalled,*submit,*suspend,*timeupdate,*toggle,*transitioncancel,*transitionend,*transitionrun,*transitionstart,*volumechange,*waiting,*webkitanimationend,*webkitanimationiteration,*webkitanimationstart,*webkittransitionend,*wheel,outerText,popover,!spellcheck,%style,#tabIndex,title,!translate,virtualKeyboardPolicy', | |
| '[HTMLElement]^[Element]|accessKey,autocapitalize,!autocorrect,!autofocus,contentEditable,dir,!draggable,enterKeyHint,!hidden,!inert,innerText,inputMode,lang,nonce,*abort,*animationcancel,*animationend,*animationiteration,*animationstart,*auxclick,*beforeinput,*beforematch,*beforetoggle,*beforexrselect,*blur,*cancel,*canplay,*canplaythrough,*change,*click,*close,*command,*compositionend,*compositionstart,*compositionupdate,*contentvisibilityautostatechange,*contextmenu,*copy,*cuechange,*cut,*dblclick,*drag,*dragend,*dragenter,*dragleave,*dragover,*dragstart,*drop,*durationchange,*emptied,*ended,*error,*focus,*focusin,*focusout,*formdata,*gotpointercapture,*input,*invalid,*keydown,*keypress,*keyup,*load,*loadeddata,*loadedmetadata,*loadstart,*lostpointercapture,*mousedown,*mouseenter,*mouseleave,*mousemove,*mouseout,*mouseover,*mouseup,*mousewheel,*paste,*pause,*play,*playing,*pointercancel,*pointerdown,*pointerenter,*pointerleave,*pointermove,*pointerout,*pointerover,*pointerrawupdate,*pointerup,*progress,*ratechange,*reset,*resize,*scroll,*scrollend,*securitypolicyviolation,*seeked,*seeking,*select,*selectionchange,*selectstart,*slotchange,*stalled,*submit,*suspend,*timeupdate,*toggle,*transitioncancel,*transitionend,*transitionrun,*transitionstart,*volumechange,*waiting,*webkitanimationend,*webkitanimationiteration,*webkitanimationstart,*webkittransitionend,*wheel,outerText,popover,!spellcheck,%style,#tabIndex,title,!translate,virtualKeyboardPolicy', |
| expect(registry.hasProperty('input', 'popoverTargetAction', [])).toBeTruthy(); | ||
| expect(registry.hasProperty('input', 'popoverTargetElement', [])).toBeTruthy(); | ||
| expect(registry.hasProperty('div', 'autocorrect', [])).toBeTruthy(); | ||
| expect(registry.hasProperty('div', 'ariaBrailleLabel', [])).toBeTruthy(); |
There was a problem hiding this comment.
AGENT: Since *command is also registered on [HTMLElement], let's add a check that command isn't exposed as a bindable property on non button elements:
| expect(registry.hasProperty('div', 'ariaBrailleLabel', [])).toBeTruthy(); | |
| expect(registry.hasProperty('button', 'command', [])).toBeTruthy(); | |
| expect(registry.hasProperty('div', 'command', [])).toBeFalsy(); |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The DOM schema is edited by hand now that the extractor is gone. A few Baseline properties are missing from it, so
binding to them fails at build time:
Same for
[commandForElement],[name]ondetails,[autocorrect],[webkitdirectory],[fetchPriority]onimg,[rel]onformand[aria-braillelabel]. The only option today is theattr.prefix.[cite]onqandblockquotehas never compiled. The entry for those elements reads'q,blockquote,cite^[HTMLElement]|',so
citewas added as a third element instead of as their property.Issue Number: N/A
What is the new behavior?
To decide what to add, I listed the writable properties Chrome exposes on each element in the schema (with
createElementNSfor SVG) and compared them with the schema. I then kept the ones that are Baseline according to web-features:command,commandForElementbuttonautocorrectwebkitdirectoryinputrelafetchPriority(+fetchprioritymapping)img,link,scriptnamedetailsariaBrailleLabel,ariaBrailleRoleDescription(+aria-*mapping)crossOriginimagerelformciteq,blockquoteI left
relListout onformand SVGa. It's a read-onlyDOMTokenListthat forwards torel, so[rel]is thebinding to use.
I also added the Baseline events missing from
[HTMLElement](animationcancel,beforeinput,beforematch,beforetoggle,command,compositionstart,compositionupdate,compositionend,contentvisibilityautostatechange,focusin,focusout,scrollend) so the language service suggests them. The MathML entry already lists some of them.Outside of completions, the only place that reads these events is the native-event exemption of the opt-in
strictUnclaimedEventNamescheck. Since it ignores case,(animationCancel)or(focusIn)are now exempt, the same way(animationEnd)already is.I left out properties that aren't Baseline yet (
closedBy,writingSuggestions,interestForElement,blocking,loadingon media) androle. Like the ARIA attributes,roleshould probably be rendered as an attribute for SSR,so it seemed better to handle it separately.
I also left out the declarative shadow DOM properties of
template(shadowRootMode,shadowRootDelegatesFocus,shadowRootClonable,shadowRootSerializable). The browser only attaches a shadow root when the HTML parser seesshadowrootmode. On a<template>created by the renderer, settingshadowRootModeonly sets the attribute (checked inChrome 156), so the binding would compile but never create a shadow root.
On
ngSrcimages,prioritystays the way to set the fetch priority.NgOptimizedImagewritesfetchpriorityitselfin
ngOnInit, so it overrides a[fetchPriority]binding on the same image.On the security side, I didn't add anything to
dom_security_schema.ts. None of the new properties takes HTML oris loaded as a resource.
commandonly acts when the user activates the button, and only on the element incommandForElement. The built-in commands open, close or toggle a dialog or a popover (popoverTargetActionalready does it for popovers) and custom
--commands only dispatch acommandevent, so it can't navigate or runscript.
relonformand SVGachanges how the form or the link navigates (noopener,noreferrer,opener), thesame way
reldoes onaandarea, which are already bindable without a security context.crossOriginon SVGimagesets the CORS mode, likecrossOriginonimg, andfetchPriorityis only a hint.citeholds a URL, butbrowsers neither fetch it nor follow it, and
ins/delalready exposecitewithout a security context.The new assertions in
dom_element_schema_registry_spec.tsfail on main, except the twociteelement checks. Thosemake sure
<cite>stays a known element and doesn't get theciteproperty.Does this PR introduce a breaking change?
This only allows bindings that used to fail with NG8002.
Other information
Like any property in the schema, these bindings set the property even in browsers that don't support it yet, and
nothing happens there, the same as with an attribute the browser doesn't know. In JIT, the runtime check tests
propName in elementin the running browser and doesn't read this schema, so it behaves the same before and after this change.One thing to keep in mind with SSR: apart from
cite, domino doesn't reflect these properties to attributes (I checkedeach one). It doesn't reflect
decodingorinerteither, and both are already in the schema. Like[popover], theseproperties are applied once the app runs in the browser. If the server output needs them, a static attribute or the
attr.prefixstill works.
commandForElementtakes an element, likepopoverTargetElement, so it's meant to be bound to a template reference (#dialog).