Skip to content

Commit

Permalink
✨ [bento][amp-accordion][fixit] Check for pre-existing role attribute…
Browse files Browse the repository at this point in the history
… before using default value (#32915)

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

* Update where role is passed

* Add explainer comment in code
  • Loading branch information
krdwan committed Feb 26, 2021
1 parent a1fb3c7 commit b2291b5
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 0 deletions.
9 changes: 9 additions & 0 deletions extensions/amp-accordion/1.0/base-element.js
Expand Up @@ -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}>
Expand Down
36 changes: 36 additions & 0 deletions extensions/amp-accordion/1.0/test/test-accordion.js
Expand Up @@ -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');
Expand Down
37 changes: 37 additions & 0 deletions extensions/amp-accordion/1.0/test/test-amp-accordion.js
Expand Up @@ -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', '');
Expand Down

0 comments on commit b2291b5

Please sign in to comment.