fix: aria polyfill overrides user values and user values override internals values

Elliott Marquez · copybara-github · commit 8aa4faf14ac8 · 2023-09-14T22:11:40.000-07:00
PiperOrigin-RevId: 565570035
diff --git a/internal/aria/aria.ts b/internal/aria/aria.ts
@@ -271,68 +271,64 @@ 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 {
           host.setAttribute('role', value);
         }
       };
 
-      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<ARIAProperty|'role'>();
+
+      // 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
@@ -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();
