From a2584c3a3e4ff4910cae167f314926891c1df4ea Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Wed, 28 Feb 2024 15:51:11 -0500 Subject: [PATCH 1/4] test(modal): add 1 passing and 1 failing test, remove redundant test --- .../components/modal/test/a11y/modal.spec.ts | 23 ------------ .../modal/test/modal-attributes.spec.tsx | 36 +++++++++++++++++++ 2 files changed, 36 insertions(+), 23 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/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..571cfad618c --- /dev/null +++ b/core/src/components/modal/test/modal-attributes.spec.tsx @@ -0,0 +1,36 @@ +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'); +}); From 6ddd3311519554712c6e5f1bd44b1682f5f039dd Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Wed, 28 Feb 2024 16:08:11 -0500 Subject: [PATCH 2/4] update test --- core/src/components/modal/test/modal-attributes.spec.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/components/modal/test/modal-attributes.spec.tsx b/core/src/components/modal/test/modal-attributes.spec.tsx index 571cfad618c..76d9b0aafd3 100644 --- a/core/src/components/modal/test/modal-attributes.spec.tsx +++ b/core/src/components/modal/test/modal-attributes.spec.tsx @@ -24,7 +24,7 @@ it('should inherit attributes when set via htmlAttributes', async () => { const page = await newSpecPage({ components: [Modal], template: () => ( - + ), }); @@ -33,4 +33,7 @@ it('should inherit attributes when set via htmlAttributes', async () => { 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); }); From b49c31312844ef0d59a1896990338686297bd257 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Wed, 28 Feb 2024 16:12:14 -0500 Subject: [PATCH 3/4] fix(modal): ariaLabel and role are inherited when set via htmlAttributes --- core/src/components/modal/modal.tsx | 41 +++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/core/src/components/modal/modal.tsx b/core/src/components/modal/modal.tsx index a02552727eb..c2ce6fc569f 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; From 6bcd49815ce8ae257788ea1943599f11ab2ede98 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Wed, 28 Feb 2024 16:17:09 -0500 Subject: [PATCH 4/4] chore: lint --- core/src/components/modal/modal.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/components/modal/modal.tsx b/core/src/components/modal/modal.tsx index c2ce6fc569f..2d84e42c964 100644 --- a/core/src/components/modal/modal.tsx +++ b/core/src/components/modal/modal.tsx @@ -379,8 +379,8 @@ export class Modal implements ComponentInterface, OverlayInterface { */ this.inheritedAttributes = { ...this.inheritedAttributes, - [attribute]: htmlAttributes[attribute] - } + [attribute]: htmlAttributes[attribute], + }; delete htmlAttributes[attribute]; }