Update to jQuery 1.14.1 and use the newly added option for dialog modal headings

forgot the ui in jquery ui , updated title and issue summary

I'd love to see this brought into Core. This adds some missing semantics to our work.

I'Ve updated the title and rescoped the issue proposed resolution since jquery ui 1.14.1 got updated 📌 Update all JavaScript dependencies which cause no changes Active . My initial proposed resolution by adding uiDialogTitleHeadingLevel: 1 after line 40 is wrong (tested it manually altering 11.x locally) . i was under the impression it would be a similar one liner like for adding the aria-modal functionality added for 11.0.0, but it seems not. reason is the _createWrapper method was already available in the dialog.jquery-ui.js but the _createTitlebar method is not available there yet.

Initially filed it as a task because of the need of updating jquery but the remaining step about changing the wrapping element of the title i would consider a bugfix.

Opened an initial MR. It is setting the option uiDialogTitleHeadingLevel that was newly introduced with jQuery UI 1.4.1 to 1 on dialog.showModal method. That way it is ensured that all dialogs modals have their title properly wrapped in an h1 instead of a span, the previous default. Since Drupal 11.0.0 and the update to jQuery UI 1.14.0 dialog modals got the aria-modal attribute added which ensures that all elements in the background of the dialog modal are removed from the accessibility object model, so the main or sometimes sole heading within a dialog modal is the title, and therefore it is advised to go with the h1 element for the title wrapper. In addition to that i've also set the margin for .ui-dialog .ui-dialog-titlebar .ui-dialog-title to zero, because the change to the h1 added an additional margin to the layout and by setting it to zero it readjusts it to the previous state of the design.

*@rocketeerbkw worked with me on the upstream change (https://github.com/jquery/jquery-ui/issues/2271) as well as on this issue here

I was able to fix the stylelint issue but the phpunit ones i feel unable to solve and they look from my none developer perspective unrelated? the errors show in the context of package manager and i've only added an option to a javascript file? if someone else could take a look please who is more familiar with this kind of stuff. thank you.

The pipeline is running all green now. I had to re-run the failing tests.

However, test coverage is needed if this is going to make it into core.

So the FunctionalJavascript? test seems to be defined as follows, from #9:

That way it is ensured that all dialogs modals have their title properly wrapped in an h1 instead of a span...

That is, the test needs to prove that at least one dialog modal does not have its title wrapped in a but rather in an
element.

uh that is odd that the tests got green without any changes to the MR.

And about the to be added tests. i've tried to chop together a starting point. I am not a developer and haven't done any work on tests so far (only managed to create the upstream changes for jquery ui in relation to dialog modals including the necessary tests for both PRs). The first PR added the aria-modal attribute the dialog. when that was added to Drupal 11.0.0/10.3.0 there was no test added. therefore i think it would be good aside testing that the title is wrapped in a h1 also to test that the dialog modal also got an aria-modal attribute added. looking at the existing tests, there is core/tests/Drupal/FunctionalJavascriptTests/Dialog/DialogPositionTest.php, and i thought it might be the easiest to simply add the two necessary assertions here. even though it is against the single responsibility principle and the test is mainly about the positioning of the dialog modal. but if a dedicated test making sure the aria-modal attribute is set and the title wrapped in a h1, that test would contain a good share of steps DialogPositionTest.php already contains - so basically all those steps would be redundant and unnecessary. so i simply added the assertions to DialogPositionTest.php for now and added the reasoning and what is happening to a comment. as i said it is only a starting point. if people think it would be better to move those assertions to a dedicated test and or if the assertions need work please correct me and update the MR.

@rkoller, That's great! I agree we should test for the aria-modal attribute too. Yes, there is a tension between DRY and single responsibility principle here. But in test code I have seen DRY win over single reponsibility before. It happens.

ha i think we can stay "relatively" dry. the tests failed... the pipeline that contains the test where the two new asserts got added passed https://git.drupalcode.org/issue/drupal-3485202/-/jobs/3825700 :D buuuut another pipeline failed https://git.drupalcode.org/issue/drupal-3485202/-/jobs/3825702 and it looks like there is another test that tests not the positioning but the dialog in general (DialogTest) ... and the two asserts that fail there check for span.ui-dialog-title:contains('AJAX Dialog & contents') ... so i guess i remove the tests from DialogPositionTest, and update the selector in DialogTest and add the assert for the aria-modal there instead as well including the comment . and about the failing nightwatch test https://git.drupalcode.org/issue/drupal-3485202/-/jobs/3825720 i will see later, seems unrelated.

@rkoller I have added feedback to the test code. I think you have to correct one of your assertions. Not sure which to use but you want to prove that getAttribute() does not return an empty string. Or better that it returns the value of the attribute.

It looks like to make the DialogTest.php tests pass you just need to change e.g.: $dialog_title = $link1_dialog_div->find('css', "span.ui-dialog-title:contains('AJAX Dialog & contents')");
to
$dialog_title = $link1_dialog_div->find('css', "h1.ui-dialog-title:contains('AJAX Dialog & contents')");

Applied code fix to DialogTest.php to make it pass.

thanks! but on a closer look it is surprising that the tests pass with your recent changes. cuz you've applied the change from span to h1 for every occasion of span. but some of the test cases are also for dialogs that are no modals (for example line 119 and 121 in the context of link 7 - non-modal, no target), cuz h1 is only set for dialog that are actual modals at this point in dialog.js

    dialog.show = () => {
      openDialog({ modal: false });
    };
    dialog.showModal = () => {
      openDialog({ modal: true, uiDialogTitleHeadingLevel: 1 });
    };

but maybe a good reminder to change the wrapping element for dialogs that are no modals as well? i would lean towards h2, might be a relatively save call in this case, but i've asked @mgifford and will report back when i get feedback.

and about the assertion about aria-modal and your comment:

The return value of getAttribute() is here. It is not true or false it is the value of the attribute or an empty string. So asserting that $dialog->getAttribute('aria-modal') is true or not true does not work. Okay, it seems the value of the 'aria-modal' attribute should be true. So that should be fine.

does that mean that $this->assertEquals('true', $dialog->getAttribute('aria-modal'), 'Dialog modal has aria-modal attribute'); would be fine? at least the same pattern is used on for example in FromGroupingElementsTest.php in line 136, therefore i thought it would be save to use, otherwise it would have to be corrected in more places in core tests? and i am still uncertain where to place that assertEquals in the DialogTest.php

@rkoller DialogTest.php works now. I fixed each pair of asserts locally using PHPUNIT. With the existing span.ui-dialog... selector the asserts were failing and with h1.ui-dialog... When I fixed the first pair in the file the test progressed to the next pair and failed on them. I think there were 6 or 7 asserts needing span changed to h1 in total.

Either the tests in DialogTest.php are defective which will require raising an issue or they are doing their job, in which case there is no need to do anything more inside the file (unless someone changes code that breaks the tests again). I think you are mistaken. h1 is applied to all modals which is why the tests in DialogTest.php are passing. It may be surprising.

As far as my code comment and #17 go, the two assertions are defective. They are passing when they should be failing. To get a handle on the reason it is worth trying to assert eg that 'span etc' does not exist. That will fail without the MR fix and pass with it. It will help to identify why the current test coverage is passing when it should not.

but if you install the ajax-test module the onethe tests are using for the given tests and you take a look at link 1 (modal) then this modals title is an h1 with an aria-modal="true" attribute while if you take a look at link 7 it opens a dialog that has no aria-modal attribute and the title is wrapped in a span. therefore the assertion in 119 and 121 would have to fail imho currently since they check for h1 and that h1 is not in place but a span instead still?! is it possible that the reason for the tests not failing is because modals and none modals are open both?! cuz the different dialogs are opened in the dialogTest.php but only the first opened dialog is also closed in line 70 with $close_button->press();, the rest is kept open?

@rkoller That is interesting. It could be that the tests in DialogTest.php have exposed their limitations and so they need to be changed, too.

the assertion in 119 and 121 would have to fail

It seems here that you are trying to beat the test. The tests are never wrong. The way we read them can be wrong. Sometimes the best thing is to try out asserts that have to fail. Try different asserts each will give more information not just about what the asserts do but on what is actually happening inside the tests.

@rkoller The assertion at 119 searches the page for an element identified by the selector 'h1.ui-dialog-title' and that contains the text, 'AJAX Dialog & contents'. I am not sure how 'link 7' is relevant?

i was completely wrong and got confused by the several context switches in a row. in the given example with 119 and 121 the point of reference was NOT link 7 which is a none modal but button 1 which is a modal (line 113) and which i've missed. so the tests are correct. managed to get the local tests running and therefore i was able to validate and cross check. so your changes are all correct and doesnt look like a shortcoming or bug of the existing tests.

and i'Ve pushed another commit that contains two asserts that add checks for the aria-modal attribute. locally those work for me.

I see one failing test the 4/8 one, i think? The test that fails looks unrelated. If re-trigger 4/8 it should pass and you will have all green pipeline. Then you should run the 'test-only' test. It has be to run manually. That test is critical because it automatically finds any test code in the MR and isolates it; then runs it against the 11.x branch (the one our MR will finally be merged into). What we want is to see that test fail.

hm i keep getting errors. the first two times i've restarted the 4/8 functional test you've mentioned, it failed each time with another mismatch for a string. totally unrelated to the changes in this MR. i've reran all tests now, again functional test 4 out of 8 fails but this time with a 403... plus an error in a test for package manager: https://git.drupalcode.org/issue/drupal-3485202/-/jobs/3827987 :/ confusing and "sort of" arbitrary.

ahhhhh and now i see the test only changes test. understood. thank you for the pointer, i would have missed that!

@rkoller Looks good. Pipeline all green. Now you should run the 'test-only changes' test.

i ran the test-only test now, but unfortunately it succeeded, and it sounded like it would have to fail instead?!

Yes, correct. If you can run the tests locally i recommend you start off by creating an assertion that has to fail. It proves that everything works. Even if it is 4 is not 2 or false is not true. From there you should figure it out. Try a commit to the pipeline if it helps convincing you that the pipeline is working right.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2025-01-03 Active . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @benjifisher, @rkoller, and @simohell.

We’ve discussed the open question whether a section heading element should be used to wrap the title of a non-modal dialog same as the title of modal dialogs, and if so, which heading level should be chosen? There was the clear consensus that same as for the modal dialogs, any section heading element would a clear improvement over a span element. Since the non-modal dialog isn’t wrapped into an article or section element, it wouldn’t be advisable to go with an h1 just for consistency reasons; any page the non modal dialog is invoked on might probably already have an h1 element. Going with an h2 instead would the most neutral semantic choice, and accept in consequence that there “might” be a potential skip of a heading level. So although hierarchical gapless heading structures reflect the best practice, skipping heading levels does not represent a WCAG failure.(https://www.tpgi.com/heading-off-confusion-when-do-headings-fail-wcag/).

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

p.s. @mgifford agreed and leaned towards the proposed resolution going with an h2 for non modal dialogs in a thread on the Drupal Slack https://drupal.slack.com/archives/C1AFW2ZPD/p1735760864177209 in the run-up to the meeting, where i’ve asked for more opinions in the #ux and #accessibility channel.

@oily i've already added the changes we've discussed yesterday outlined in #30 📌 Update to jQuery 1.14.1 and use the newly added option for dialog modal headings Active to the MR last night (basically added the h2 to non modal dialogs and included an assertion that one of the non modal dialog examples has their title wrapped in an h2).

I got the automated tests green again but the test-only changes confuse me and remain green/passing. i followed your recommendation and tested locally. i simply copied the content of DialogTest.php into the clipboard, then checked out 11.x, then opened DialogTest.php again and pasted/replaced the test code with the one from the MR, and finally reran that test on 11.x with the test code from the MR.... there things fail as expected:

FAILURES!
Tests: 2, Assertions: 14, Failures: 1, PHPUnit Deprecations: 1.
Failed to run phpunit core/tests/Drupal/FunctionalJavascriptTests/Ajax/DialogTest.php: exit status 1

but on https://git.drupalcode.org/issue/drupal-3485202/-/jobs/3890198 it looks like tests are being skipped(?!?) and that is the reason why the job is shown as passed?

OK, but some tests were skipped!
Tests: 2, Assertions: 0, Skipped: 2.

@rkoller Thank you for the details of the meeting and the test-only test. This may be an issue with the pipeline. It happened recently on another issue i was working on. Local testing proved that test coverage was effective. You could raise this in the #contribute slack channel. Else the reviewer can test locally.

i've already posted in #testing last night https://drupal.slack.com/archives/C223PR743/p1735958223766589 and asked there, but no answer yet. will give it another day or two and then repost in contribute (or comment in one or another thread that i found in the context of test-only tests not failing). but aside that detail i leave the issue at needs review so people can take a look at the recent changes. think the only thing left to do aside another round of review might and figuring out the test-only problem is updating the issue summary so it reflects the recent changes. will do so the next days.

@rkoller Thank you for the work!

Updated the issue summary