From b2291b5179e28070105d753f343a224f82268dc1 Mon Sep 17 00:00:00 2001
From: Kevin Dwan <25626770+krdwan@users.noreply.github.com>
Date: Fri, 26 Feb 2021 09:25:15 -0500
Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20[bento][amp-accordion][fixit]=20Che?=
 =?UTF-8?q?ck=20for=20pre-existing=20role=20attribute=20before=20using=20d?=
 =?UTF-8?q?efault=20value=20(#32915)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Check for pre-existing role attribute before using default value

* Update where role is passed

* Add explainer comment in code
---
 extensions/amp-accordion/1.0/base-element.js  |  9 +++++
 .../amp-accordion/1.0/test/test-accordion.js  | 36 ++++++++++++++++++
 .../1.0/test/test-amp-accordion.js            | 37 +++++++++++++++++++
 3 files changed, 82 insertions(+)

diff --git a/extensions/amp-accordion/1.0/base-element.js b/extensions/amp-accordion/1.0/base-element.js
index 1b1ef6be89d5..fc1231c78c3d 100644
--- a/extensions/amp-accordion/1.0/base-element.js
+++ b/extensions/amp-accordion/1.0/base-element.js
@@ -103,13 +103,22 @@ function getState(element, mu, getExpandStateTrigger) {
       'id': section.getAttribute('id'),
       'onExpandStateChange': expandStateShim,
     });
+    // For headerProps and contentProps:
+    // || undefined needed for the `role` attribute since an element w/o
+    // role results in `null`.  When `null` is passed into Preact, the
+    // default prop value is not used.  `undefined` triggers default prop
+    // value.  This is not needed for `id` since this is handled with
+    // explicit logic (not default prop value) and all falsy values are
+    // handled the same.
     const headerProps = dict({
       'as': headerShim,
       'id': section.firstElementChild.getAttribute('id'),
+      'role': section.firstElementChild.getAttribute('role') || undefined,
     });
     const contentProps = dict({
       'as': contentShim,
       'id': section.lastElementChild.getAttribute('id'),
+      'role': section.lastElementChild.getAttribute('role') || undefined,
     });
     return (
       <AccordionSection {...sectionProps}>
diff --git a/extensions/amp-accordion/1.0/test/test-accordion.js b/extensions/amp-accordion/1.0/test/test-accordion.js
index 2735fbec6991..542f78963dfc 100644
--- a/extensions/amp-accordion/1.0/test/test-accordion.js
+++ b/extensions/amp-accordion/1.0/test/test-accordion.js
@@ -334,6 +334,42 @@ describes.sandboxed('Accordion preact component', {}, (env) => {
       );
     });
 
+    it('should default role attribute unless role prop provided', () => {
+      wrapper = mount(
+        <Accordion>
+          <AccordionSection key={1} expanded>
+            <AccordionHeader role="cat">header1</AccordionHeader>
+            <AccordionContent role="dog">content1</AccordionContent>
+          </AccordionSection>
+          <AccordionSection key={2}>
+            <AccordionHeader>header2</AccordionHeader>
+            <AccordionContent>content2</AccordionContent>
+          </AccordionSection>
+        </Accordion>
+      );
+
+      const dom = wrapper.getDOMNode();
+      expect(dom.localName).to.equal('section');
+
+      const sections = wrapper.find(AccordionSection);
+      expect(sections).to.have.lengthOf(2);
+
+      const header0 = sections.at(0).find('div').at(0).getDOMNode();
+      const header1 = sections.at(1).find('div').at(0).getDOMNode();
+      const content0 = sections.at(0).find('div').at(1).getDOMNode();
+      const content1 = sections.at(1).find('div').at(1).getDOMNode();
+
+      expect(header0).to.have.attribute('role');
+      expect(header0.getAttribute('role')).to.equal('cat');
+      expect(content0).to.have.attribute('role');
+      expect(content0.getAttribute('role')).to.equal('dog');
+
+      expect(header1).to.have.attribute('role');
+      expect(header1.getAttribute('role')).to.equal('button');
+      expect(content1).to.have.attribute('role');
+      expect(content1.getAttribute('role')).to.equal('region');
+    });
+
     it('should expand a section on click', () => {
       const dom = wrapper.getDOMNode();
       expect(dom.localName).to.equal('section');
diff --git a/extensions/amp-accordion/1.0/test/test-amp-accordion.js b/extensions/amp-accordion/1.0/test/test-amp-accordion.js
index 09e819b9cf31..8fe858599d96 100644
--- a/extensions/amp-accordion/1.0/test/test-amp-accordion.js
+++ b/extensions/amp-accordion/1.0/test/test-amp-accordion.js
@@ -365,6 +365,43 @@ describes.realWin(
       );
     });
 
+    it('should not overwrite existing role attributes', async () => {
+      element = html`
+        <amp-accordion layout="fixed" width="300" height="200">
+          <section expanded id="section1">
+            <h1 role="cat">header1</h1>
+            <div role="dog">content1</div>
+          </section>
+          <section>
+            <h1 id="h2">header2</h1>
+            <div>content2</div>
+          </section>
+        </amp-accordion>
+      `;
+      win.document.body.appendChild(element);
+      await element.buildInternal();
+
+      const sections = element.children;
+      const {
+        firstElementChild: header0,
+        lastElementChild: content0,
+      } = sections[0];
+      const {
+        firstElementChild: header1,
+        lastElementChild: content1,
+      } = sections[1];
+
+      expect(header0).to.have.attribute('role');
+      expect(header0.getAttribute('role')).to.equal('cat');
+      expect(content0).to.have.attribute('role');
+      expect(content0.getAttribute('role')).to.equal('dog');
+
+      expect(header1).to.have.attribute('role');
+      expect(header1.getAttribute('role')).to.equal('button');
+      expect(content1).to.have.attribute('role');
+      expect(content1.getAttribute('role')).to.equal('region');
+    });
+
     it('should pick up new children', async () => {
       const newSection = document.createElement('section');
       newSection.setAttribute('expanded', '');