Removal of backdrop click close trigger & addition of closable boolean for modal component

[brenner-company]

An enhancement for the AuModal component regarding the following:

• The addition of an adjustable close trigger list.
• The removal of the escape key close trigger (as mentioned here https://github.com/appuniversum/ember-appuniversum/issues/411#issuecomment-1659786997)
• The addition of a closable boolean.

More information can be found inside the complementary FR: https://github.com/appuniversum/ember-appuniversum/issues/411.

Closes #411

[brenner-company]

Note: I'll add some test covering the new functionality if this PR gets more tangible or gets finalized.

[brenner-company]

@Windvis Should I also add a test covering @closable (when using close button within modal header & escape key)?

[brenner-company]

@Windvis: I've added the test (https://github.com/appuniversum/ember-appuniversum/pull/412/commits/7c90f022896abba45c710f772ac60a60cfa4a1f8) in the meantime, but with two remarks/questions:

• The test fails at the moment because of the following: when settings @closable to false and providing a blank modal with no title, body & footer, there is no element for focus-trap to focus to so it throws an error (as expected). Two things I noticed or want to get cleared up a bit:
  • When looking at the getter initialFocus I see that .au-c-modal__title is returned as a default, but the element is actually not focusable (to focus-trap). Should this be corrected? Tying in with the failing test above, this could be a possible solution (by just adding a title to the test modal).
  • But that is still not completely covering blank modals (even without a title). Is it maybe an idea to provide a fallbackFocus element (as mentioned here: https://github.com/focus-trap/focus-trap#createoptions) to make sure focus-trap doesn't throw an error? Maybe the modal element itself (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog#accessibility_considerations mentions it as a possibility, though not a common one)?
• Should I also add the escape key press to this test or is the clicking of the close button (within the header) sufficient for this?

@Dietr If you want to chime in on the first part of the two questions, feel free!

[brenner-company]

@Windvis Is possible to give this a (finalizing) look in the following days? (I'm asking because this missing feature is blocking a story within Kaleidos)

[Windvis]

• The test fails at the moment because of the following: when settings @closable to false and providing a blank modal with no title, body & footer, there is no element for focus-trap to focus to so it throws an error (as expected). Two things I noticed or want to get cleared up a bit:

  • When looking at the getter initialFocus I see that .au-c-modal__title is returned as a default, but the element is actually not focusable (to focus-trap). Should this be corrected? Tying in with the failing test above, this could be a possible solution (by just adding a title to the test modal).

I don't know, sorry. I have little experience with element focusing. Why don't the other tests fail? focus trap tries to focus the close button or something?

• But that is still not completely covering blank modals (even without a title). Is it maybe an idea to provide a fallbackFocus element (as mentioned here: https://github.com/focus-trap/focus-trap#createoptions) to make sure focus-trap doesn't throw an error? Maybe the modal element itself (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog#accessibility_considerations mentions it as a possibility, though not a common one)?

You want to prevent that focus-trap throws exceptions because it's an implementation detail of the component? So if I understand it correctly, the difference between initialFocus and fallbackFocus is that the latter won't throw an error if there are no focusable elements inside? Maybe we should use that instead of the initialFocus default value then?

• Should I also add the escape key press to this test or is the clicking of the close button (within the header) sufficient for this?

I think it might be nice to test the escape key press as well, but that can be copy pasted from the other test.

[Windvis]

Btw, the title / description is a bit confusing. It's the backdrop close that was removed, right? The others are just conditional based on @closable?

[brenner-company]

I don't know, sorry. I have little experience with element focusing. Why don't the other tests fail? focus trap tries to focus the close button or something?

That is correct. In all the other cases there is a close button to focus to. Normally, this would also be the case for the title, but as mentioned, that doesn't work 😔

So if I understand it correctly, the difference between initialFocus and fallbackFocus is that the latter won't throw an error if there are no focusable elements inside? Maybe we should use that instead of the initialFocus default value then?

I'm not entirely sure, but I'll give a quick test today and let you know.

I think it might be nice to test the escape key press as well, but that can be copy pasted from the other test.

Thanks, will do!

[brenner-company]

Btw, the title / description is a bit confusing. It's the backdrop close that was removed, right? The others are just conditional based on @closable?

Damn, you're correct, I'll correct that!

[brenner-company]

I've made a small recording of the 'new' focus behaviour within the modal (with added dotted outlines for modal title & modal for easier focus visualisation):

https://github.com/appuniversum/ember-appuniversum/assets/18174827/d45ba06e-f741-4af2-a390-b86c314bb0b3

Seems good right? (@Dietr, feel free to interject if needed)