Skip to content

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

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

Merged
merged 17 commits into from
Sep 9, 2024
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
14dc459
Add option to set html tag for dialog modal title
rpkoller Jul 31, 2024
c22ba5b
Fix the line length lint error
rpkoller Aug 1, 2024
564b793
Add test to assert if the html tag is correctly set
rpkoller Aug 1, 2024
6f8307d
tagName is not a function. Removed parenthesis
rpkoller Aug 1, 2024
8f8c2a1
add the missing tab for the second line of uiDialogTitle
rpkoller Aug 1, 2024
a277562
move the <> to the option
rpkoller Aug 1, 2024
9238fc0
adjust to the changes in dialog.js and set the option to <h2>
rpkoller Aug 1, 2024
d4aa8a0
fix for the test
rpkoller Aug 2, 2024
b2f3e72
fix linting errors on the test
rpkoller Aug 2, 2024
7b9ea13
fix another linting error on the test
rpkoller Aug 2, 2024
478e434
add new option to common.js and common-deprecated.js
rpkoller Aug 2, 2024
140fa73
use and accept integer values only for html element
rpkoller Aug 5, 2024
f46ff12
Add new tests for dialog title heading levels
rpkoller Aug 5, 2024
6ffab2e
change to the new default value 0 for uiDialogTitleHeadingLevel
rpkoller Aug 5, 2024
5652776
fix linting errors
rpkoller Aug 5, 2024
c70786d
fix dialog title heading level tests
rpkoller Aug 5, 2024
8f583cb
add another assertion for the dialog title heading lvl with option foo
rpkoller Aug 5, 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/common-deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ common.testWidget( "dialog", {
resizable: true,
show: null,
title: null,
uiDialogTitleTagName: "<span>",
width: 300,

// Callbacks
Expand Down
1 change: 1 addition & 0 deletions tests/unit/dialog/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ common.testWidget( "dialog", {
resizable: true,
show: null,
title: null,
uiDialogTitleTagName: "<span>",
width: 300,

// Callbacks
Expand Down
10 changes: 10 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 @@ -117,6 +117,16 @@ QUnit.test( "aria-modal", function( assert ) {
element.remove();
} );

QUnit.test( "ui dialog title tagname", function( assert ) {
assert.expect( 1 );

var element, nodeName;

element = $( "<div>" ).dialog( { modal: true, uiDialogTitleTagName: "<h2>" } );
nodeName = element.dialog( "widget" ).find( ".ui-dialog-title" ).get( 0 ).nodeName.toLowerCase();
assert.equal( nodeName, "h2", "The dialog title element is h2" );
} );

QUnit.test( "widget method", function( assert ) {
assert.expect( 1 );
var dialog = $( "<div>" ).appendTo( "#qunit-fixture" ).dialog();
Expand Down
4 changes: 3 additions & 1 deletion ui/widgets/dialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ $.widget( "ui.dialog", {
resizable: true,
show: null,
title: null,
uiDialogTitleTagName: "<span>",
width: 300,

// Callbacks
Expand Down Expand Up @@ -437,7 +438,8 @@ $.widget( "ui.dialog", {
}
} );

uiDialogTitle = $( "<span>" ).uniqueId().prependTo( this.uiDialogTitlebar );
uiDialogTitle = $( this.options.uiDialogTitleTagName )
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@rpkoller rpkoller Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

.uniqueId().prependTo( this.uiDialogTitlebar );
this._addClass( uiDialogTitle, "ui-dialog-title" );
this._title( uiDialogTitle );

Expand Down