Selectmenu: Introduce _renderButton method

[fnagel]

Fixes #10142

Add extension point for modifying the button element (aka the selected item).

[jzaefferer]

Let's add a test for this. Otherwise looks good.

[fnagel]

Ok, but what should I test here?

[scottgonzalez]

None of this can be in here. These are all required functionality.

[scottgonzalez]

This needs to wait until 1.12.0 since it's adding a new API.

[scottgonzalez]

This implementation is actually quite wrong. The extension point is supposed to be an equivalent of _renderItem(), not a method for generating a long-lived wrapper.

[fnagel]

Looks good vs. five comments ;-)

@scottgonzalez Ok, but how should it look like? Just return an plain jQuerified span? Adding to dom and adding required attributes later on?

[fnagel]

I'm not thrilled about the fact that an extension point needs to concern itself with other built-in options, but I don't see any way around this.

Or is this already the answer to my question? So just extract the adding to dom from the current version?

[scottgonzalez]

Essentially, you should've replaced _setText(). This is not at all helping the user accomplish what they want. Please read the ticket again.

[scottgonzalez]

I'm going to close this for now since it doesn't actually address the request in the ticket.

[fnagel]

The actual request in the ticket (changing color) can already be done by using callback events as I outlined in my fiddle in the ticket. I think the outlined way to go is the best way to do so.

In my opinion the ticket talks about two things:

1. Change the generated markup of the button (which is rendered just once on create)
2. Change the markup when an item is selected

We don't want to re-render / replace the button markup on each item select as that would break a11y handling. And we have multiple attributes required for a11y which is tricky when we allow to customize the markup of the button.

Proposal: Add a _renderButton method which returns a span already including all attributes. Within this method we create the buttonText span and the icon span, too. Only appending to the DOM is done in _drawButton. In addition we add a method (like _changeButton) which by default just calls _setText on the buttonText span.

[scottgonzalez]

Change the generated markup of the button (which is rendered just once on create)

I have no idea where you're getting this from. The request is just about the rendering of the item itself.

[fnagel]

The request is just about the rendering of the item itself.

The item itself (the button) is only rendered once on create. The given use case (changing color) is not achievable by just modifying the rendering on create.

In the original plugin the modified markup of list items applied to the selected item as well. [...] It seems custom markup is stripped or not used for the default/selected option. Ideally the same custom markup would be used in both places.

This is where I get this from. The 'original' selectmenu copied the classes and the content of the selected option to the button element. It's a misunderstanding of how selectmenu widget works.

Anyway: adding a _renderButton or better _changeButton method as I outlined above seems helpful for the color change use case and especially when considering the use case of http://bugs.jqueryui.com/ticket/10189

[scottgonzalez]

The item itself (the button) is only rendered once on create.

The item is not the button. The button contains the item.

The 'original' selectmenu copied the classes and the content of the selected option to the button element.

That can be accomplished without touching the actual button, just it's content.

[fnagel]

Ok, perhaps we can talk better about what we have in mind by taking a look this implementation: fnagel@bf0b549

I think it's very flexible but provides us a little more safety regarding needed attributes / a11y settings.

[scottgonzalez]

I have no desire to expose a method for rendering the actual button when it requires setting and appending an internal property. Similarly, _updateButton() (which should probably be called _renderButtonItem()), shouldn't require use of an internal property either. Please look at how _renderItem() works. There should be absolutely no reliance on internal properties.

[fnagel]

And another try: fnagel@a65a6d6

If we want to pass the item object to the _renderButtonItem method it's needed to parse the option. We could easily change this to use the option itself (and have a quite smaller changeset) but using the item object seems more consistent to me.

[fnagel]

Sorry, posted wrong link. Updated comment.

[scottgonzalez]

That looks good. Can you add a test that overrides the method on a widget instance?

[fnagel]

Yes. I will open a new pull request.

[fnagel]

Replaced with #1299