From 8aa4faf14ac8207c9a39f783f971bfbf5933bfa5 Mon Sep 17 00:00:00 2001 From: Elliott Marquez Date: Thu, 14 Sep 2023 22:11:00 -0700 Subject: [PATCH] fix: aria polyfill overrides user values and user values override internals values PiperOrigin-RevId: 565570035 --- internal/aria/aria.ts | 76 +++++++++++++++---------- internal/aria/aria_test.ts | 112 +++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 28 deletions(-) diff --git a/internal/aria/aria.ts b/internal/aria/aria.ts index e23a61f62f..b152065379 100644 --- a/internal/aria/aria.ts +++ b/internal/aria/aria.ts @@ -271,55 +271,57 @@ export function polyfillElementInternalsAria( throw new Error('Missing setupHostAria()'); } - let firstConnectedCallbacks: Array<() => void> = []; + let firstConnectedCallbacks: + Array<{property: ARIAProperty | 'role', callback: () => void}> = []; let hasBeenConnected = false; // Add support for Firefox, which has not yet implement ElementInternals aria for (const ariaProperty of ARIA_PROPERTIES) { - let ariaValueBeforeConnected: string|null = null; + let internalAriaValue: string|null = null; Object.defineProperty(internals, ariaProperty, { enumerable: true, configurable: true, get() { - if (!hasBeenConnected) { - return ariaValueBeforeConnected; - } - - // Dynamic lookup rather than hardcoding all properties. - // tslint:disable-next-line:no-dict-access-on-struct-type - return host[ariaProperty]; + return internalAriaValue; }, set(value: string|null) { const setValue = () => { + internalAriaValue = value; + if (!hasBeenConnected) { + firstConnectedCallbacks.push( + {property: ariaProperty, callback: setValue}); + return; + } + // Dynamic lookup rather than hardcoding all properties. // tslint:disable-next-line:no-dict-access-on-struct-type host[ariaProperty] = value; }; - if (!hasBeenConnected) { - ariaValueBeforeConnected = value; - firstConnectedCallbacks.push(setValue); - return; - } - setValue(); }, }); } - let roleValueBeforeConnected: string|null = null; + let internalRoleValue: string|null = null; Object.defineProperty(internals, 'role', { enumerable: true, configurable: true, get() { - if (!hasBeenConnected) { - return roleValueBeforeConnected; - } - - return host.getAttribute('role'); + return internalRoleValue; }, set(value: string|null) { const setRole = () => { + internalRoleValue = value; + + if (!hasBeenConnected) { + firstConnectedCallbacks.push({ + property: 'role', + callback: setRole, + }); + return; + } + if (value === null) { host.removeAttribute('role'); } else { @@ -327,12 +329,6 @@ export function polyfillElementInternalsAria( } }; - if (!hasBeenConnected) { - roleValueBeforeConnected = value; - firstConnectedCallbacks.push(setRole); - return; - } - setRole(); }, }); @@ -344,7 +340,31 @@ export function polyfillElementInternalsAria( } hasBeenConnected = true; - for (const callback of firstConnectedCallbacks) { + + const propertiesSetByUser = new Set(); + + // See which properties were set by the user on host before we apply + // internals values as attributes to host. Needs to be done in another + // for loop because the callbacks set these attributes on host. + for (const {property} of firstConnectedCallbacks) { + const wasSetByUser = + host.getAttribute(ariaPropertyToAttribute(property)) !== null || + // Dynamic lookup rather than hardcoding all properties. + // tslint:disable-next-line:no-dict-access-on-struct-type + host[property] !== undefined; + + if (wasSetByUser) { + propertiesSetByUser.add(property); + } + } + + for (const {property, callback} of firstConnectedCallbacks) { + // If the user has set the attribute or property, do not override the + // user's value + if (propertiesSetByUser.has(property)) { + continue; + } + callback(); } diff --git a/internal/aria/aria_test.ts b/internal/aria/aria_test.ts index 8fccc375d8..4313523622 100644 --- a/internal/aria/aria_test.ts +++ b/internal/aria/aria_test.ts @@ -198,6 +198,80 @@ describe('aria', () => { element.remove(); }); + it('should not override aria attributes on host when set before connection', + async () => { + const element = new TestElement(); + element.setAttribute('aria-label', 'Value set by user'); + element.internals.ariaLabel = 'Value set on internals'; + document.body.appendChild(element); + await element.updateComplete; + expect(element.getAttribute('aria-label')) + .withContext('aria-label attribute value on host') + .toEqual('Value set by user'); + expect(element.internals.ariaLabel) + .withContext('ariaLabel internals property still the same') + .toEqual('Value set on internals'); + + element.remove(); + }); + + it('should not override aria properties on host when set before connection', + async () => { + const element = new TestElement(); + element.ariaLabel = 'Value set by user'; + element.internals.ariaLabel = 'Value set on internals'; + document.body.appendChild(element); + await element.updateComplete; + expect(element.getAttribute('aria-label')) + .withContext('aria-label attribute value on host') + .toEqual('Value set by user'); + expect(element.ariaLabel) + .withContext('ariaLabel property value on host') + .toEqual('Value set by user'); + expect(element.internals.ariaLabel) + .withContext('ariaLabel internals property still the same') + .toEqual('Value set on internals'); + + element.remove(); + }); + + it('should not override role attribute on host when set before connection', + async () => { + const element = new TestElement(); + element.setAttribute('role', 'Value set by user'); + element.internals.role = 'Value set on internals'; + document.body.appendChild(element); + await element.updateComplete; + expect(element.getAttribute('role')) + .withContext('role attribute value on host') + .toEqual('Value set by user'); + expect(element.internals.role) + .withContext('role internals property still the same') + .toEqual('Value set on internals'); + + element.remove(); + }); + + it('should not override role property on host when set before connection', + async () => { + const element = new TestElement(); + element.role = 'Value set by user'; + element.internals.role = 'Value set on internals'; + document.body.appendChild(element); + await element.updateComplete; + expect(element.getAttribute('role')) + .withContext('role attribute value on host') + .toEqual('Value set by user'); + expect(element.role) + .withContext('role property value on host') + .toEqual('Value set by user'); + expect(element.internals.role) + .withContext('role internals property still the same') + .toEqual('Value set on internals'); + + element.remove(); + }); + it('should handle setting role multiple times before connection', async () => { const element = new TestElement(); @@ -216,6 +290,25 @@ describe('aria', () => { element.remove(); }); + it('should handle setting role multiple times before connection when property is set on host', + async () => { + const element = new TestElement(); + element.role = 'radio'; + element.internals.role = 'button'; + element.internals.role = 'checkbox'; + + expect(element.internals.role) + .withContext('internals.role before connection') + .toEqual('checkbox'); + document.body.appendChild(element); + await element.updateComplete; + expect(element.internals.role) + .withContext('internals.role after connection') + .toEqual('checkbox'); + + element.remove(); + }); + it('should handle setting aria properties multiple times before connection', async () => { const element = new TestElement(); @@ -234,6 +327,25 @@ describe('aria', () => { element.remove(); }); + it('should handle setting aria properties multiple times before connection when property is set on host', + async () => { + const element = new TestElement(); + element.ariaLabel = 'First'; + element.internals.ariaLabel = 'First'; + element.internals.ariaLabel = 'Second'; + + expect(element.internals.ariaLabel) + .withContext('internals.ariaLabel before connection') + .toEqual('Second'); + document.body.appendChild(element); + await element.updateComplete; + expect(element.internals.ariaLabel) + .withContext('internals.ariaLabel after connection') + .toEqual('Second'); + + element.remove(); + }); + it('should handle setting role after first connection while disconnected', async () => { const element = new TestElement();