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

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: Markus Doits, Assigned: Tom Puttemans)

Tracking

({reproducible, testcase})

33 Branch
mozilla53
reproducible, testcase
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

Attachments

(1 attachment, 10 obsolete attachments)

23.92 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Core & HTML
Ever confirmed: true
Keywords: reproducible, testcase
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
Summary: select required not following html standard exactly → <select required> constraint validation algorithm marks <select> as being missing when non-placeholder-label-option with an empty value attribute is selected
Created attachment 8477905 [details] [diff] [review]
Only the first option with empty value is a Placeholder for <select>

Looks like we just have to check for the first option?
Attachment #8477905 - Flags: review?(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?
Attachment #8477905 - Flags: review?(bzbarsky) → review-
I guess it's worth checking what other UAs do with optgroup, and adding a test for it.
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

3 years ago
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
> 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

3 years ago
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.
I think we're agreeing on what _should_ happen.
Created attachment 8482006 [details] [diff] [review]
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!
Attachment #8477905 - Attachment is obsolete: true
Attachment #8482006 - Flags: review?(bzbarsky)
Created attachment 8482008 [details] [diff] [review]
Fix <select> validity status for listboxes and for non-placeholder empty valued options

Sorry wrong patch.
Attachment #8482006 - Attachment is obsolete: true
Attachment #8482006 - Flags: review?(bzbarsky)
Attachment #8482008 - Flags: review?(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.
Attachment #8482008 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 12

7 months ago
Created attachment 8794978 [details] [diff] [review]
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.
Attachment #8794978 - Flags: review?(jst)
(Assignee)

Comment 13

7 months ago
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 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!
Attachment #8794978 - Flags: review?(jst) → review+
(Assignee)

Comment 15

7 months ago
Created attachment 8795295 [details] [diff] [review]
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.
Attachment #8794978 - Attachment is obsolete: true
Attachment #8795295 - Flags: review?(jst)
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?
Attachment #8795295 - Flags: review?(jst) → review-
(Assignee)

Comment 17

7 months ago
Created attachment 8795555 [details] [diff] [review]
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).
Attachment #8795295 - Attachment is obsolete: true
Flags: needinfo?(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!
Flags: needinfo?(bzbarsky)
Attachment #8795555 - Flags: review+
(Assignee)

Comment 19

7 months ago
Created attachment 8797251 [details] [diff] [review]
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.
Attachment #8795555 - Attachment is obsolete: true
Flags: needinfo?(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
Flags: needinfo?(bzbarsky)
Attachment #8797251 - Flags: review+
Assignee: nobody → mozilla
Keywords: checkin-needed
Attachment #8482008 - Attachment is obsolete: true

Comment 21

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5aeed15578
Fix <select> validity status for listboxes and for non-placeholder empty valued options. r=bz
Keywords: checkin-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
Flags: needinfo?(mozilla)
Also seeing similar failures like https://treeherder.mozilla.org/logviewer.html#?job_id=37028930&repo=mozilla-inbound possibly only on android.
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
(Assignee)

Comment 25

7 months ago
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.
Status: NEW → ASSIGNED
Flags: needinfo?(mozilla)
(Assignee)

Comment 26

7 months ago
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
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.
(Assignee)

Comment 28

5 months ago
Created attachment 8814237 [details] [diff] [review]
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.
Attachment #8797251 - Attachment is obsolete: true
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky) → needinfo?(mozilla)
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?
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.
(Assignee)

Comment 32

5 months ago
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.
Flags: needinfo?(mozilla)
Created attachment 8815048 [details] [diff] [review]
Tom's patch with the change from comment 29 applied and the reftests fixed
Attachment #8814237 - Attachment is obsolete: true
Created attachment 8815050 [details] [diff] [review]
Patch to fix the optgroup thing; needed for the web platform test to pass
Attachment #8815050 - Flags: review?(bugs)

Updated

5 months ago
Attachment #8815050 - Flags: review?(bugs) → review+
Created attachment 8815534 [details] [diff] [review]
Combined patch
Attachment #8815048 - Attachment is obsolete: true
Assignee: mozilla → bzbarsky
Assignee: bzbarsky → mozilla
Attachment #8815050 - Attachment is obsolete: true

Comment 36

5 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5644a5462ce5
Fix <select> validity status for listboxes and for non-placeholder empty valued options.  r=bzbarsky
Tom, thank you for sticking with this and fixing this bug!
(Assignee)

Comment 38

5 months ago
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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5644a5462ce5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.