Problem/Motivation
Multiple-value select lists include a convenience '- None -' option but it is not selected by default. This leaves the end-user confused as to whether or not to they should select the '- None -' option, or instead not select any option.
Updated Steps to reproduce:
- Install Base Drupal 8
- Click Menu -> Structure -> Taxonomy
- Click Add Vocabulary and add a sample vocabulary like "Automobiles"
- Add Terms like " 1) Cars, 2) Bikes, 3) Trucks"
- With this the taxonomy is created.
- Go back to Menu-> Structure -> Content Types
- You should see Article and Basic Page listed here. Click the dropdown near to edit on "Article" row and select Manage Fields
- You will see list of fileds in the contet type like title,body etc
- In the bottom, there is a text filed labeled "Add a new Field". Type a Label - "Atuomobile" here
- On the select box in the same row, click the dropdown "Select a field type" and select term reference
- The widget column "widget" should be select list. Hit Save
- In the next page, change "Maximum number of values users can enter" to "Unlimited" and hit save
- Yet another page opens with the Field label and a text area and at the bottom , the option to select "Default Value" for the field. Select "None" and Click "Save Settings"
- You are all set. Close the light box by clicking the "X" at the top right. Let us test the feature
- Click Menu -> Content
- Click the Blue "Add Content" Button and select Article
- Give a title and skip the rest and look at the Taxonomy field that you added earlier near the bottom of the page
- Your selection "None" should be highlighted here
- With the basic Drupal 8 install, None is not highlighted for me
Proposed resolution
Modify either FormAPI or the field module in order to have the '- None -' option selected by default only if there are no saved or default value(s) for the select list.
Remaining tasks
Reviews needed, tests to be written
User interface changes
On multiple-value select lists, the '- None -' option will be selected by default.
Original report by @mbutcher
When adding a select widget for a taxonomy term to my field, I cannot set the default field to "-None-".
Steps to reproduce:
- Add a taxonomy term field to a node type.
- Set the widget type to "Select list"
- On the settings for the widget, set the "Default value" to "-none-"
Save
Outcome:
"-None-" is no longer highlighted, nor is it marked as the default on an article edit form.
Expected:
In both forms I expect "-none-" to be marked as the default.
This has been a source of confusion to our users, who don't know if they need to select "-none-" or leave it unselected.
https://img.skitch.com/20111221-dd52pfaa9aaxpcnxdih34kp939.png
Comment | File | Size | Author |
---|---|---|---|
#18 | single_select.JPG | 21.33 KB | kbk |
#55 | interdiff.txt | 757 bytes | dcam |
#55 | make-none-selected-1379070-55.patch | 3.26 KB | dcam |
Comments
Comment #1
joachim CreditAttribution: joachim commentedI can't reproduce this. Have you any other settings or enabled modules that may be relevant?
Comment #2
yched CreditAttribution: yched commentedRe-categorizing
Comment #3
xjmI was also unable to reproduce this in D8. Could you give exact steps to reproduce, like maybe the particular vocabulary and terms? Or even a test case that replicates your config?
Maybe related: #1381140: A #default_value = 0 for #type radios checks all radios.
Comment #4
xjmOh! I figured out how to reproduce it. It needs to be a multiple value field, as in the screenshot
Comment #5
xjmAnd actually this appears to be a field/forms issue, because it also happens with a mutlivalue text list field.
Comment #6
yched CreditAttribution: yched commentedHm, true, that's actually not allowed by Field API.
'- None -' comes back as "no value", and "no value" means we don't store any "default value specified" (just like, if the widget was a textbox, we would treat 'empty string' as 'no default value was specified'), and therefore nothing gets preselected on entity creation forms.
On non-multiple selects, the '- None -' option does come preselected, but that's only the usual behavior of select dropdowns, the 1st value shows as selected if no explicit value is set.
I don't think I see a workaround. I'd tend to mark this "by design".
Comment #7
xjmYeah, the "None" option is really just a convenient way to deselect other options you may have selected. On the other hand, I do see the UX issue with it not being selected by default.
Comment #8
Bojhan CreditAttribution: Bojhan commentedI see no reason, why we wouldn't allow this to be a default value - I imagine there be challenges from a back-end perspective, but even if its only on display, it should solve the issue.
Comment #9
xjmMoving back to field land.
Comment #10
xjmComment #11
xjmLet's add an automated test for this to get this issue rolling. Also let's add a full issue summary including the info from #4 and #5.
Comment #12
joachim CreditAttribution: joachim commented> On non-multiple selects, the '- None -' option does come preselected, but that's only the usual behavior of select dropdowns, the 1st value shows as selected if no explicit value is set.
If that's the case on single-value selects, surely the fix for this belongs in FormAPI land, and is to make the convenience 'None' option preselected on multi-value selects.
Otherwise, if we only fix this in FieldAPI, we'll have a UI inconsistency.
Comment #13
gaas CreditAttribution: gaas commentedThis is my attempt at a patch for this. Unfortunately I still get a test failure for OptionsWidgetsTest::testRadioButtons that I don't really understand.
For non-required single-value fields there will be radiobutton with a "N/A" entry that should be selected. The patch fixes that case as well.
Comment #14
gaas CreditAttribution: gaas commentedComment #16
kid_icarus CreditAttribution: kid_icarus commentedHere is my attempt to address the issue in FormAPI land.
The test provided is similar to @Gisle's, only that it asserts '_none' is selected in the multi-value test instead of the single value test.
I'm uncertain as to whether my version of the fix is valid, and furthermore, if the provided assertion would be better suited to live in another test (specifically a form test, rather than a field test).
Feedback much appreciated :)
Comment #17.0
kid_icarus CreditAttribution: kid_icarus commentedAdded Issue Summary
Comment #17.1
kid_icarus CreditAttribution: kid_icarus commentedUpdated issue summary.
Comment #18
kbk CreditAttribution: kbk commentedCould this problem apply to single-select term reference fields as well? When I select "N/A" as the Default Value > Save > revisit settings page > the Default Value is not set. If I choose an actual term, the Default Value is retained.
Comment #19
YesCT CreditAttribution: YesCT commented#16: make_none_selected-1379070-16-complete.patch queued for re-testing.
Comment #21
YesCT CreditAttribution: YesCT commentedtagging for reroll
Comment #22
rkjha CreditAttribution: rkjha commentedPatch rerolled
Comment #23
rkjha CreditAttribution: rkjha commentedComment #25
rkjha CreditAttribution: rkjha commentedI will have a look at what went wrong and will get the patch updated as soon as I get some time.
Comment #26
rkjha CreditAttribution: rkjha commentedPatch rerolled and updated. Let us hope it passes this time.
Comment #27
star-szr@rkjha - Thanks! An interdiff would be very helpful here :)
Comment #29
disasm CreditAttribution: disasm commentedThanks rkjha for the reroll. I was reviewing this, and a few things to note (completely unrelated to your excellent work rkjha):
1) The interface for this is not very good in my opinion. having a "None" option on a multi-select list is confusing. What if I select none and another tag? Will it just select the other tag, will the none override the other tag, will it be in some kind of pseudo state where it's both selected and unselected? In my opinion, the none makes perfect sense for the dropdown select, but for the multi-select, it should just have a list of tags, and then if none are selected, it marks it as having none selected.
2) The STR needs updated. An STR needs to be detailed enough that someone who has never worked on an issue can go through from start to finish. The first step should always be install D8, and we need steps for adding terms to the vocabulary, adding the field to the content type, creating new content page, etc...
Again, thanks for your hard work rkjha!
Comment #29.0
disasm CreditAttribution: disasm commentedUpdated issue summary.
Comment #30
zetagraph CreditAttribution: zetagraph commentedTried the STR on clean install and applied the patch in 26 manually. Seems to work for me and fixes the behaviour where the '- None -' option is now stays selected in the content type field settings and the node.
Comment #31
rkjha CreditAttribution: rkjha commentedWhat you have pointed out is great, disasm, but there is one issue that comes in my mind(which I feel remains unaddressed).
Suppose, for multi-select, I have a list of tags with none of them selected in the beginning(as you've proposed). I select some tags but soon realize that I didn't intent to select any. In this case, I have only two option. Either there is some "none" option, selecting which undoes the selection of rest all tags or there is some feature that one can deselect the tag by clicking it again.
Comment #32
attiks CreditAttribution: attiks commented#31 if you CTRL click on a selected option it will get deselected, works in most browser AFAIK. Only problem might be with mobile.
Comment #32.0
shariharan CreditAttribution: shariharan commentedAdded couple steps to the STR for creating test vocabulary and terms.
Comment #32.1
shariharan CreditAttribution: shariharan commentedUpdated Steps to Reproduce
Comment #33
shariharan CreditAttribution: shariharan commentedUpdated Steps to reproduce:
updated the issue summary
Comment #33.0
shariharan CreditAttribution: shariharan commentedmoved the STR to the top
Comment #34
kshama_deshmukh CreditAttribution: kshama_deshmukh commentedComment #35
yched CreditAttribution: yched commentedAs stated in #6, and as the patches show, this is not related to Field API & the "select" widgets, that's how Form API's underlying 'select' #type works.
Re-categorizing.
Comment #35.0
yched CreditAttribution: yched commentedAdding back the original STR which I removed accidentally
Comment #36
YesCT CreditAttribution: YesCT commentedunassigning. it has been a while. please post a comment back to pick this back up.
I think a little more hints about what tests are still needed would help.
Comment #37
YesCT CreditAttribution: YesCT commentedneeds reroll, instructions: https://drupal.org/contributor-tasks/reroll
(changed the previous sprint tag, because it had a # in it, which is trouble after the d.o d7 upgrade, #2171455: d.o does not autocomplete tags starting with # hash anymore)
Comment #38
akozma CreditAttribution: akozma commentedComment #39
akozma CreditAttribution: akozma commentedRe-rolling the patch from #26.
Comment #42
swentel CreditAttribution: swentel commentedThis will make it work - but I had to cheat a little in the optionsWidgetBase class. not sure yet whether that's the best approach, but tests should be green now.
Comment #43
akozma CreditAttribution: akozma commented@swentel, I don't know neither which is the best approach.
I made also a fix on patch #39, which pass the test. Maybe it will be useful.
Comment #44
swentel CreditAttribution: swentel commentedThat's indeed much saner :)
Comment #47
akozma CreditAttribution: akozma commentedThe patch failed on Drupal\taxonomy\Tests\TermFieldMultipleVocabularyTest, but as I see the patch is correct.
I checked the TermFieldMultipleVocabularyTest::testTaxonomyTermFieldMultipleVocabularies() function and it seems the call for testing the widget display should be changed.
Before:
$this->assertFieldByName("{$this->field_name}[]", '', 'Widget is still displayed.');
- this is searching for the select field with the value '', but we will never have that in this case.
After:
$this->assertFieldByName("{$this->field_name}[]", NULL, 'Widget is still displayed.');
- at this call we are interested to check if the widget displayed only, so NULL as value will ignore the checking for value also.
The value check are done in WebTestBase::assertFieldByXPath(), "if (isset($value)) {...}" , and because the isset on empty string is TRUE than the assertFieldByName actually search for the select with value ''.
Comment #48
miniwebs2 CreditAttribution: miniwebs2 commentedWorking at a code sprint at DrupalSouth in Wellington - this was my first ever test of a patch (mentor beside me!!!!), we could confirm that the problem still exists in current Drupal core still exists.
Applied the patch (mentor still beside me!!!!) and we now confirm that the above patch at #47 does work and therefore makes the Default setting on 'None' stick.
Rechecked it on local dev and all good.
Seems RTBC (that's what the mentor said I write!!!)
Comment #49
catchCommitted/pushed to 8.x, thanks!
Moving to 7.x for backport.
Comment #50
klonos...we should really get used to hiding previous, obsolete patches/files when uploading new ones ;)
Comment #51
Sutharsan CreditAttribution: Sutharsan commented... and to removing 'Needs reroll' tags when done ;)
Comment #52
akozma CreditAttribution: akozma commentedComment #53
dcam CreditAttribution: dcam commentedBackported #47 to D7.
Comment #55
dcam CreditAttribution: dcam commentedOops. I didn't change the field ID in that line.
Comment #56
mgiffordI went through the steps to reproduce and it looks good. This will be a good improvement to D7.
You can also verify that None doesn't work in D7 Core now by just looking at the Content types as the value doesn't save right now - admin/structure/types/manage/article/fields/field_atuomobile
Comment #57
mgiffordWhoops, wanted to RTBC IT.
Comment #60
dcam CreditAttribution: dcam commentedRandom test failures...
Comment #64
deanflory CreditAttribution: deanflory commentedShould this be applied to D7.31 or is does it still have issues? The testing fails are confusing (not really, but the retesting is).
Comment #66
dcam CreditAttribution: dcam commentedThose test failures were the result of a Test server issue. It was mistakenly configured to run with the wrong version of PHP. The issue was fixed. I thought I retested all of the affected issues, but I missed this one, probably because its status wasn't set to Needs Work.
I should note that Testbot auto-retests issues now and for 7.x will occasionally crash. This results in a test failure. Again, there is no issue with the patch. It just has to be retested and set back to RTBC.
Comment #70
David_Rothstein CreditAttribution: David_Rothstein commentedSorry, but this fix can't be right... The '_none' key only has special meaning with the Options module, so the form API shouldn't have any code that treats it specially.
At least in Drupal 7, isn't the actual bug in the Options module? I would expect it has something to do with this code...
Comment #75
sassafrass CreditAttribution: sassafrass as a volunteer commentedThis issue still exists in Drupal 7. Do we need to create a new ticket to address it in D7?
Comment #76
mgifford@David_Rothstein Didn't this get fixed in D8 in #49?
I guess that "_none" is in this patch https://www.drupal.org/files/issues/make_none_selected-1379070-47-comple...
So if not "_none" what?
@sassafrass there is still a patch for D7 that might work. It still has this same issue with "_none".
Comment #79
praveen kumar R CreditAttribution: praveen kumar R at Drupal Partners commentedI followed the above steps to reproduce the issue and it works fine with #47 patch file.
Comment #81
cilefen CreditAttribution: cilefen commented@praveen kumar R
I do not understand what you reviewed. #47 is a 3-year-old patch that was committed.
@sassafrass A maintainer raised an issue in #70 about _none that hasn't yet been settled.
Comment #83
AndrewTur CreditAttribution: AndrewTur commentedHi,
I followed the steps in the summary and the issue seems fixed in 8.4.x.
But I'm a bit confused to what the comment #70 is about. What needs to be done for this issue to get resolved it would be helpful to update the summary so this can get moved along.
Comment #87
shobhit_juyal CreditAttribution: shobhit_juyal as a volunteer and at Srijan | A Material+ Company commentedMade some correction in the issue description.
Comment #89
quietone CreditAttribution: quietone as a volunteer commentedThis issue was committed back in Feb 2014 when Drupal 8 was in alpha. There is only one question here that needs to be answered.
Looking at form.inc the change is in 'form_select_options' which is for converting the options in a select element. That looks like the right place to make this change. So, move this to Drupal 7?
Comment #90
quietone CreditAttribution: quietone as a volunteer commentedAsked in #bugsmash and lendude agreed that this should be moved to Drupal 7.