Allow null values to be treated as deselect options.

[awaters-escribe]

When a renderer doesn't render null as an empty string "" the setting allowSingleDeselect doesn't take effect. Therefore, when ChosenValueListBox encounters a null value it passes an empty string as the list box item's value. ChosenImpl now looks at the element's value attribute to detect this, while keeping the old functionality operational.

[olafleur]

It seems we have a failing test in master. I'll check that.

[olafleur]

The test that is failing is tabNavigation in ChosenIT. I think it's because you are changing the behaviour of ChosenValueListBox.

Could you fix the failing test or make sure that it still passes (depending on what is relevant)?

[awaters-escribe]

On my local machine the test fails from master, I'm not sure if it is a version issue with Chrome/ChromeDriver, can you force a rebuild of master in team city to rule that out?

[olafleur]

Still failing after forcing the rebuild.

[meriouma]

What's the use case? I'm not sure I understand why you cannot use the current placeholder functionality? You want to allow "" value to have the same meaning as null for the placeholder?

[awaters-escribe]

master fails on the same test http://teamcity.arcbees.com/viewLog.html?buildId=6239&tab=buildResultsDiv&buildTypeId=GwtChosen_MonitorPulls.

The issue is that in order for allowSingleDeselect to actually be active the first item in the needs to have "" as its text, this is so there is an item to deselect to. When using ChosenValueListBox that means he renderer needs to render the first value as "", usually this is accomplished by inserting a null into the list and having the renderer render null as "". However, in the application I am developing the renderer renders null as "Select One". This breaks allowSingleDeselect because the text is no longer "", but is still the item to deselect to. This change makes it so that when a null value is passed into ChosenValueListBox it changes the value to "", but leaves the text the same as the render. Then I changed ChosenImpl to look at getValue(). Since getText() == getValue() when using the single argument addItem() this change does not break the old way that it worked.

[meriouma]

It seems you are doing this pull request to bypass how the placeholder is handled. You are using the renderer to render a value as the placeholder, but in reality, you should use the setPlaceholderText on your ChosenOptions. @jDramaix Thoughts?

[awaters-escribe]

This is separate from a placeholder. With ChosenValueListBox one must add a null value to acceptable values in order to have something to deselect to. Therefore the first item in acceptableValues is null. Now, before my change, ChosenImpl would only select the null value if it rendered as "" (an empty string). In my application null is rendered as "Select One", while the merits of rendering null as "" or "Select One" can be debated, I won't touch on that subject. This change makes it so a null value has its form value as "", and ChosenImpl is updated to check the form value instead of the text of the rendered item.

See https://github.com/ArcBees/gwtchosen/blob/d9eec8188c044dedfda7b0d15b12f4b1b41e7499/plugin/src/main/java/com/arcbees/chosen/client/gwt/BaseChosenValueListBox.java#L153 for more details about null being in the list.

[awaters-escribe]

Can I get some feedback on this? It is documented and fully tested, what else are you guys looking for?

[christiangoudreau]

@jDramaix to review

[jDramaix]

seems pretty straightforward. LGTM

[olafleur]

@awaters-escribe could you fix the conflicts? After that we could merge that PR as we have an LGTM from @jDramaix

[ewolk]

@jDramaix @olafleur merge conflicts have been fixed, and pr rebased on the latest.

the tests that are failing are: DesktopChosenIT.allowSingleDeselect MobileChosenIT.allowSingleDeselect it is unclear from here whether these tests should be failing or not.