Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(modal): ariaLabel and role are inherited when set via htmlAttributes #29099

Merged
merged 4 commits into from
Feb 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions core/src/components/modal/modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +360 to +362
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to other ideas since the code is a bit gnarly. I did consider using the Watch callback for attributes, but I realized that wouldn't help us since Watch callbacks do not get run when the component is loaded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this approach. There's probably a cleaner way of doing it with some clever use of Object.keys()/filter()/reduce(), but the general strategy works for me. I came up with this after staring at it for 20 minutes; might be slightly easier to grok and it cuts a layer of nesting.

if (htmlAttributes !== undefined) {
  const htmlAttributesToInherit = Object.keys(htmlAttributes).filter(attr => attr in attributesToInherit);
  htmlAttributesToInherit.forEach((attribute) => {
    this.inheritedAttributes = {
      ...this.inheritedAttributes,
      [attribute]: htmlAttributes[attribute],
    };

    delete htmlAttributes[attribute];
  });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up just sticking with the approach in the PR. The Object.keys approach has some scalability concerns since worst case you'll need to iterate through htmlAttributes 3 times. (Object.keys, then filtering the array, then iterating through the result). We only inherit 2 attributes, so the performance impact right now is negligible. But if we were to increase the number of attributes inherited this would become more of a problem.

*/
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;
Expand Down
23 changes: 0 additions & 23 deletions core/src/components/modal/test/a11y/modal.spec.ts

This file was deleted.

39 changes: 39 additions & 0 deletions core/src/components/modal/test/modal-attributes.spec.tsx
Original file line number Diff line number Diff line change
@@ -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: () => <ion-modal overlayIndex={1} aria-label="my label" role="presentation"></ion-modal>,
});

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: () => (
<ion-modal overlayIndex={1} htmlAttributes={{ 'aria-label': 'my label', role: 'presentation' }}></ion-modal>
),
});

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);
});
Loading