1052045 - <select required> constraint validation algorithm marks <select> as being missing when non-placeholder-label-option with an empty value attribute is selected

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Markus Doits]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:33.0) Gecko/20100101 Firefox/33.0 (Beta/Release)
Build ID: 20140811004001

Steps to reproduce:

According to the [spec](http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-select-element:attr-select-required-2) a required select element is only invalid as long as the placeholder label option is selected (first option with empty value).

So given this html (jsfiddle.net/nm3pteL3/4/):

<select required>
    <option value="">Placeholder</option>
    <option value="a">a</option>
    <option value="b">b</option>
    <option value="">c (empty)</option>
</select>

When selecting option c, the select should be valid.


Actual results:

Instead, it is marked as invalid.


Expected results:

Only the first option (placeholder) should make the select invalid.

I suspect it is tested whether the value is blank for the required check.

Webkit (tested Safari 8 and Chrome 38) and IE (tested version 11) do it right.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Only the first option with empty value is a Placeholder for <select>

Looks like we just have to check for the first option?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8477905 [details] [diff] [review]
Only the first option with empty value is a Placeholder for <select>

Then why do you need the loop?  Also, what about the optgroup check in the spec?

Comment 3

[Boris Zbarsky [:bzbarsky]]

I guess it's worth checking what other UAs do with optgroup, and adding a test for it.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

I totally missed the optgroup part! Sorry about that. Chrome & Opera follow the specs as written, an empty-valued option inside a optgroup is not a placeholder for the <select>.

Comment 5

[Alexander Farkas]

There is not only an optgroup part, but also a part about select with size higher than 1 and a select with multiple true.

Which means the following selects are all valid:

<select required size="2">
    <option value="">empty and first, but not placeholder option, because size > 2</option>
    <option></option>
</select>

<select required multiple>
    <option value="">empty and first, but not placeholder option because multiple</option>
    <option></option>
</select>

see also: https://bugzilla.mozilla.org/show_bug.cgi?id=621469

Comment 6

[Boris Zbarsky [:bzbarsky]]

> but also a part about select with size higher than 1 and a select with multiple true

Sure, because the whole thing in question only applies to comboboxes, not listboxes.  

I guess it's not clear that the current IsValueMissing() is only called for comboboxes.  If not, that needs to be fixed too.

Comment 7

[Alexander Farkas]

Not sure wether I understand you right (My english). But the required property and the valueMissing constrain does also apply to listboxes. The only difference is, that a multiple select can't have a placeholder option, but if selectedIndex is -1 this select can be invalid.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I think we're agreeing on what _should_ happen.

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1052045 - Fix <select> validity status for listboxes and for non-placeholder empty valued options

Thanks Boris & Alexander for your comments!

I checked and IsValueMissing is used for both comboboxes and listboxes, so we need to fix both. With this patch

* <select> with @multiple=true or @size != 0 are never suffering from missing value (except if they're empty)
* Only the first <option> is a candidate for being a placeholder
* <option> that are not direct child of the <select> element are not considered placeholder

bz, sorry about the sloppy patch, hope this is better :-)

Thanks!

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Fix <select> validity status for listboxes and for non-placeholder empty valued options

Sorry wrong patch.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8482008 [details] [diff] [review]
Fix <select> validity status for listboxes and for non-placeholder empty valued options

>+  if (Size() != 0 || Multiple()) {
>+    return false;
>+  }

No, this is wrong.  If size != 0 or multiple, then value is missing only if all options are unselected.

So really, you want to go through the options until you find the first one that is selected and then return false, but ignore a selected option that matches the "placeholder label option" definition.

Comment 12

[Tom Puttemans]

Attachment: Fix <select> validity status for listboxes and for non-placeholder empty valued options

This issue still persists today. I've forked the JSFiddle to add a form and a submit button (otherwise testing in Google Chrome didn't seem to trigger any UI) and added some extra cases for testing: http://jsfiddle.net/Glodenox/qjwuorb8/3/

I don't usually write C++, nor am I familiar with the Firefox codebase as this is my first ever submission. A proper code review will probably be necessary, though the changes are quite small. I mostly suspect that the verification of the parent node will require some extra checks.

I'm not sure who to ask to review this patch, so I just went with the first person in the 'suggested reviewers' list, sorry if that was wrong. This patch seems to put the behaviour in line with the other browsers, though I haven't verified whether Firefox now matches the rest of the spec concerning the multiline attribute.

Comment 13

[Tom Puttemans]

I'd like to add as well that Edge and Internet Explorer seems to mostly match the current behaviour of Firefox. Google Chrome and Opera seem to follow the spec (http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-select-element:attr-select-required-2)

If preferred, I'll look into adding some tests as well.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 8794978 [details] [diff] [review]
Fix <select> validity status for listboxes and for non-placeholder empty valued options

   for (uint32_t i = 0; i < length; ++i) {
     RefPtr<HTMLOptionElement> option = Item(i);
+    nsAutoString value;
+    MOZ_ALWAYS_SUCCEEDS(option->GetValue(value));
+    nsCOMPtr<nsINode> parent = option->GetParent();
+    // Check for valid placeholder, don't count it as a value
+    if (!hasPlaceholderLabel && !Multiple() && Size() <= 1 && value.IsEmpty()
+        && parent && parent->IsHTMLElement(nsGkAtoms::select)) {
+      hasPlaceholderLabel = true;
+      continue;
+    }

One could argue that we could save the GetValue() calls on a bunch of option elements if we only got the value inside an if (!hasPlaceholderLabel) check, but that's probably not a worthwhile optimization in general browsing. And I do believe the null check of the parent pointer can be removed here since all options inside of a select should by definition have a non-null parent.

r=jst with the null check removed, and yes, please add some tests for this change.

And thanks for the patch!

Comment 15

[Tom Puttemans]

Attachment: Fix <select> validity status for listboxes and for non-placeholder empty valued options ; r=jst

At first I was also intending to only call GetValue() when really needed, but it made the code harder to read, which made me draw the same conclusion.

I didn't remove the null check in the end as I encountered issues without it during testing. The tab kept crashing during Mochitest, which seems to trace back to the first select in the test not being properly closed with </select>. For some reason that seems to result in options that are accounted for as children, but their GetParent() returns null.
I couldn't find this behaviour when testing through the JavaScript API, so it seems to not cause issues there. I won't be able to investigate this further though. While it's not really valid HTML that is causing the issue, I don't think we want to allow tab crashes due to this.

I've now added several assertions that verify that the implementation matches the spec. It's not a complete test, but most cases should be covered. The Mochitest succeeded locally.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8795295 [details] [diff] [review]
Fix <select> validity status for listboxes and for non-placeholder empty valued options ; r=jst

Sorry to butt in here (I know, I have reviews turned off right now, sorry), but:

1) This patch doesn't implement the right definition of "placeholder label option".  It's not "the first option with an empty value".  It's "the first option, if that option has an empty value".  The tests should be modified so they would have caught this.  Though it's worth verifying that the spec matches the behavior of other browsers here.

2) The test should be a web platform test, not a mochitest.

I'm happy to review the patch here in general, for what it's worth.  Just needinfo me, please?

Comment 17

[Tom Puttemans]

Attachment: Fix <select> validity status for listboxes and for non-placeholder empty valued options ; r=jst

No problem, you were absolutely correct to butt in here. I did indeed misread what the spec said. I've now adjusted the implementation so it only looks at the first option and I've also taken the GetValue() call within this if now.

I've added a new file for the web platform test, which seems to run fine and checks most of the rules for the select element concerning validity. I haven't tested these tests on other browsers as I don't have the configuration for that set up, but apart from Internet Explorer and Edge, the other browsers seem to follow the spec on this (apart from setting the size of a select to 0 instead of 1 by default).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8795555 [details] [diff] [review]
Fix <select> validity status for listboxes and for non-placeholder empty valued options ; r=jst

Sorry for the lag here.  :( 

>+      nsCOMPtr<nsINode> parent = option->GetParent();

You don't need this, or the tag check.  Just do:

  if (i == 0 && !Multiple() && Size() <= 1 && option()->GetParent() == this)

and probably push the GetValue() call inside that condition too.

>+  assert_equals(select.value, "", "Even though it is an invalid option, the empty value should be set.");

The option isn't invalid.  The select is...  Maybe just "The placeholder's value should be the select's value right now"?

>+  select.multiple = true;

This part doesn't make sense, because size is still 2.  You should probably test the size > 1 and multiple cases separately...

r=me with those nits fixed.  Thank you for doing this!

Comment 19

[Tom Puttemans]

Attachment: Fix <select> validity status for listboxes and for non-placeholder empty valued options

No problem, I wouldn't have been able to fix it any sooner than this myself anyway as my weekend was packed :) Also, I'd rather have to wait a bit for a proper review than quickly getting a shallow review.

I think that those corrections have been applied now. Feel free to let me know if you see anything else.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8797251 [details] [diff] [review]
Fix <select> validity status for listboxes and for non-placeholder empty valued options

Excellent, thank you!

r=me

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5aeed15578
Fix <select> validity status for listboxes and for non-placeholder empty valued options. r=bz

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=37027904&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/a40e6f483853

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Also seeing similar failures like https://treeherder.mozilla.org/logviewer.html#?job_id=37028930&repo=mozilla-inbound possibly only on android.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Looks like we had tests depending on the buggy non-spec behavior.

Tom, are you willing to take a look at those, or do you want me to fix them up?  The relevant tests look like:

  dom/html/test/test_bug596511.html
  dom/html/test/test_bug605124-2.html 
  dom/html/test/test_bug605125-2.html 
  dom/html/test/test_bug612730.html
  dom/html/test/test_bug622597.html

Comment 25

[Tom Puttemans]

I don't mind fixing them myself. I've come this far, I might as well fix those tests too. The first test I've come across relied on the placeholder label check on the second element to invalidate the select, so I presume those other tests will be similar.

Comment 26

[Tom Puttemans]

Hmm, I've hit a bit of a wall. There still seems to be an issue with the check

  option->GetParent() == this

option->GetParent() seems to be returning a nullptr, even though it's obvious that parent should be set as we've just requested the 'items' of that parent. The return value remains nullptr until I explicitly add a value to the option or if I included a value in the constructor. What am I missing?

It seems like I can fix it with the following condition, but this is ugly and will probably result in nasty surprises later on:
  if (i == 0 && !Multiple() && Size() <= 1 && (
    option->GetParent() == nullptr || option->GetParent() == this)) {

The easiest way to trigger this issue is executing the following statements:
var select = document.createElement('select');
select.required = true;
select.add(new Option());
select.validity.valid; // This will return true, though it should be false

Comment 27

[Boris Zbarsky [:bzbarsky]]

Ugh.  The callstack to the last HTMLSelectElement::IsValueMissing call in the testcase from comment 26 looks like this:

#0  mozilla::dom::HTMLSelectElement::IsValueMissing (this=0x12c56f340) at HTMLSelectElement.cpp:1768
#1  0x000000010523a8b2 in mozilla::dom::HTMLSelectElement::UpdateValueMissingValidityState (this=0x12c56f340) at HTMLSelectElement.cpp:1802
#2  0x0000000105246631 in mozilla::dom::HTMLSelectElement::SelectSomething (this=0x12c56f340, aNotify=true) at HTMLSelectElement.cpp:1228
#3  0x00000001052442c4 in mozilla::dom::HTMLSelectElement::CheckSelectSomething (this=0x12c56f340, aNotify=true) at HTMLSelectElement.cpp:1204
#4  0x0000000105243fc8 in mozilla::dom::HTMLSelectElement::InsertOptionsIntoList (this=0x12c56f340, aOptions=0x136a38e80, aListIndex=0, aDepth=0, aNotify=true) at HTMLSelectElement.cpp:299
#5  0x0000000105242820 in mozilla::dom::HTMLSelectElement::WillAddOptions (this=0x12c56f340, aOptions=0x136a38e80, aParent=0x12c56f340, aContentIndex=0, aNotify=true) at HTMLSelectElement.cpp:426
#6  0x00000001052425cc in mozilla::dom::SafeOptionListMutation::SafeOptionListMutation (this=0x7fff5fbf7770, aSelect=0x12c56f340, aParent=0x12c56f340, aKid=0x136a38e80, aIndex=0, aNotify=true) at HTMLSelectElement.cpp:74
#7  0x0000000105238616 in mozilla::dom::SafeOptionListMutation::SafeOptionListMutation (this=0x7fff5fbf7770, aSelect=0x12c56f340, aParent=0x12c56f340, aKid=0x136a38e80, aIndex=0, aNotify=true) at HTMLSelectElement.cpp:60
#8  0x0000000105243b29 in mozilla::dom::HTMLSelectElement::InsertChildAt (this=0x12c56f340, aKid=0x136a38e80, aIndex=0, aNotify=true) at HTMLSelectElement.cpp:204
#9  0x0000000103b1a6b8 in nsINode::ReplaceOrInsertBefore (this=0x12c56f340, aReplace=false, aNewChild=0x136a38e80, aRefChild=0x0, aError=@0x7fff5fbf7e30) at nsINode.cpp:2504
#10 0x0000000103972dc1 in nsINode::InsertBefore (this=0x12c56f340, aNode=@0x136a38e80, aChild=0x0, aError=@0x7fff5fbf7e30) at nsINode.h:1848
#11 0x0000000103972e04 in nsINode::AppendChild (this=0x12c56f340, aNode=@0x136a38e80, aError=@0x7fff5fbf7e30) at nsINode.h:1852
#12 0x0000000105244b95 in mozilla::dom::HTMLSelectElement::Add (this=0x12c56f340, aElement=@0x136a38e80, aBefore=0x0, aError=@0x7fff5fbf7e30) at HTMLSelectElement.cpp:574

Note that this is happening before HTMLSelectElement::InsertChildAt has called InsertChildAt on its parent class, so in particular before we've actually inserted the <option> into the DOM!

I think the simplest thing to do here is to have SafeOptionListMutation grab the currently selected index in the constructor if aSelect is not null, and then in the destructor if the selected index has changed go ahead and call UpdateValueMissingValidityState() on the select, followed by UpdateState(), with the boolean for the latter being the aNotify that got passed to the constructor.  That should handle cases in which insertion of options might have triggered a validity change, which can only happen when nothing was selected before the insert and the insert therefore triggered the SelectSomething() codepath.

Comment 28

[Tom Puttemans]

Attachment: Fix <select> validity status for listboxes and for non-placeholder empty valued options

Sorry for the long radio silence. I've been a bit busy lately.
As I don't feel comfortable enough in C++ and am not familiar enough with these components, I'm going to give up on the 'not a placeholder when child of an optgroup' clause. Either that check becomes a separate bug or it gets added to this patch by someone else, either is fine by me. Personally, I think this will already cover the grand majority of issues that people may have encountered.

So the attached patch does all checks except the optgroup one. The failing tests have been adjusted so they follow spec concerning placeholder options. All tests in dom/html/test/ seem to work now, I hope I didn't miss anything.

I also encountered an issue with the Option constructor setting the selectedness to true when it shouldn't, but that's already being handled in Bug 1300282. The failing checks due to this have been set as todo now.

Comment 29

[Boris Zbarsky [:bzbarsky]]

No worries about radio silence.  And my apologies for the 4-day lag; I blame the turkeys.  ;)

Looking at the patch:

>+++ b/dom/html/test/test_bug622597.html
>-      select.selectedIndex = 1;

I think it would more closely preserve the intent of the test if you changed the second option to be selected initially and set selectedIndex to 0 here, right?

I will take a look at the optgroup bits.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Also, it looks like the wpt test removed a few of the 'placeholder.value = ""' bits in the latest patch version.  Is that just because that's the default value anyway so they were superfluous?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Looks like there are still two test failures: 

REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-valid/select/select-required-multiple-invalid.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-valid/select/select-required-multiple-ref.html | image comparison, max difference: 255, number of differing pixels: 2007
REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-invalid/select/select-required-multiple-invalid.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-invalid/select/select-required-multiple-ref.html | image comparison, max difference: 255, number of differing pixels: 2007

and indeed, with the changes that <select> is valid.  I'll just update that test before posting the final patch.

Comment 32

[Tom Puttemans]

About test_bug622597.html: indeed, that would be better. With my patch the .focus() call won't have much of an effect any more, sort of defeating the purpose of that test.

Indeed, the 'placeholder.value = ""' assignments were completely unnecessary and as such I've removed them.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Tom's patch with the change from comment 29 applied and the reftests fixed

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix the optgroup thing; needed for the web platform test to pass

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: Combined patch

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5644a5462ce5
Fix <select> validity status for listboxes and for non-placeholder empty valued options.  r=bzbarsky

Comment 37

[Boris Zbarsky [:bzbarsky]]

Tom, thank you for sticking with this and fixing this bug!

Comment 38

[Tom Puttemans]

Thank you for guiding me through this Boris!
It's been an interesting and educational experience. If something else starts bugging me, I'll probably try to get it patched as well :)

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5644a5462ce5