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

nolanlawson · 2023-08-09T15:59:48Z

Details

The @lwc/aria-reflection polyfill still exists, but we don't use it in the LWC engine. It's just there for backwards-compat in LEX, similar to @lwc/synthetic-shadow.

Old description

This disables the global ARIA reflection polyfill by default. It can still be enabled with the flag ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL.

The idea is that we can set this to true in lwc-platform for as long as we need the backwards-compat support. Unfortunately we don't have a good idea of how to fix this with API versioning, because (since it's a global polyfill), it's currently being used both by LWC components and Aura components in LEX.

On the bright side, we can at least remove it from the core LWC npm package, which means that non-LEX environments will not get the global polyfill.

Does this pull request introduce a breaking change?

🚨 Yes, it does introduce a breaking change.

Note: this change only applies to non-Lightning Experience use cases such as Jest tests and off-core scenarios. It also only applies to non-LightningElement HTML elements.

Removal of non-standard ARIA APIs

The following non-standard ARIA APIs are now deprecated:

ariaActiveDescendant
ariaControls
ariaDescribedBy
ariaDetails
ariaErrorMessage
ariaFlowTo
ariaLabelledBy
ariaOwns

… when used on a non-LightningElement HTML element. E.g.:

const div = this.template.querySelector('div')
console.log(div.ariaActiveDescendant) // undefined

For a LightningElement, you can continue to use this.ariaActiveDescendant and element.ariaActiveDescendant as before.

For arbitrary HTML elements, however, instead of using these non-standard APIs, please use the appropriate setAttribute/getAttribute APIs instead:

aria-activedescendant
aria-controls
aria-describedby
aria-details
aria-errormessage
aria-flowto
aria-labelledby
aria-owns

E.g.:

const div = this.template.querySelector('div')
console.log(div.getAttribute('aria-activedescendant')) // string ID, if attribute is set

Removal of standard ARIA reflection global polyfill

Some browsers and environments, notably Firefox and JSDOM, do not yet support ARIA reflection for properties such as role and ariaLabel. For maximum compatibility, please use the attribute format instead:

element.role -> element.getAttribute('role')
element.ariaLabel -> element.getAttribute('aria-label')

If you are using the latest version of @lwc/jest-*, a polyfill for JSDOM is already included.

Deprecation of `@lwc/aria-reflection` package

If you were using this package for whatever reason, it is now deprecated. You should probably not be using it, unless you absolutely need it for backwards compatibility and don't want to update code that's relying on it.

Does this pull request introduce an observable change?

⚠️ Yes, it does include an observable change.

See above.

GUS work item

W-10820032

nolanlawson · 2023-08-09T16:00:40Z

Note I considered an alternative implementation where, instead of using an lwcRuntimeFlag, we treat @lwc/aria-reflection similar to @lwc/synthetic-shadow and sideload it as a separate polyfill. Unfortunately this ended up complicating the code in lwc-platform quite a bit, and duplicated @lwc/shared code between the two, so I'm not sure it's worth it. But we could always go in that direction later, to remove the lwcRuntimeFlag entirely.

nolanlawson · 2023-08-09T16:01:06Z

packages/@lwc/engine-server/src/__tests__/fixtures.spec.ts

-
-    describe('disable aria reflection polyfill', () => {
-        testWithFeatureFlagEnabled('DISABLE_ARIA_REFLECTION_POLYFILL');
-    });


There is no longer any difference in server mode with this flag on or off, so this test isn't needed anymore.

nolanlawson · 2023-08-09T16:07:02Z

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

+    //
+    // Also note that we apply this to BaseBridgeElement.prototype to avoid excessively redefining property
+    // accessors inside the HTMLBridgeElementFactory.
+    applyAriaReflection(BaseBridgeElement.prototype);


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

No confusion about server vs client – same logic for both

The global polyfill is simpler – it affects one line of code basically (whether we apply the polyfill to Element.prototype or not)

It's easier to detect violations – we can just detect Element.prototype.aria* directly with no need to filter out Lightning/BridgeElements.

nolanlawson · 2023-08-09T16:09:20Z

packages/@lwc/integration-karma/test/accessibility/synthetic-cross-root-aria/index.spec.js

+            // polyfill is not applied
+            if (window.lwcRuntimeFlags.ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL) {
+                usePropertyAccessValues.push(true);
+            }


This test was busted before, but we didn't notice because we weren't testing with global ARIA reflection disabled in native shadow mode.

nolanlawson · 2023-08-09T16:09:50Z

.circleci/config.yml

      - run_karma:
          disable_synthetic: true
-          disable_aria_reflection_polyfill: true
+          enable_aria_reflection_global_polyfill: true


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

…ault-aria-reflection-off

…f' into nolan/default-aria-reflection-off

nolanlawson · 2023-08-10T22:27:10Z

As it turns out, JSDOM does not support ARIA string reflection (jsdom/jsdom#3323), so the following does not work:

<div role="button" aria-label="foo"></div>

element.role // undefined
element.ariaLabel // undefined

(This matches Firefox's current behavior, but not Chrome's/Safari's.)

Unfortunately this is working in LWC Jest tests today, because we are applying this polyfill to the global Element.prototype. Removing this polyfill could cause a lot of Jest tests to fail.

In addition, we are currently not warning on string reflection (role, ariaLabel, etc.), only for element reflection (e.g. ariaLabelledBy). So people are not even currently aware that they are relying on polyfilled functionality. And even if we did warn, folks would not see it in the Chrome DevTools, since Chrome does support ARIA string reflection.

nolanlawson · 2023-08-11T16:41:27Z

I opened a PR on JSDOM for this (jsdom/jsdom#3586), but in the interests of expediency, we may want to temporarily add the minimal string reflection polyfill to lwc-test.

nolanlawson · 2023-08-11T19:58:41Z

Argument against fixing this in lwc-test: getting a test failure would indicate that the component may be broken on Firefox (outside of LEX).

…reflection-off

nolanlawson · 2023-08-22T22:55:02Z

Per discussion in DevSync, removed the polyfill entirely and moved it to lwc-platform: 4cafff8 ^

The lwcRuntimeFlag is also gone, although we still need a way to test the global polyfill in Karma, so I did that separately.

…reflection-off

nolanlawson · 2023-08-24T18:52:14Z

/nucleus test

nolanlawson · 2023-08-24T20:17:44Z

/nucleus test

nolanlawson · 2023-08-24T20:47:06Z

Realized I could simplify things even further by just removing the @lwc/aria-reflection package entirely. If we're not planning on exposing it externally, then there's no point in having it at all.

When we release v4.0, we can just mark the existing package as deprecated.

…reflection-off

nolanlawson · 2023-08-25T19:58:36Z

After discussion in PR review:

Keep @lwc/aria-reflection as a global polyfill
Make it self-contained (don't import from @lwc/shared)

This makes @lwc/aria-reflection conceptually the same as @lwc/synthetic-shadow.

nolanlawson · 2023-08-25T20:27:29Z

/nucleus test

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

packages/@lwc/engine-core/src/patches/detect-non-standard-aria.ts

ekashida · 2023-08-31T17:44:45Z

packages/@lwc/engine-core/src/patches/detect-non-standard-aria.ts

@@ -45,7 +43,8 @@ function findVM(elm: Element): VM | undefined {
    // Else it might be a light DOM component. Walk up the tree trying to find the owner
    let parentElement: Element | null = elm;
    while (!isNull((parentElement = parentElement.parentElement))) {
-        if (isLightningElement(parentElement)) {
+        if (parentElement instanceof BaseBridgeElement) {


The elm instanceof LightningElement check isn't required here because findVM only deals with DOM elements?

Correct. LightningElement is an internal thing that you can't see from the outside by traversing the DOM.

ekashida · 2023-08-31T21:53:50Z

packages/@lwc/engine-core/src/patches/detect-non-standard-aria.ts

@@ -56,32 +55,30 @@ function findVM(elm: Element): VM | undefined {
 }

 function checkAndReportViolation(elm: Element, prop: string, isSetter: boolean, setValue: any) {
-    if (!isLightningElement(elm)) {


How come we no longer require this check?

I actually couldn't remember, so I wrote a test to confirm the behavior: 892212b

I checked, and the reason this works i because we are overriding the Element.prototype accessors in detect-non-standard-aria after the accessors were applied to LightningElement.prototype and BaseBridgeElement.prototype. So when you call the accessors directly on the component (internally with this.aria* or externally with elm.aria*), it never goes through the accessors in this file.

This is kind of confusing, so I added a comment to explain: 8ab13fa

…reflection-off

nolanlawson · 2023-09-01T17:08:25Z

/nucleus test

ekashida · 2023-09-20T16:31:36Z

packages/@lwc/aria-reflection/src/index.ts

@@ -1,20 +1,85 @@
 /*
- * Copyright (c) 2018, salesforce.com, inc.
+ * Copyright (c) 2023, salesforce.com, inc.


Just curious whether you're updating these manually?

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

…reflection-off

fix(engine-dom): disable ARIA reflection polyfill (BREAKING CHANGE)

6c913e1

nolanlawson requested a review from a team as a code owner August 9, 2023 15:59

nolanlawson commented Aug 9, 2023

View reviewed changes

chore: clearer name

27fe2a4

nolanlawson commented Aug 9, 2023

View reviewed changes

fix: update types.ts

32c0c7c

nolanlawson commented Aug 9, 2023

View reviewed changes

nolanlawson added this to the 4.0.0 milestone Aug 9, 2023

This comment was marked as outdated.

Sign in to view

nolanlawson added 2 commits August 10, 2023 09:39

Merge remote-tracking branch 'origin/milestone-v4.0.0' into nolan/def…

827c800

…ault-aria-reflection-off

Merge remote-tracking branch 'origin/nolan/default-aria-reflection-of…

d47b1a2

…f' into nolan/default-aria-reflection-off

nolanlawson changed the base branch from prerelease-v4.0.0 to milestone-v4.0.0 August 10, 2023 16:40

chore: empty commit

d62fe1d

nolanlawson changed the base branch from milestone-v4.0.0 to master August 10, 2023 19:09

nolanlawson added the nomerge label Aug 10, 2023

chore: bump ci

f5bc994

This comment was marked as outdated.

Sign in to view

nolanlawson mentioned this pull request Aug 15, 2023

fix: minimal ARIA string reflection polyfill for JSDOM salesforce/lwc-test#207

Merged

nolanlawson added 2 commits August 22, 2023 15:37

Merge remote-tracking branch 'origin/master' into nolan/default-aria-…

774d91b

…reflection-off

fix: remove the global polyfill for real

4cafff8

fix: header

7e5c53a

nolanlawson changed the title ~~fix(engine-dom): disable ARIA reflection polyfill (BREAKING CHANGE)~~ fix(engine-dom): remove ARIA reflection global polyfill (BREAKING CHANGE) Aug 22, 2023

Merge remote-tracking branch 'origin/master' into nolan/default-aria-…

5e14ddf

…reflection-off

fix: remove @lwc/aria-reflection entirely

953df88

nolanlawson added 3 commits August 24, 2023 14:51

fix: detect before patching

88dd671

Merge remote-tracking branch 'origin/master' into nolan/default-aria-…

4068dfb

…reflection-off

fix: retain @lwc/aria-reflection

021998e

fix: fix comments

e7949e6

nolanlawson mentioned this pull request Aug 30, 2023

Add missing ARIA attr/prop reflections #3693

Closed

ekashida reviewed Aug 31, 2023

View reviewed changes

nolanlawson added 5 commits September 1, 2023 09:12

Merge remote-tracking branch 'origin/master' into nolan/default-aria-…

c14333e

…reflection-off

fix: tighten prototype types

540eeb7

fix: comment

4cf39a8

test: add test for non-standard props NOT reporting

892212b

fix: add explanatory comment

8ab13fa

nolanlawson requested a review from ekashida September 1, 2023 17:34

nolanlawson mentioned this pull request Sep 14, 2023

feat: adding missing aria attributes #3702

Merged

ekashida approved these changes Sep 21, 2023

View reviewed changes

nolanlawson added 2 commits October 16, 2023 07:46

Merge remote-tracking branch 'origin/master' into nolan/default-aria-…

85167e1

…reflection-off

Merge remote-tracking branch 'origin/master' into nolan/default-aria-…

dbcdad9

…reflection-off

nolanlawson removed the nomerge label Oct 20, 2023

Merge remote-tracking branch 'origin/master' into nolan/default-aria-…

d118c6d

…reflection-off

nolanlawson merged commit a7d190f into master Oct 20, 2023
11 checks passed

nolanlawson deleted the nolan/default-aria-reflection-off branch October 20, 2023 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nolanlawson commented Aug 9, 2023 •

edited

Loading

nolanlawson commented Aug 9, 2023

nolanlawson Aug 9, 2023

nolanlawson Aug 9, 2023 •

edited

Loading

nolanlawson Aug 9, 2023 •

edited

Loading

nolanlawson Aug 9, 2023

This comment was marked as outdated.

nolanlawson commented Aug 10, 2023

nolanlawson commented Aug 11, 2023

This comment was marked as outdated.

nolanlawson commented Aug 11, 2023

nolanlawson commented Aug 22, 2023 •

edited

Loading

nolanlawson commented Aug 24, 2023

nolanlawson commented Aug 24, 2023

nolanlawson commented Aug 24, 2023

nolanlawson commented Aug 25, 2023

nolanlawson commented Aug 25, 2023

ekashida Aug 31, 2023

nolanlawson Sep 1, 2023

ekashida Aug 31, 2023

nolanlawson Sep 1, 2023

nolanlawson commented Sep 1, 2023

ekashida Sep 20, 2023

nolanlawson Sep 21, 2023

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

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

Conversation

nolanlawson commented Aug 9, 2023 • edited Loading

Details

Does this pull request introduce a breaking change?

Removal of non-standard ARIA APIs

Removal of standard ARIA reflection global polyfill

Deprecation of @lwc/aria-reflection package

Does this pull request introduce an observable change?

GUS work item

nolanlawson commented Aug 9, 2023

Choose a reason for hiding this comment

nolanlawson Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

nolanlawson Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

nolanlawson commented Aug 10, 2023

nolanlawson commented Aug 11, 2023

This comment was marked as outdated.

nolanlawson commented Aug 11, 2023

nolanlawson commented Aug 22, 2023 • edited Loading

nolanlawson commented Aug 24, 2023

nolanlawson commented Aug 24, 2023

nolanlawson commented Aug 24, 2023

nolanlawson commented Aug 25, 2023

nolanlawson commented Aug 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson commented Aug 9, 2023 •

edited

Loading

Deprecation of `@lwc/aria-reflection` package

nolanlawson Aug 9, 2023 •

edited

Loading

nolanlawson Aug 9, 2023 •

edited

Loading

nolanlawson commented Aug 22, 2023 •

edited

Loading