Dialog: Add option to put the dialog title in a header element

[rpkoller]

PR for #2271. Initial draft adding an option based on the naming @mgol suggested on the issue (uiDialogTitleTagName ) and using span as the default. Currently working on adding tests as well in the next step.

[rpkoller]

Ok i've finished an initial draft. I am not sure if the multiline syntax and formatting i've used is correct but i had to satisfy the linter and get to a line length < 100 characters. and i've also added a test. i've created a modal dialog and set the desired tag name to h2. i've picked an html tag != the default value span. I then used the queryselector to pick the title element which has the class name of .ui-dialog-title and with the tagName property i've checked if that element is the given h2. and i thought it is unnecessary to check several different scenarios like for the aria-modal test. i thought a single one should be enough here i hope?

[rpkoller]

The good thing, the test works (the expected value is an h2 but the actual value is still a span). so my fix for the line length linting error isn't working (as a one liner it works locally).

[rpkoller]

Got the test to green with the help of @rocketeerbkw. the only thing we were not sure if it was necessary to add the new option to the common-deprecated.js file as well.

[mgol]

Thinking about it more, I have doubts about this approach. See my comments.

[mgol]

Passing user input directly to the jQuery factory is dangerous - apart from obvious abuse when people pass selectors instead of tags here, they can e.g. pass <script>, etc. Protecting from that would require significantly more code, so I'm thinking we should actually change the approach.

How about introducing an option called uiDialogTitleHeadingLevel with the default of 0 and possible values all integers between 0 & 6; you can use Number.isInteger for that. We can check the value and revert to 0 if it's invalid. Then, for 0 we use <span> but for other values we use a proper heading level.

That should cover the important use cases here and avoid possible security issues. Thoughts?

[rpkoller]

in my initial draft i kept the <> out of the option but thought it would over complicate things so i ended up with the current version. but i agree that your suggested approach would be the better choice. would something like that work?

function between ( headingLvl ) {
  return Number.isInteger( headingLvl ) && headingLvl > 0 && headingLvl <= 6 ? "h" + headingLvl : "span";
};
uiDialogTitle = $( "<" + between( this.options.uiDialogTitleHeadingLevel ) + ">" ).uniqueId().prependTo( this.uiDialogTitlebar );

i am not sure if it would be a clean code wise and i also dont know what the preferred name for the function would be. but the shown snippet is at least already working: the only difference to your suggestion is that the condition is only true between 1 and 6 everything else 0 included becomes a span.

and i would also add a few test cases.

[mgol]

That works, but since this logic is quite simple and only used in a single place, it's not necessary to create a function for it, I think. You can just define a local variable with the computed tag name & use it below.

Try to keep lines ideally below 80 characters (and definitely below 100), breaking them into multiple lines when necessary.

And yes, a few test cases would be needed, when the option was:

1. not passed (testing default behavior).
2. 0
3. 1
4. 6
5. 7 (testing that it reverts to span)
6. "foo" (testing that it reverts to span)

Those should be enough, I think. Optionally, instead of testing 1 & 5 individually, you can create a loop through values 1-6, but I don't think it's absolutely necessary.

[rpkoller]

ok thanks! the suggested changes are in place and the tests are green.

[mgol]

LGTM. I'd like one addtional review but we're not in a hurry - 1.14.0 was released today so the next release won't happen immediately.

We also need docs for this new option before we can include it. The docs for dialog are at https://api.jqueryui.com/dialog/. Can you document it? It'd be a similar entry to closeOnEscape here:
https://github.com/jquery/api.jqueryui.com/blob/1c46b8ea6ca5b0220856649283c049c4946cb391/entries/dialog.xml#L134-L137

We also need a note that 1.13.1 is the first release with this API, like the one we have here: https://api.jqueryui.com/datepicker/#option-onUpdateDatepicker. This is triggered by adding the <added> tag; see https://github.com/jquery/api.jqueryui.com/blob/1c46b8ea6ca5b0220856649283c049c4946cb391/entries/datepicker.xml#L461-L465 for the source of it.

[rpkoller]

so you mean creating a pull request for api.jqueryui..com including the note about the initial release in 1.14.1? i can create that initial draft over there.

And do i need to create an issue in the api.jqueryui.com issue queue first or would it be ok to directly open a pull request there? cuz on the jqueryui repo ive also had to use the issue number of the issue for the branch. for this issue here the branch is called for example 2271-add-option-uidialogtitle-html-element.

[mgol]

Just a PR will be fine. We can have it reference the corresponding UI issue + PR in the commit message.

[rpkoller]

oki, and should i omit using an issue number in the branch name?

[mgol]

The branch name doesn’t matter, you can use whatever is convenient for you.

[mgol]

@fnagel can you review as well?

[fnagel]

Looks good to me. Thanks for your contribution!

[mgol]

Thanks, merged! The change will arrive in jQuery UI 1.14.1.

[rpkoller]

Thank you! and one question do you already have a rough plan when you want to release 1.14.1? i ask if there is a slight chance it might be released before the first beta for the next minor release drupal (11.1) arrives , which is planned for november 11th?

[mgol]

@rpkoller yeah, a release in October is likely if nothing critical comes out and the milestone (https://github.com/jquery/jquery-ui/milestone/8) is clean; currently, there's one issue to fix there, but it's simple.

[rpkoller]

that would be perfect, thank you for the headsup!

[mgol]

@rpkoller jQuery UI 1.14.1 with this change has been released.