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

[liamdebeasi]

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
• No

Other information

Dev build: 7.7.4-dev.11709154993.1b49c313

[liamdebeasi]

I wanted to add tests for all of the inherited roles, and I also wanted to test the htmlAttributes behavior that I added in this PR. As a result, I decided to remove this test in favor of more generic attribute tests.

[liamdebeasi]

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.

[averyrousseau]

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

[liamdebeasi]

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.

[averyrousseau]

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