From de13633a182d963876434db773aa346833f956fd Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 29 Feb 2024 11:20:34 -0500 Subject: [PATCH] fix(modal): ariaLabel and role are inherited when set via htmlAttributes (#29099) Issue number: Internal --------- ## What is the current behavior? Modal inherits `aria-label` and `role` to an element inside of its Shadow DOM. However, this only works if developers set the attributes on the host element directly. Setting the attributes via the `htmlAttributes` property causes the attributes to be set on the host and not inherited. ## What is the new behavior? - `aria-label` and `role` are inherited even when set using `htmlAttributes`. ## Does this introduce a breaking change? - [ ] Yes - [x] No ## Other information Dev build: `7.7.4-dev.11709154993.1b49c313` --- core/src/components/modal/modal.tsx | 41 ++++++++++++++++++- .../components/modal/test/a11y/modal.spec.ts | 23 ----------- .../modal/test/modal-attributes.spec.tsx | 39 ++++++++++++++++++ 3 files changed, 78 insertions(+), 25 deletions(-) delete mode 100644 core/src/components/modal/test/a11y/modal.spec.ts create mode 100644 core/src/components/modal/test/modal-attributes.spec.tsx diff --git a/core/src/components/modal/modal.tsx b/core/src/components/modal/modal.tsx index a02552727eb..2d84e42c964 100644 --- a/core/src/components/modal/modal.tsx +++ b/core/src/components/modal/modal.tsx @@ -345,10 +345,47 @@ export class Modal implements ComponentInterface, OverlayInterface { } componentWillLoad() { - const { breakpoints, initialBreakpoint, el } = this; + const { breakpoints, initialBreakpoint, el, htmlAttributes } = this; const isSheetModal = (this.isSheetModal = breakpoints !== undefined && initialBreakpoint !== undefined); - this.inheritedAttributes = inheritAttributes(el, ['aria-label', 'role']); + const attributesToInherit = ['aria-label', 'role']; + this.inheritedAttributes = inheritAttributes(el, attributesToInherit); + + /** + * When using a controller modal you can set attributes + * using the htmlAttributes property. Since the above attributes + * need to be inherited inside of the modal, we need to look + * and see if these attributes are being set via htmlAttributes. + * + * We could alternatively move this to componentDidLoad to simplify the work + * here, but we'd then need to make inheritedAttributes a State variable, + * thus causing another render to always happen after the first render. + */ + if (htmlAttributes !== undefined) { + attributesToInherit.forEach((attribute) => { + const attributeValue = htmlAttributes[attribute]; + if (attributeValue) { + /** + * If an attribute we need to inherit was + * set using htmlAttributes then add it to + * inheritedAttributes and remove it from htmlAttributes. + * This ensures the attribute is inherited and not + * set on the host. + * + * In this case, if an inherited attribute is set + * on the host element and using htmlAttributes then + * htmlAttributes wins, but that's not a pattern that we recommend. + * The only time you'd need htmlAttributes is when using modalController. + */ + this.inheritedAttributes = { + ...this.inheritedAttributes, + [attribute]: htmlAttributes[attribute], + }; + + delete htmlAttributes[attribute]; + } + }); + } if (isSheetModal) { this.currentBreakpoint = this.initialBreakpoint; diff --git a/core/src/components/modal/test/a11y/modal.spec.ts b/core/src/components/modal/test/a11y/modal.spec.ts deleted file mode 100644 index a80c33e0b82..00000000000 --- a/core/src/components/modal/test/a11y/modal.spec.ts +++ /dev/null @@ -1,23 +0,0 @@ -import { newSpecPage } from '@stencil/core/testing'; - -import { Modal } from '../../modal'; - -describe('modal: a11y', () => { - it('should allow for custom role', async () => { - /** - * Note: This example should not be used in production. - * This only serves to check that `role` can be customized. - */ - const page = await newSpecPage({ - components: [Modal], - html: ` - - `, - }); - - const modal = page.body.querySelector('ion-modal')!; - const modalWrapper = modal.shadowRoot!.querySelector('.modal-wrapper')!; - - await expect(modalWrapper.getAttribute('role')).toBe('alertdialog'); - }); -}); diff --git a/core/src/components/modal/test/modal-attributes.spec.tsx b/core/src/components/modal/test/modal-attributes.spec.tsx new file mode 100644 index 00000000000..76d9b0aafd3 --- /dev/null +++ b/core/src/components/modal/test/modal-attributes.spec.tsx @@ -0,0 +1,39 @@ +import { h } from '@stencil/core'; +import { newSpecPage } from '@stencil/core/testing'; + +import { Modal } from '../modal'; + +it('should inherit attributes', async () => { + /** + * Note: This example should not be used in production. + * This only serves to check that `role` can be customized. + */ + const page = await newSpecPage({ + components: [Modal], + template: () => , + }); + + const modal = page.body.querySelector('ion-modal')!; + const contentWrapper = modal.shadowRoot!.querySelector('[part="content"]')!; + + expect(contentWrapper.getAttribute('aria-label')).toBe('my label'); + expect(contentWrapper.getAttribute('role')).toBe('presentation'); +}); + +it('should inherit attributes when set via htmlAttributes', async () => { + const page = await newSpecPage({ + components: [Modal], + template: () => ( + + ), + }); + + const modal = page.body.querySelector('ion-modal')!; + const contentWrapper = modal.shadowRoot!.querySelector('[part="content"]')!; + + expect(contentWrapper.getAttribute('aria-label')).toBe('my label'); + expect(contentWrapper.getAttribute('role')).toBe('presentation'); + + expect(modal.hasAttribute('aria-label')).toBe(false); + expect(modal.hasAttribute('role')).toBe(false); +});