Change the element wrapping the title in dialogs from span to heading element

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

I'Ve updated the title and rescoped the issue proposed resolution since jquery ui 1.14.1 got updated #3493146: Update all JavaScript dependencies which cause no changes. 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.

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.

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.

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

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?

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.

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!

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

Usability review

We discussed this issue at #3495184: Drupal Usability Meeting 2025-01-03. 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 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.

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.

Updated the issue summary

I've removed the fail-on-skip as well as the debug flag from test-only.sh and also ran a rebase after #3494332: Functional Javascript tests are silently skipped on Gitlab CI due to selenium standalone configuration went in. fingers crossed that the test-only tests aren't skipped anymore and properly fail now.

@oily since you were following along #3494332: Functional Javascript tests are silently skipped on Gitlab CI due to selenium standalone configuration as well. i ran the test-only test now after the rebase and the job failed: https://git.drupalcode.org/issue/drupal-3485202/-/jobs/4151589 . but to me it looks like the testing "infrastructure" aka selenium failed but not the actual test?! do i read that correctly? i "thought" the changes from #3494332: Functional Javascript tests are silently skipped on Gitlab CI due to selenium standalone configuration were contained in that rebase?

The test-only test is failing as it should.

yeah but if you take a look at the output as noted in #41 you'll notice the two failures were:

Caused by
WebDriver\Exception\CurlExec: Could not resolve host: selenium

that is an infrastructure specific error not an error based on what is actually tested against with this MR?

@oily the fail was a false fail. after some more investigation and some advice from @catch it turns out the tests-only pipeline service definition has to be changed from with-chrome to with-selenium-chrome... the last two commits tested that on this issue. made the change on this MR, the test-only test then correctly failed with

There was 1 failure:
1) Drupal\FunctionalJavascriptTests\Ajax\DialogTest::testDialog
Failed asserting that null is not null.
/builds/issue/drupal-3485202/core/tests/Drupal/FunctionalJavascriptTests/Ajax/DialogTest.php:64
FAILURES!
Tests: 2, Assertions: 14, Failures: 1.

(see https://git.drupalcode.org/issue/drupal-3485202/-/jobs/4155477) . but per the recommendation of @catch i've created a new issue that makes that one line change. #3502629: Change the test-only pipeline service from with-chrome to with-selenium-chrome. when this issue is in then we can rebase this issue and can come to a proper finish.

@oily since you are kindly following along. i would have one question. if you see the comments here in this thread, between #44 and #45 i've committed and tested the changes that got into #3502629: Change the test-only pipeline service from with-chrome to with-selenium-chrome and then committed and went back to the previous state with *with-chrome... now #3502629: Change the test-only pipeline service from with-chrome to with-selenium-chrome got fixed, so i went ahead and rebased this MR to get the changes in. problem is. if you take a look at https://git.drupalcode.org/project/drupal/-/merge_requests/10631/diffs#n... , there are changes for the pipeline.yml. it somehow the changes introduced in the other issue and that got added by rebase are getting invalidated and the service changed to *with-chrome. looking at https://git.drupalcode.org/project/drupal/-/merge_requests/10631/commits... is it possible that on a rebase all the commits on a MR are committed again after the rebased core? and that is the reason the change gets reverted again back to *with-chrome with ba444f84??? and my question is how to fix that?

I've simply pushed another commit now reverting the changes of the commit on the MR from the 27th of january invalidating the changes in the pipeline.yml (4th february) that got in with the rebase... tests are green now and the test only fails properly now as well: https://git.drupalcode.org/issue/drupal-3485202/-/jobs/4243209 ... so the only thing left to do is still another pair of eyes reviewing the MR.