Skip to content

Selectmenu: Fix selecting options following hidden ones #2144

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 1 commit into from
May 10, 2023
Merged

Selectmenu: Fix selecting options following hidden ones #2144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
65 changes: 62 additions & 3 deletions tests/unit/selectmenu/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,16 +394,75 @@ QUnit.test( "Options with hidden attribute should not be rendered", function( as
setTimeout( function() {
button.trigger( "click" );
options = menu.children()
.map( function() {
return $( this ).text();
.get()
.filter( function( item ) {
return $( item ).is( ":visible" );
Comment on lines +398 to +399
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to change the test as the options are now back in the DOM, just hidden.

} )
.get();
.map( function( item ) {
return $( item ).text();
} );
assert.deepEqual( options, [ "Slower", "Medium", "Fast", "Faster" ], "correct elements" );

ready();
} );
} );

QUnit.test( "Options with hidden attribute should not break the widget (gh-2082)",
function( assert ) {
var ready = assert.async();
assert.expect( 1 );

var button;
var element = $( "#speed" );

element.find( "option" ).slice( 0, 2 ).prop( "hidden", true );
element.val( "Faster" );
element.selectmenu();

button = element.selectmenu( "widget" );
button.simulate( "focus" );
setTimeout( function() {
try {
button.trigger( "click" );
assert.strictEqual( button.text(), "Faster", "Selected value is correct" );
} catch ( e ) {
assert.ok( false, "Clicking on the select box crashed" );
}

ready();
} );
} );

QUnit.test( "Optgroups with hidden attribute should not break the widget (gh-2082)",
function( assert ) {
var ready = assert.async();
assert.expect( 1 );

var button;
var element = $( "#files" );

element.find( "optgroup" ).first().prop( "hidden", true );
element
.find( "optgroup" ).eq( 1 )
.find( "option" ).first()
.prop( "hidden", true );
element.val( "someotherfile" );
element.selectmenu();

button = element.selectmenu( "widget" );
button.simulate( "focus" );
setTimeout( function() {
try {
button.trigger( "click" );
assert.strictEqual( button.text(), "Some other file", "Selected option is correct" );
} catch ( e ) {
assert.ok( false, "Clicking on the select box crashed" );
}

ready();
} );
} );

QUnit.test( "extra listeners created after selection (trac-15078, trac-15152)", function( assert ) {
assert.expect( 3 );

Expand Down
12 changes: 7 additions & 5 deletions ui/widgets/selectmenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,12 @@ return $.widget( "ui.selectmenu", [ $.ui.formResetMixin, {
if ( item.disabled ) {
this._addClass( li, null, "ui-state-disabled" );
}
this._setText( wrapper, item.label );

if ( item.hidden ) {
li.prop( "hidden", true );
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about adding hidden prop here. We don't do that anywhere else. But it seems there is no other usage of the hidden attribute, so there is nothing to compare it to directly.

What about using "aria-hidden" attribute and "ui-helper-hidden" CSS class?

Copy link
Member Author

Choose a reason for hiding this comment

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

@fnagel Sure, I can change the code. The behavior in IE 9 & 10 will be different but perhaps if the hidden attribute is there, it's expected that we should hide it even if the browser doesn't support it natively.

One thing - I see the ui-helper-hidden class definition in CSS but other than that, I don't see it anywhere in code; only the ui-helper-hidden-accessible one seems to be used. Am I missing anything?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about my proposed change either :-D Just wanted to point out our options and get your opinion on it.

You are right, ui-helper-hidden is not used in JS. And ui-helper-hidden-accessible seems not correct here.

Maybe its best to use it as is it is (with the hidden property) and let the client (or its assistive technology) decide what to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case, I think I'd leave it at hidden. @fnagel do you have other concerns? If not, could you approve?

} else {
this._setText( wrapper, item.label );
}

return li.append( wrapper ).appendTo( ul );
},
Expand Down Expand Up @@ -658,10 +663,6 @@ return $.widget( "ui.selectmenu", [ $.ui.formResetMixin, {
var that = this,
data = [];
options.each( function( index, item ) {
if ( item.hidden ) {
return;
}

data.push( that._parseOption( $( item ), index ) );
} );
this.items = data;
Expand All @@ -675,6 +676,7 @@ return $.widget( "ui.selectmenu", [ $.ui.formResetMixin, {
index: index,
value: option.val(),
label: option.text(),
hidden: optgroup.prop( "hidden" ) || option.prop( "hidden" ),
optgroup: optgroup.attr( "label" ) || "",
disabled: optgroup.prop( "disabled" ) || option.prop( "disabled" )
};
Expand Down