Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(engine-dom): remove ARIA reflection global polyfill (BREAKING CHANGE) #3666

Merged
merged 24 commits into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
6c913e1
fix(engine-dom): disable ARIA reflection polyfill (BREAKING CHANGE)
nolanlawson Aug 8, 2023
27fe2a4
chore: clearer name
nolanlawson Aug 9, 2023
32c0c7c
fix: update types.ts
nolanlawson Aug 9, 2023
827c800
Merge remote-tracking branch 'origin/milestone-v4.0.0' into nolan/def…
nolanlawson Aug 10, 2023
d47b1a2
Merge remote-tracking branch 'origin/nolan/default-aria-reflection-of…
nolanlawson Aug 10, 2023
d62fe1d
chore: empty commit
nolanlawson Aug 10, 2023
f5bc994
chore: bump ci
nolanlawson Aug 10, 2023
774d91b
Merge remote-tracking branch 'origin/master' into nolan/default-aria-…
nolanlawson Aug 22, 2023
4cafff8
fix: remove the global polyfill for real
nolanlawson Aug 22, 2023
7e5c53a
fix: header
nolanlawson Aug 22, 2023
5e14ddf
Merge remote-tracking branch 'origin/master' into nolan/default-aria-…
nolanlawson Aug 24, 2023
953df88
fix: remove @lwc/aria-reflection entirely
nolanlawson Aug 24, 2023
88dd671
fix: detect before patching
nolanlawson Aug 24, 2023
4068dfb
Merge remote-tracking branch 'origin/master' into nolan/default-aria-…
nolanlawson Aug 24, 2023
021998e
fix: retain @lwc/aria-reflection
nolanlawson Aug 25, 2023
e7949e6
fix: fix comments
nolanlawson Aug 25, 2023
c14333e
Merge remote-tracking branch 'origin/master' into nolan/default-aria-…
nolanlawson Sep 1, 2023
540eeb7
fix: tighten prototype types
nolanlawson Sep 1, 2023
4cf39a8
fix: comment
nolanlawson Sep 1, 2023
892212b
test: add test for non-standard props NOT reporting
nolanlawson Sep 1, 2023
8ab13fa
fix: add explanatory comment
nolanlawson Sep 1, 2023
85167e1
Merge remote-tracking branch 'origin/master' into nolan/default-aria-…
nolanlawson Oct 16, 2023
dbcdad9
Merge remote-tracking branch 'origin/master' into nolan/default-aria-…
nolanlawson Oct 17, 2023
d118c6d
Merge remote-tracking branch 'origin/master' into nolan/default-aria-…
nolanlawson Oct 20, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ commands:
enable_native_custom_element_lifecycle:
type: boolean
default: false
disable_aria_reflection_polyfill:
enable_aria_reflection_global_polyfill:
type: boolean
default: false
node_env_production:
Expand Down Expand Up @@ -95,7 +95,7 @@ commands:
<<# parameters.disable_synthetic >> DISABLE_SYNTHETIC=1 <</ parameters.disable_synthetic >> \
<<# parameters.force_native_shadow_mode >> FORCE_NATIVE_SHADOW_MODE_FOR_TEST=1 <</ parameters.force_native_shadow_mode >> \
<<# parameters.enable_native_custom_element_lifecycle >> ENABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE=1 <</ parameters.enable_native_custom_element_lifecycle >> \
<<# parameters.disable_aria_reflection_polyfill >> DISABLE_ARIA_REFLECTION_POLYFILL=1 <</ parameters.disable_aria_reflection_polyfill >> \
<<# parameters.enable_aria_reflection_global_polyfill >> ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL=1 <</ parameters.enable_aria_reflection_global_polyfill >> \
<<# parameters.node_env_production >> NODE_ENV_FOR_TEST=production <</ parameters.node_env_production >> \
<<# parameters.disable_synthetic_shadow_support_in_compiler >> DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER=1 <</ parameters.disable_synthetic_shadow_support_in_compiler >> \
<<# parameters.legacy_browsers >> LEGACY_BROWSERS=1 <</ parameters.legacy_browsers >> \
Expand Down Expand Up @@ -188,7 +188,7 @@ commands:
enable_native_custom_element_lifecycle:
type: boolean
default: false
disable_aria_reflection_polyfill:
enable_aria_reflection_global_polyfill:
type: boolean
default: false
node_env_production:
Expand All @@ -210,7 +210,7 @@ commands:
disable_synthetic: << parameters.disable_synthetic >>
force_native_shadow_mode: << parameters.force_native_shadow_mode >>
enable_native_custom_element_lifecycle: << parameters.enable_native_custom_element_lifecycle >>
disable_aria_reflection_polyfill: << parameters.disable_aria_reflection_polyfill >>
enable_aria_reflection_global_polyfill: << parameters.enable_aria_reflection_global_polyfill >>
node_env_production: << parameters.node_env_production >>
disable_synthetic_shadow_support_in_compiler: << parameters.disable_synthetic_shadow_support_in_compiler >>
api_version: << parameters.api_version >>
Expand Down Expand Up @@ -302,9 +302,11 @@ jobs:
- run_karma:
disable_synthetic: true
api_version: 58
- run_karma:
enable_aria_reflection_global_polyfill: true
- run_karma:
disable_synthetic: true
disable_aria_reflection_polyfill: true
enable_aria_reflection_global_polyfill: true
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we may as well test with synthetic shadow on vs off, especially since this flag will be set to true in LEX.

- run_karma:
disable_synthetic: true
disable_synthetic_shadow_support_in_compiler: true
Expand Down
17 changes: 5 additions & 12 deletions packages/@lwc/aria-reflection/README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# @lwc/aria-reflection

> **Note:** use this code at your own risk. It is optimized for backwards-compatibility, not
> as a forward-looking polyfill that keeps up to date with web standards.

Polyfill for [ARIA string reflection](https://wicg.github.io/aom/spec/aria-reflection.html) on Elements.
This is part of the [Accessibility Object Model](https://wicg.github.io/aom/explainer.html) (AOM).

Expand All @@ -21,20 +24,10 @@ npm install @lwc/aria-reflection
```

```js
import { applyAriaReflection } from '@lwc/aria-reflection';

applyAriaReflection();
```

The polyfill is applied as soon as the function is executed.

Optionally, you can pass in a custom prototype:

```js
applyAriaReflection(MyCustomElement.prototype);
import '@lwc/aria-reflection';
```

By default, the polyfill is applied to the global `Element.prototype`.
The polyfill is applied globally to `Element.prototype` as soon as the module is imported.

## Implementation

Expand Down
4 changes: 1 addition & 3 deletions packages/@lwc/aria-reflection/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,5 @@
}
}
},
"dependencies": {
"@lwc/shared": "3.2.0"
}
"dependencies": {}
}
11 changes: 0 additions & 11 deletions packages/@lwc/aria-reflection/src/detect.ts

This file was deleted.

87 changes: 76 additions & 11 deletions packages/@lwc/aria-reflection/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,85 @@
/*
* Copyright (c) 2018, salesforce.com, inc.
* Copyright (c) 2023, salesforce.com, inc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious whether you're updating these manually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manually, yes. We should really go through and just run a script or something.

* All rights reserved.
* SPDX-License-Identifier: MIT
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
import { AriaPropNameToAttrNameMap, keys } from '@lwc/shared';
import { detect } from './detect';
import { patch } from './polyfill';

export function applyAriaReflection(prototype: any = Element.prototype) {
const ElementPrototypeAriaPropertyNames = keys(AriaPropNameToAttrNameMap);
// Minimal polyfill of ARIA string reflection, plus some non-standard ARIA props
// Taken from https://github.com/salesforce/lwc/blob/44a01ef/packages/%40lwc/shared/src/aria.ts#L22-L70
// This is designed for maximum backwards compatibility on LEX - it should never change.
// We deliberately don't import from @lwc/shared because that would make this code less portable.
const ARIA_PROPERTIES = [
'ariaActiveDescendant',
'ariaAtomic',
'ariaAutoComplete',
'ariaBusy',
'ariaChecked',
'ariaColCount',
'ariaColIndex',
'ariaColSpan',
'ariaControls',
'ariaCurrent',
'ariaDescribedBy',
'ariaDetails',
'ariaDisabled',
'ariaErrorMessage',
'ariaExpanded',
'ariaFlowTo',
'ariaHasPopup',
'ariaHidden',
'ariaInvalid',
'ariaKeyShortcuts',
'ariaLabel',
'ariaLabelledBy',
'ariaLevel',
'ariaLive',
'ariaModal',
'ariaMultiLine',
'ariaMultiSelectable',
'ariaOrientation',
'ariaOwns',
'ariaPlaceholder',
'ariaPosInSet',
'ariaPressed',
'ariaReadOnly',
'ariaRelevant',
'ariaRequired',
'ariaRoleDescription',
'ariaRowCount',
'ariaRowIndex',
'ariaRowSpan',
'ariaSelected',
'ariaSetSize',
'ariaSort',
'ariaValueMax',
'ariaValueMin',
'ariaValueNow',
'ariaValueText',
'role',
];

for (let i = 0, len = ElementPrototypeAriaPropertyNames.length; i < len; i += 1) {
const propName = ElementPrototypeAriaPropertyNames[i];
if (detect(propName, prototype)) {
patch(propName, prototype);
}
for (const prop of ARIA_PROPERTIES) {
const attribute = prop.replace(/^aria/, 'aria-').toLowerCase(); // e.g. ariaPosInSet => aria-posinset

if (!Object.getOwnPropertyDescriptor(Element.prototype, prop)) {
Object.defineProperty(Element.prototype, prop, {
get() {
return this.getAttribute(attribute);
},
set(value) {
// Per the spec, only null is treated as removing the attribute. However, Chromium/WebKit currently
// differ from the spec and allow undefined as well. Here, we follow the spec, as well as
// our historical behavior. See: https://github.com/w3c/aria/issues/1858
if (value === null) {
this.removeAttribute(attribute);
} else {
this.setAttribute(attribute, value);
}
},
// configurable and enumerable to allow it to be overridden – this mimics Safari's/Chrome's behavior
configurable: true,
enumerable: true,
});
}
}
34 changes: 0 additions & 34 deletions packages/@lwc/aria-reflection/src/polyfill.ts

This file was deleted.

1 change: 0 additions & 1 deletion packages/@lwc/engine-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
}
},
"dependencies": {
"@lwc/aria-reflection": "3.2.0",
"@lwc/features": "3.2.0",
"@lwc/shared": "3.2.0"
},
Expand Down
20 changes: 10 additions & 10 deletions packages/@lwc/engine-core/src/framework/base-bridge-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import {
keys,
htmlPropertyToAttribute,
} from '@lwc/shared';
import { applyAriaReflection } from '@lwc/aria-reflection';
import { logError } from '../shared/logger';
import { applyAriaReflection } from '../libs/aria-reflection/aria-reflection';
import { getAssociatedVM } from './vm';
import { getReadOnlyProxy } from './membrane';
import { HTMLElementConstructor } from './html-element';
Expand Down Expand Up @@ -182,15 +182,15 @@ 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
if (lwcRuntimeFlags.DISABLE_ARIA_REFLECTION_POLYFILL) {
// If ARIA reflection is not applied globally to Element.prototype, apply it 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
//
// Also note that we apply this to BaseBridgeElement.prototype to avoid excessively redefining property
// accessors inside the HTMLBridgeElementFactory.
applyAriaReflection(BaseBridgeElement.prototype);
}
//
// 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);
Copy link
Collaborator Author

@nolanlawson nolanlawson Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applying ARIA reflection to the base bridge element and lightning element unilaterally has a few benefits:

  1. No confusion about server vs client – same logic for both
  2. The global polyfill is simpler – it affects one line of code basically (whether we apply the polyfill to Element.prototype or not)
  3. It's easier to detect violations – we can just detect Element.prototype.aria* directly with no need to filter out Lightning/BridgeElements.

}

freeze(BaseBridgeElement);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ import {
keys,
setPrototypeOf,
} from '@lwc/shared';
import { applyAriaReflection } from '@lwc/aria-reflection';

import { logError } from '../shared/logger';
import { getComponentTag } from '../shared/format';
import { applyAriaReflection } from '../libs/aria-reflection/aria-reflection';

import { HTMLElementOriginalDescriptors } from './html-properties';
import { getWrappedComponentsListener } from './component';
Expand Down Expand Up @@ -728,16 +728,10 @@ for (const propName in HTMLElementOriginalDescriptors) {

defineProperties(LightningElement.prototype, lightningBasedDescriptors);

function applyAriaReflectionToLightningElement() {
// If ARIA reflection is not applied globally to Element.prototype, or if we are running server-side,
// apply it to LightningElement.prototype.
// This allows `this.aria*` property accessors to work from inside a component, and to reflect `aria-*` attrs.
applyAriaReflection(LightningElement.prototype);
}

if (!process.env.IS_BROWSER || lwcRuntimeFlags.DISABLE_ARIA_REFLECTION_POLYFILL) {
applyAriaReflectionToLightningElement();
}
// 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);

defineProperty(LightningElement, 'CustomElementConstructor', {
get() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright (c) 2023, salesforce.com, inc.
* All rights reserved.
* SPDX-License-Identifier: MIT
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
import {
AriaPropNameToAttrNameMap,
defineProperty,
getOwnPropertyDescriptor,
isNull,
isUndefined,
keys,
} from '@lwc/shared';
import { LightningElement } from '../../framework/base-lightning-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
// 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);
ekashida marked this conversation as resolved.
Show resolved Hide resolved
},
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,
});
}
}
}
Loading