Dialog: Add aria-modal support

[rpkoller]

PR for the issue outlined in #2246 . Hope I've done everything in the correct manner. Was trying to follow http://contribute.jquery.org/commits-and-pull-requests/ as good as I was able to.

In regards of the PR itself, broader considerations are outlined in the corresponding issue. But I've went with adding aria-modal="true only, since it is the least invasive and breaking option in regards of backward compatibility to fix the problem. In short, elements in the background of a dialog modal are added to the AOM without the aria-modal attribute or the dialog element in place. And for the test i went with the same pattern that was applied if the role attribute is in place with a value of dialog. Therefore I've checked if the attribute aria-modal is there with a value set to true

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: rpkoller / name: Ralf Koller (729c6d0, 268dc62, 8283b78, a8a6e26, 7f88e7f, 42edf77, 2506b1c, 22ea1a7, 546144d, 4810065)
• ✅ login: mgol / name: Michał Gołębiowski-Owczarek (3da0e24, e42ab78, 67490a0, 174bb27, 2b40401)

[fnagel]

Apart from the little change I've noted this looks looks good to me, thanks for your contribution!

[fnagel]

Looks good to me!

What do you think @mgol?

[mgol]

Thanks for the PR!

One question - shouldn't setting aria-modal be tied to the modal option on the dialog widget? See https://api.jqueryui.com/dialog/#option-modal. When modal is false, the intention seems to be that you can still interact with what's under the dialog so we shouldn't set aria-modal.

@fnagel what do you think?

[rocketeerbkw]

Discussed with @rpkoller, this is a possible change to the tests given the suggested changes in https://github.com/jquery/jquery-ui/pull/2257/files#r1619122875.

if (element.dialog( "option", "modal" )) {
  assert.equal( wrapper.attr( "aria-modal" ), "true", "aria-modal attribute" );
} else {
  assert.ok( !element.attr( "aria-modal" ), "aria-modal not set" );
}

[mgol]

ifs like that are not a good practice in test - you'd still be testing only one of them and the other would be left untested. We need both tests to run. We need one dialog with the modal option set - and then verify the aria-modal attribute is set to "true", and another one without the modal option that verifies the attribute has not been set.

This test is failing right now because the dialog was created without the modal option.

I'd recommend creating a separate test just after this one titled ARIA, aria-modal where you create three dialogs - one with the modal option set to true, one to false and one with no such option passed, and verify proper aria-model value or presence for each of them.

This "ARIA" test should be reverted to its initial form, all new assertions would be added to this new test.

[rpkoller]

@mgol thanks for the explanation. but one word about the example of animated.html. i'Ve tested with safari, yes tabbing is limited within the dialog modal, you the user are unable to tab outside, but clicking outside the dialog modal you cant as well, at least it has not effect. "technically" from my understanding the general expection of clicking outside a dialog modal should actually close / collapse the modal (not only by clicking the close button). at the moment you can click everywher outside the dialog modal but it as no effect at all, for example the cursor is also not changing to a pointer hovering the open dialog button. but i disagree about the usage of aria-modal in this situation, it should be added to such dialogs, alone for the fact if the dialog modal is open and you start the rotor in voiceover you have two buttons available; you have open dialog button AND the close button both available under the form controls section as well as the buttons section. that is highly confusing already on a simple example page like that. and on a sidenot i've noticed that the dialog is also draggable. this isnt announced by the screenreader nor is the positions controlable via the keyboard (but that is completely out of the scope for this issue, just noticed while playing with the animated example).

and one additional note from the mdn docs:

If a dialog is not modal — there is no inert background and focus isn't confined to the dialog — either include aria-modal="false" or omit the attribute altogether.

that would also support the point you'Ve made with the w3c link @fnagel. and i also agreee in regards of the different behavior across browsers and AT setups. a not up to date overview is provided on here: https://a11ysupport.io/tech/aria/aria-modal_attribute

i've committed @mgols last suggestion, now using "aria-modal": this.options.modal ? "true" : null . in regards of updating the test and the requirement to also check the other cases. i've got some help from @rocketeerbkw at a local meetup last night where he came up with the suggestion he posted in the comments here. question to you two, @mgol and @fnagel, would that test be enough with that general else and the way the if is called or would it be necessary to be more granular and specific?

[mgol]

@mgol thanks for the explanation. but one word about the example of animated.html. i'Ve tested with safari, yes tabbing is limited within the dialog modal, you the user are unable to tab outside, but clicking outside the dialog modal you cant as well, at least it has not effect.

@rpkoller This is because the only thing you can interact with in this demo is the Open Dialog button and the dialog is already open. If you add another button with a handler to alert something, the alert will show up. So you definitely can interact with what's outside of the dialog without closing the dialog if the modal option is not enabled.

[mgol]

ifs like that are not a good practice in test - you'd still be testing only one of them and the other would be left untested. We need both tests to run. We need one dialog with the modal option set - and then verify the aria-modal attribute is set to "true", and another one without the modal option that verifies the attribute has not been set.

This test is failing right now because the dialog was created without the modal option.

I'd recommend creating a separate test just after this one titled ARIA, aria-modal where you create three dialogs - one with the modal option set to true, one to false and one with no such option passed, and verify proper aria-model value or presence for each of them.

This "ARIA" test should be reverted to its initial form, all new assertions would be added to this new test.

[rpkoller]

apologies just noticed your comment today. i've reverted the ARIA test back to its initial state and i will try to come up with a test called aria-modal as you suggested. will give it a try tonight and see how far i get.

[rpkoller]

please bare with me, took me a while, and as i've mentioned i am not a developer (my focus is in content, ux and a11y). but i've tried to piece together a draft for an additional test. added three assertions based on the setter in the api docs: https://api.jqueryui.com/dialog/#option-modal . not sure if that is the most elegant way. but for the first assertion i set the option to true and then check if the aria-modal attribute is there and set to true. for the option set to false i check if the aria-modal attribute is there as well but set to false. for the last i am not entirely sure but i think instead of yes or no i went with null assuming that would not add the modal option. in consequence the "value" for the aria-modal attribute is also null aka not added. that was my rational behind the draft.

[mgol]

It's option, not options, but you can see in checks on this PR you're getting test failures like:

cannot call methods on dialog prior to initialization; attempted to call method 'options'

The widget needs to be initialized first. It'd help if you read https://learn.jquery.com/jquery-ui/how-jquery-ui-works/ which explains these basics.

Here, you should just create the widget passing proper options to it as described in the "Initialization" section of that doc.

That also made me realize we also need to modify the _setOption method of the dialog. Find it in the file, you'll need to add:

		if ( key === "modal" ) {
			uiDialog.attr( "aria-modal", value ? "true" : null );
		}

in there. Unfortunately, it seems the one in _createWrapper is needed as well.

Then, we need tests for both the setting the option initially and then modifying it later to cover all grounds.

To summarize, I'd create three sections in this test - one with modal set to true: .dialog( { modal: true } ); one when it's set to false, and one when no options are passed: .dialog() (you still need to initialize like that before calling .dialog( "option", "modal", true )!)

And then, within each of those sections, first do a proper assertion and then call .dialog( "options", "modal", VALUE ) substituting VALUE for these three options and check again if the value is as expected. That should give you total 12 assertions in this test.

[rpkoller]

thanks a lot for the detailed explanation, that helped figuring things out, even though it was way out of my comfort zone. but with the help of @rocketeerbkw i was able to finish the draft thursday night.

[mgol]

Looks good! Just a few suggestions for tests.

[mgol]

Looks good! Just a few suggestions for tests.

[mgol]

Looks good! Just a few suggestions for tests.

[mgol]

@rpkoller I added a few minor test tweaks by myself and landed the PR. Thanks for going through the whole review process!

[mgol]

@rpkoller This patch has been released as part of jQuery UI 1.14.0-beta.2: https://blog.jqueryui.com/2024/06/jquery-ui-1-14-0-beta-2-released/. I'd appreciate testing if it works as intended! 🙂

[rpkoller]

yay, and thanks to you @mgol for your patience and all the explanations along the way! and in regards of testing, there was a new MR created in drupal for beta2. i've tested the MR now, it was applied correctly, but somehow the aria-modal wasn't added. As you know i am not a developer therefore i never really minded the technical details on the drupal end implementation wise. but I've asked on the issue how the dialog modal is initialized in drupal and also referenced this PR. I'll keep you posted as soon as i get any feedback.

[rpkoller]

just a brief update. things look good. beta2 got merged into drupal 11.x three days ago. the only change necessary, 'aria-modal': this.options.modal ? 'true' : null,had to be added since drupal has a custom wrapper for dialog.js. with that in place everything works like a charm. the aria-modal attribute is getting added if the modal option is set to true and all the existing functionality still works as expected.

[mgol]

@rpkoller thanks for the update!