Skip to content

Commit

Permalink
fix(engine-core): fix non-native ARIA reflection [backport] (#3919)
Browse files Browse the repository at this point in the history
  • Loading branch information
nolanlawson committed Jan 9, 2024
1 parent 4590a9d commit f8732a1
Show file tree
Hide file tree
Showing 16 changed files with 806 additions and 55 deletions.
41 changes: 23 additions & 18 deletions packages/@lwc/engine-core/src/framework/base-bridge-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
22 changes: 18 additions & 4 deletions packages/@lwc/engine-core/src/framework/base-lightning-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
create,
defineProperties,
defineProperty,
entries,
freeze,
hasOwnProperty,
isFunction,
Expand All @@ -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';
Expand Down Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
});
});
});
});
});
});
});
});
Loading

0 comments on commit f8732a1

Please sign in to comment.