fix(engine-core): fix non-native ARIA reflection [backport]

packages/@lwc/engine-core/src/framework/base-bridge-element.ts

@@ -23,7 +23,7 @@ import {
     htmlPropertyToAttribute,
     isNull,
 } from '@lwc/shared';
-import { applyAriaReflection } from '../libs/aria-reflection/aria-reflection';
+import { ariaReflectionPolyfillDescriptors } from '../libs/aria-reflection/aria-reflection';
 import { logWarn } from '../shared/logger';
 import { getAssociatedVM } from './vm';
 import { getReadOnlyProxy } from './membrane';
@@ -164,7 +164,11 @@ export function HTMLBridgeElementFactory(
                     // and can break tooling that expects it to be iterable or defined, e.g. Jest:
                     // https://github.com/jestjs/jest/blob/b4c9587/packages/pretty-format/src/plugins/DOMElement.ts#L95
                     // It also doesn't make sense to override e.g. "constructor".
-                    .filter((propName) => !(propName in HTMLElementPrototype))
+                    .filter(
+                        (propName) =>
+                            !(propName in HTMLElementPrototype) &&
+                            !(propName in ariaReflectionPolyfillDescriptors)
+                    )
             );
 
             for (const propName of nonPublicPropertiesToWarnOn) {
@@ -250,29 +254,30 @@ export function HTMLBridgeElementFactory(
     return HTMLBridgeElement as HTMLElementConstructor;
 }
 
+// We do some special handling of non-standard ARIA props like ariaLabelledBy as well as props without (as of this
+// writing) broad cross-browser support like ariaBrailleLabel. This is so the reflection works correctly and preserves
+// backwards compatibility with the previous global polyfill approach.
+//
+// The goal here is to expose `elm.aria*` property accessors to work from outside a component, and to reflect `aria-*`
+// attrs. This is especially important because the template compiler compiles aria-* attrs on components to aria* props.
+// Note this works regardless of whether the global ARIA reflection polyfill is applied or not.
+//
+// Also note this ARIA reflection only really makes sense in the browser. On the server, there is no
+// `renderedCallback()`, so you cannot do e.g. `this.template.querySelector('x-child').ariaBusy = 'true'`. So we don't
+// need to expose ARIA props outside the LightningElement
+const basePublicProperties = [
+    ...getOwnPropertyNames(HTMLElementOriginalDescriptors),
+    ...(process.env.IS_BROWSER ? getOwnPropertyNames(ariaReflectionPolyfillDescriptors) : []),
+];
+
 export const BaseBridgeElement = HTMLBridgeElementFactory(
     HTMLElementConstructor,
-    getOwnPropertyNames(HTMLElementOriginalDescriptors),
+    basePublicProperties,
     [],
     [],
     null,
     false
 );
 
-if (process.env.IS_BROWSER) {
-    // This ARIA reflection only really makes sense in the browser. On the server, there is no `renderedCallback()`,
-    // so you cannot do e.g. `this.template.querySelector('x-child').ariaBusy = 'true'`. So we don't need to expose
-    // ARIA props outside the LightningElement
-    //
-    // Apply ARIA reflection to HTMLBridgeElement.prototype. This allows `elm.aria*` property accessors to work from
-    // outside a component, and to reflect `aria-*` attrs. This is especially important because the template compiler
-    // compiles aria-* attrs on components to aria* props.
-    // Note this works regardless of whether the global ARIA reflection polyfill is applied or not.
-    //
-    // Also note that we apply this to BaseBridgeElement.prototype to avoid excessively redefining property
-    // accessors inside the HTMLBridgeElementFactory.
-    applyAriaReflection(BaseBridgeElement.prototype);
-}
-
 freeze(BaseBridgeElement);
 seal(BaseBridgeElement.prototype);

packages/@lwc/engine-core/src/framework/base-lightning-element.ts

@@ -17,6 +17,7 @@ import {
     create,
     defineProperties,
     defineProperty,
+    entries,
     freeze,
     hasOwnProperty,
     isFunction,
@@ -31,7 +32,7 @@ import {
 
 import { logError, logWarn } from '../shared/logger';
 import { getComponentTag } from '../shared/format';
-import { applyAriaReflection } from '../libs/aria-reflection/aria-reflection';
+import { ariaReflectionPolyfillDescriptors } from '../libs/aria-reflection/aria-reflection';
 
 import { HTMLElementOriginalDescriptors } from './html-properties';
 import { getWrappedComponentsListener } from './component';
@@ -814,12 +815,25 @@ for (const propName in HTMLElementOriginalDescriptors) {
     );
 }
 
-defineProperties(LightningElement.prototype, lightningBasedDescriptors);
-
 // Apply ARIA reflection to LightningElement.prototype, on both the browser and server.
 // This allows `this.aria*` property accessors to work from inside a component, and to reflect `aria-*` attrs.
 // Note this works regardless of whether the global ARIA reflection polyfill is applied or not.
-applyAriaReflection(LightningElement.prototype);
+if (process.env.IS_BROWSER) {
+    // In the browser, we use createBridgeToElementDescriptor, so we can get the normal reactivity lifecycle for
+    // aria* properties
+    for (const [propName, descriptor] of entries(ariaReflectionPolyfillDescriptors) as [
+        name: string,
+        descriptor: PropertyDescriptor
+    ][]) {
+        lightningBasedDescriptors[propName] = createBridgeToElementDescriptor(propName, descriptor);
+    }
+} else {
+    // On the server, we cannot use createBridgeToElementDescriptor because getAttribute/setAttribute are
+    // not supported on HTMLElement. So apply the polyfill directly on top of LightningElement
+    defineProperties(LightningElement.prototype, ariaReflectionPolyfillDescriptors);
+}
+
+defineProperties(LightningElement.prototype, lightningBasedDescriptors);
 
 defineProperty(LightningElement, 'CustomElementConstructor', {
     get() {

packages/@lwc/engine-core/src/libs/aria-reflection/aria-reflection.ts

@@ -6,42 +6,41 @@
  */
 import {
     AriaPropNameToAttrNameMap,
-    defineProperty,
-    getOwnPropertyDescriptor,
     isNull,
     isUndefined,
-    keys,
+    create,
+    getPropertyDescriptor,
+    entries,
 } from '@lwc/shared';
-import { LightningElement } from '../../framework/base-lightning-element';
+import { HTMLElementPrototype } from '../../framework/html-element';
 
 // Apply ARIA string reflection behavior to a prototype.
 // This is deliberately kept separate from @lwc/aria-reflection. @lwc/aria-reflection is a global polyfill that is
-// needed for backwards compatibility in LEX, whereas `applyAriaReflection` is designed to only apply to our own
+// needed for backwards compatibility in LEX, whereas this is designed to only apply to our own
 // LightningElement/BaseBridgeElement prototypes.
-export function applyAriaReflection(prototype: HTMLElement | LightningElement) {
-    for (const propName of keys(AriaPropNameToAttrNameMap)) {
-        const attrName = AriaPropNameToAttrNameMap[propName];
-        if (isUndefined(getOwnPropertyDescriptor(prototype, propName))) {
-            // Note that we need to call this.{get,set,has,remove}Attribute rather than dereferencing
-            // from Element.prototype, because these methods are overridden in LightningElement.
-            defineProperty(prototype, propName, {
-                get(this: HTMLElement): any {
-                    return this.getAttribute(attrName);
-                },
-                set(this: HTMLElement, newValue: any) {
-                    // TODO [#3284]: there is disagreement between browsers and the spec on how to treat undefined
-                    // Our historical behavior is to only treat null as removing the attribute
-                    // See also https://github.com/w3c/aria/issues/1858
-                    if (isNull(newValue)) {
-                        this.removeAttribute(attrName);
-                    } else {
-                        this.setAttribute(attrName, newValue);
-                    }
-                },
-                // configurable and enumerable to allow it to be overridden – this mimics Safari's/Chrome's behavior
-                configurable: true,
-                enumerable: true,
-            });
-        }
+// Note we only need to handle ARIA reflections that aren't already in Element.prototype
+export const ariaReflectionPolyfillDescriptors = create(null);
+for (const [propName, attrName] of entries(AriaPropNameToAttrNameMap)) {
+    if (isUndefined(getPropertyDescriptor(HTMLElementPrototype, propName))) {
+        // Note that we need to call this.{get,set,has,remove}Attribute rather than dereferencing
+        // from Element.prototype, because these methods are overridden in LightningElement.
+        ariaReflectionPolyfillDescriptors[propName] = {
+            get(this: HTMLElement): any {
+                return this.getAttribute(attrName);
+            },
+            set(this: HTMLElement, newValue: any) {
+                // TODO [#3284]: there is disagreement between browsers and the spec on how to treat undefined
+                // Our historical behavior is to only treat null as removing the attribute
+                // See also https://github.com/w3c/aria/issues/1858
+                if (isNull(newValue)) {
+                    this.removeAttribute(attrName);
+                } else {
+                    this.setAttribute(attrName, newValue);
+                }
+            },
+            // configurable and enumerable to allow it to be overridden – this mimics Safari's/Chrome's behavior
+            configurable: true,
+            enumerable: true,
+        };
     }
 }

packages/@lwc/integration-karma/test/api/getComponentDef/index.spec.js

@@ -65,9 +65,7 @@ const GLOBAL_HTML_ATTRIBUTES = [
     'spellcheck',
     'tabIndex',
     'title',
-    // Copy over all aria props supported on Element.prototype. Note that this will vary from browser to browser.
-    // See: https://wicg.github.io/aom/spec/aria-reflection.html
-    ...Object.keys(Element.prototype).filter((prop) => ariaProperties.includes(prop)),
+    ...ariaProperties,
 ].sort();
 
 const message = (propName) =>

packages/@lwc/integration-karma/test/component/aria-reflection/index.spec.js

@@ -0,0 +1,183 @@
+import { createElement } from 'lwc';
+import { ariaPropertiesMapping, extractDataIds } from 'test-utils';
+import NoPropDeclared from 'x/noPropDeclared';
+import PropDeclared from 'x/propDeclared';
+import ApiPropDeclared from 'x/apiPropDeclared';
+import TrackPropDeclared from 'x/trackPropDeclared';
+import NoPropDeclaredNoSuper from 'x/noPropDeclaredNoSuper';
+import PropDeclaredNoSuper from 'x/propDeclaredNoSuper';
+import ApiPropDeclaredNoSuper from 'x/apiPropDeclaredNoSuper';
+import TrackPropDeclaredNoSuper from 'x/trackPropDeclaredNoSuper';
+
+describe('aria reflection', () => {
+    // Test with and without a custom superclass, since we may set the property accessor differently in each case
+    const variants = [
+        {
+            name: 'has custom superclass',
+            components: {
+                NoPropDeclared: {
+                    tagName: 'x-no-prop-declared',
+                    Ctor: NoPropDeclared,
+                },
+                PropDeclared: {
+                    tagName: 'x-prop-declared',
+                    Ctor: PropDeclared,
+                },
+                ApiPropDeclared: {
+                    tagName: 'x-api-prop-declared',
+                    Ctor: ApiPropDeclared,
+                },
+                TrackPropDeclared: {
+                    tagName: 'x-track-prop-declared',
+                    Ctor: TrackPropDeclared,
+                },
+            },
+        },
+        {
+            name: 'no custom superclass',
+            components: {
+                NoPropDeclared: {
+                    tagName: 'x-no-prop-declared-no-super',
+                    Ctor: NoPropDeclaredNoSuper,
+                },
+                PropDeclared: {
+                    tagName: 'x-prop-declared-no-super',
+                    Ctor: PropDeclaredNoSuper,
+                },
+                ApiPropDeclared: {
+                    tagName: 'x-api-prop-declared-no-super',
+                    Ctor: ApiPropDeclaredNoSuper,
+                },
+                TrackPropDeclared: {
+                    tagName: 'x-track-prop-declared-no-super',
+                    Ctor: TrackPropDeclaredNoSuper,
+                },
+            },
+        },
+    ];
+
+    const scenarios = [
+        {
+            name: 'No prop declared',
+            componentKey: 'NoPropDeclared',
+            expectAttrReflection: true,
+        },
+        {
+            name: 'Prop declared',
+            componentKey: 'PropDeclared',
+            // declaring a prop in the component results in no attribute reflection
+            expectAttrReflection: false,
+        },
+        {
+            name: '@api prop declared',
+            componentKey: 'ApiPropDeclared',
+            // declaring a prop in the component results in no attribute reflection
+            expectAttrReflection: false,
+        },
+        {
+            name: '@track prop declared',
+            componentKey: 'TrackPropDeclared',
+            // declaring a prop in the component results in no attribute reflection
+            expectAttrReflection: false,
+        },
+    ];
+
+    scenarios.forEach(({ name: scenarioName, componentKey, expectAttrReflection }) => {
+        describe(scenarioName, () => {
+            variants.forEach(({ name: variantName, components }) => {
+                describe(variantName, () => {
+                    const { tagName, Ctor } = components[componentKey];
+
+                    Object.entries(ariaPropertiesMapping).forEach(([propName, attrName]) => {
+                        function validateAria(elm, expected) {
+                            const dataIds = extractDataIds(elm);
+
+                            // rendering the prop works
+                            expect(dataIds[propName].textContent).toBe(
+                                expected === null ? '' : expected
+                            );
+
+                            // the property is correct
+                            expect(elm[propName]).toBe(expected);
+                            expect(elm.getPropInternal(propName)).toBe(expected);
+
+                            // the attr is reflected (if we expect that to work)
+                            expect(elm.getAttribute(attrName)).toBe(
+                                expectAttrReflection ? expected : null
+                            );
+                            expect(elm.getAttrInternal(attrName)).toBe(
+                                expectAttrReflection ? expected : null
+                            );
+                        }
+
+                        describe(propName, () => {
+                            it('no initial value', async () => {
+                                const elm = createElement(tagName, { is: Ctor });
+
+                                document.body.appendChild(elm);
+
+                                await Promise.resolve();
+                                validateAria(elm, null);
+                                expect(elm.renderCount).toBe(1);
+                            });
+
+                            it('set externally', async () => {
+                                const elm = createElement(tagName, { is: Ctor });
+
+                                // set initial prop before rendering
+                                elm[propName] = 'foo';
+
+                                document.body.appendChild(elm);
+
+                                await Promise.resolve();
+                                validateAria(elm, 'foo');
+                                expect(elm.renderCount).toBe(1);
+
+                                // mutate prop
+                                elm[propName] = 'bar';
+
+                                await Promise.resolve();
+                                validateAria(elm, 'bar');
+                                expect(elm.renderCount).toBe(2);
+
+                                // remove
+                                elm[propName] = null;
+
+                                await Promise.resolve();
+                                validateAria(elm, null);
+                                expect(elm.renderCount).toBe(3);
+                            });
+
+                            it('set internally', async () => {
+                                const elm = createElement(tagName, { is: Ctor });
+
+                                // set initial prop before rendering
+                                elm.setPropInternal(propName, 'foo');
+
+                                document.body.appendChild(elm);
+
+                                await Promise.resolve();
+                                validateAria(elm, 'foo');
+                                expect(elm.renderCount).toBe(1);
+
+                                // mutate prop
+                                elm.setPropInternal(propName, 'bar');
+
+                                await Promise.resolve();
+                                validateAria(elm, 'bar');
+                                expect(elm.renderCount).toBe(2);
+
+                                // remove
+                                elm.setPropInternal(propName, null);
+
+                                await Promise.resolve();
+                                validateAria(elm, null);
+                                expect(elm.renderCount).toBe(3);
+                            });
+                        });
+                    });
+                });
+            });
+        });
+    });
+});