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 2 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
1 change: 1 addition & 0 deletions tests/unit/dialog/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ QUnit.test( "ARIA", function( assert ) {
var element = $( "<div>" ).dialog(),
wrapper = element.dialog( "widget" );
assert.equal( wrapper.attr( "role" ), "dialog", "dialog role" );
assert.equal( wrapper.attr( "aria-modal" ), "true", "aria-modal attribute" );
rpkoller marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

assert.equal( wrapper.attr( "aria-labelledby" ), wrapper.find( ".ui-dialog-title" ).attr( "id" ) );
assert.equal( wrapper.attr( "aria-describedby" ), element.attr( "id" ), "aria-describedby added" );
element.remove();
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": "true"
rpkoller marked this conversation as resolved.
Show resolved Hide resolved
} )
.appendTo( this._appendTo() );

Expand Down
Loading