Selectmenu: Prevent exception when the user types in selectmenu, when the result is an optgroup

[SimenB]

Hi!

This PR is probably done somewhat wrong, but I thought I should get this in before the weekend.

Ticket: http://bugs.jqueryui.com/ticket/10571

If you have a Selectmenu with optgroups, and type while it has focus, it may hit an optgroup as a result. If this happens, you get an exception.

Fiddle: http://jsfiddle.net/coLcs5pz/1/

Just put focus on the selectmenu, and type S, and you'll get an exception in the console.
Uncaught TypeError: Cannot read property 'index' of undefined

There is probably some better way to solve this problem, but this fixes it for us at least.

[SimenB]

Build failure... The proxy at work prevents me from actually cloning from GitHub, but if you haven't done anything with this until tomorrow (CET), I'll take a look at it.

EDIT: Only JSHint error. I'll squash commits tomorrow (I'm doing these commits through GH's website).

I can add a test as well

[scottgonzalez]

Thanks, please start by filing a bug report. We'll also need you to sign our CLA. You should read through the contributing guidelines as it documents how to properly contribute.

[SimenB]

Filed CLA and bug

Anything else I'm missing? Other than the fact I should probably write a unit test for this.

[scottgonzalez]

That's it. Though now that I'm looking at your code, I don't think this is the right fix. The underlying problem is that optgroups shouldn't be able to get focus in the first place.

[SimenB]

So I should look in menu.js instead, and do some filtering? I've only really started looking at the internals of jQuery UI after you went AMD, so I'm not familiar at all with the codebase

[scottgonzalez]

Yeah, menu's key handling should ignore dividers.

[SimenB]

Ping @scottgonzalez I tried changing code in menu now, is that closer to a good solution?

I messed up when trying to clean up formatting (editing files and keeping the styling when copying from IDEA into GH's editor isn't easy!). I'll squash away the commits tomorrow

[scottgonzalez]

Couldn't you just change this line to use ".ui-menu-item" instead of this.options.items?

[scottgonzalez]

Hmm...that change alone probably isn't good, since it will traverse into submenus. I don't think we've spent any time thinking about how this should behave with submenus.

[scottgonzalez]

Probably the easiest solution to this while ignoring the submenu question is to just add another filter:

match = this.activeMenu
    .find( this.options.items )

    // Only match on items, not dividers or other content (#10571)
    .filter( ".ui-menu-item" )
    .filter(function() {
        ...

[SimenB]

Ping @scottgonzalez OK, I've added your solution now, and wrote a test for it.

The test is in selectmenu_events, while it should probably be in menu_events, but I couldn't figure out how to properly test it there. And I suppose a test for exactly what triggered the error isn't wrong anyway.
Any ideas on adding a test to menu_events as well?

[SimenB]

The same search is performed right under if there's no match.
https://github.com/jquery/jquery-ui/pull/1329/files#diff-d338fe82d253858f3e1ce2f17e82f104R261

Should the menu-item filter be applied there as well?

Another question: would it make sense to cache the menu-items like this.options.items (this.options.menutimes?) is now, to avoid having to filter through everything every time? IDK, just throwing out ideas.

[scottgonzalez]

We should probably pull the logic out into a method and call it in both locations to avoid the duplication. Caching the items would require tracking them on a per menu basis (note that this isn't a per instance basis because menus nest automatically).

[scottgonzalez]

What was making it tricky to test this directly on menu? Isn't it pretty much the exact same test?

[SimenB]

I moved the initial filtering (excluding the regex-check) into it's own variable, does that work?

My problem writing a test in menu_events is that in a normal menu, I can't seem to figure out what is not considered a menu-item. Both class="ui-state-disabled" and class="ui-menu-group" items get the ui-menu-item-class, so I suppose they're not supposed to get filtered out.

[scottgonzalez]

I moved the initial filtering (excluding the regex-check) into it's own variable, does that work?

The logic is still exactly the same, why not just move everything into a function and pass the regex?

My problem writing a test in menu_events is that in a normal menu, I can't seem to figure out what is not considered a menu-item.

Anything that doesn't match the items option.

[SimenB]

Something like how it is now?

Anything that doesn't match the items option.

But that matches all the children of the menu, right ("> *")? I worked around it by making it a divider, but it seems kind of hackish.

Should I remove the test from Selectmenu? I feel like the test in there is more of a real use-case which kind of justifies it's existence.

Thank you for your patience with me, btw. I bet you could have fixed it yourself quicker, and with less effort, than this.

[SimenB]

Ping @scottgonzalez Is there anything else I can do with this?

[scottgonzalez]

Sorry for the delay, I was traveling last week. I'll likely get back to this tomorrow.

[scottgonzalez]

This should verify that the correct element is provided, not just that it's a menu item.

[scottgonzalez]

We're very close to being able to land this :-) Let me know if you need any help with the test update.

[SimenB]

Ok, that last push should cover your comments.

[scottgonzalez]

Thanks. Just landed this in master :-)