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.

Sign up for GitHub

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

Dialog: Add aria-modal support #2257

Merged
merged 15 commits into from
Jun 14, 2024
Merged

Dialog: Add aria-modal support #2257

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a8a6e26
Add aria-modal attribute true to dialog modal wrapper
rpkoller May 24, 2024
729c6d0
Add a test if aria-modal attribute is there and true
rpkoller May 24, 2024
268dc62
fix: increasing # of expected asserts from 4 to 5
rpkoller May 24, 2024
7f88e7f
only add aria-modal when this.options.modal is set to true
rpkoller May 31, 2024
22ea1a7
revert the ARIA test back to its initial form per @mgols recommendation
rpkoller Jun 7, 2024
546144d
Initial draft for the aria-modal test
rpkoller Jun 11, 2024
2506b1c
Fixing initial draft that contained odd unintended changes
rpkoller Jun 11, 2024
4810065
change option to options for the dialog modal options
rpkoller Jun 12, 2024
8283b78
Add check for modal to _setOption method
rpkoller Jun 14, 2024
42edf77
applying mgol's suggested changes to the aria-modal test
rpkoller Jun 14, 2024
174bb27
Apply suggestions from code review
mgol Jun 14, 2024
e42ab78
Avoid setting the modal option to null
mgol Jun 14, 2024
3da0e24
Update tests/unit/dialog/core.js
mgol Jun 14, 2024
67490a0
Only declare element & wrapper once in tests
mgol Jun 14, 2024
2b40401
Fix indentation
mgol Jun 14, 2024
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
20 changes: 20 additions & 0 deletions tests/unit/dialog/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,26 @@ QUnit.test( "ARIA", function( assert ) {
element.remove();
} );

QUnit.test( "aria-modal", function( assert ) {
assert.expect( 3 );

mgol marked this conversation as resolved.
Show resolved Hide resolved
var element = $( "<div>" ).dialog( "options", "modal", true ),
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

wrapper = element.dialog( "widget" );
assert.equal( wrapper.attr( "aria-modal" ), "true", "aria-modal attribute set to true" );
element.remove();

var element = $( "<div>" ).dialog( "options", "modal", false ),
wrapper = element.dialog( "widget" );
assert.equal( wrapper.attr( "aria-modal" ), "false", "aria-modal attribute set to false" );
element.remove();

var element = $( "<div>" ).dialog( "options", "modal", null ),
wrapper = element.dialog( "widget" );
assert.equal( wrapper.attr( "aria-modal" ), null, "no aria-modal attribute added" );
element.remove();
} );


QUnit.test( "widget method", function( assert ) {
assert.expect( 1 );
var dialog = $( "<div>" ).appendTo( "#qunit-fixture" ).dialog();
Expand Down
3 changes: 2 additions & 1 deletion ui/widgets/dialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,8 @@ $.widget( "ui.dialog", {

// Setting tabIndex makes the div focusable
tabIndex: -1,
role: "dialog"
role: "dialog",
"aria-modal": this.options.modal ? "true" : null
} )
.appendTo( this._appendTo() );

Expand Down
Loading