Bug 50380 – Implement "required" attribute for select tags

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Dai Mikurube]

Implementing "required" attribute for <select>.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#attr-select-required

[Dai Mikurube]

This bug depends on the bug 43887 because an attribute "required" refers an attribute "size" of <select>.

[Kent Tamura]

(In reply to comment #1)
> This bug depends on the bug 43887 because an attribute "required" refers an attribute "size" of <select>.

I don't think this depends on Bug 43887.  We don't need to change the external behavior of HTMLSelectElement::size to refer "display size" internally.

[Dai Mikurube]

(In reply to comment #2)
It looks like "display size" differs from "size" or default size, only if "size" cannot be parsed by rules for non-negative integers. In my understanding, "display size" must be the default "size" if "size" is absent.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#concept-select-size
> The display size of a select element is the result of applying the rules for parsing non-negative integers to the value of element's size attribute, if it has one and parsing it is successful. If applying those rules to the attribute's value is not successful, or if the size attribute is absent, the element's display size is the default value of the attribute.


In addition, the internal value corresponding to "display size" is only in the WebCore/renderer. Referring values inside the renderer from WebCore/html is unreasonable, I think. To introduce "display size" appropriately, it is better to add a new member variable into HTMLSelectElement. But it may be redundant.

Reading the discussion at the bug 43887, IFMO, ignoring "required" if size > 1 and non-multiple may be reasonable. (Though ignoring it if size == 1 and non-multiple is compliant.) What do you think?

[Dai Mikurube]

(In reply to comment #3)
> It looks like "display size" differs from "size" or default size, only if "size" cannot be parsed by rules for non-negative integers. In my understanding, "display size" must be the default "size" if "size" is absent.

It's misleading. "display size" differs from "size" only if "size" is absent or cannot be parsed. In this case, "display size" must be the default value of "size". Otherwise, "display size" must be the result of applying rules for parsing.

[Kent Tamura]

(In reply to comment #3)
> In addition, the internal value corresponding to "display size" is only in the WebCore/renderer.
> Referring values inside the renderer from WebCore/html is unreasonable, I think. To introduce "display
> size" appropriately, it is better to add a new member variable into HTMLSelectElement. But it may be
> redundant.

We can calculate the display-size on the fly in HTMLSelectElement, right?

[Dai Mikurube]

Created attachment 75465 [details]
Patch

[Dai Mikurube]

(In reply to comment #5)
> We can calculate the display-size on the fly in HTMLSelectElement, right?

My note was misleading. We can calculate display-size on the fly, but :

1. The display-size must not be different from external size eventually. (unless size is absent or invalid.)
2. It's still redundant since a similar calculation is done in the renderer.
3. This calculation has no special difference from ignoring "required" if size > 1 and non-multiple.


Attached a patch with "size > 1". It can be replaced with on-the-fly calculating display-size, if required.

[Kent Tamura]

Comment on attachment 75465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75465&action=review

> LayoutTests/fast/forms/ValidityState-valueMissing-001.html:30
> +        shouldBe('v[0].validity.valueMissing', 'true');
> +        shouldBe('v[1].validity.valueMissing', 'true');
> +        shouldBe('v[2].validity.valueMissing', 'true');
> +        shouldBe('v[3].validity.valueMissing', 'false');
> +        shouldBe('v[4].validity.valueMissing', 'false');
> +        shouldBe('v[5].validity.valueMissing', 'false');
> +        shouldBe('v[6].validity.valueMissing', 'false');
> +        shouldBe('v[7].validity.valueMissing', 'false');
> +        shouldBe('v[8].validity.valueMissing', 'false');
> +        shouldBe('v[9].validity.valueMissing', 'false');

The resultant text doesn't have good readability.
How about adding id= attributes to the victims, and change these checks like

function valueMissingFor(id) { return document.getElmeentById(id).validity.valueMissing; }
shouldBeTrue('valueMissingFor("input-requied")');
shouldBeFalse('valueMissingFor("select-without-placeholder-label")');

> LayoutTests/fast/forms/ValidityState-valueMissing-001.html:40
>  <body onload="test()">

We don't need to make this test async though It's not your responsibility.
We can move the <script> element into <body>, and add <script> for js-test-post.js like tests you have updated before.

> LayoutTests/fast/forms/ValidityState-valueMissing-002.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Same comments as ValidityState-valueMissing-001.html.

> LayoutTests/fast/forms/ValidityState-valueMissing-003.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Same comments as ValidityState-valueMissing-001.html.

> LayoutTests/fast/forms/checkValidity-002.html:31
> +shouldBe("v[0].checkValidity()", "false");
> +shouldBe("v[1].checkValidity()", "false");
> +shouldBe("v[2].checkValidity()", "false");
> +shouldBe("v[3].checkValidity()", "false");
> +shouldBe("v[4].checkValidity()", "true");

Same comments as ValidityState-valueMissing-001.html.

> LayoutTests/fast/forms/required-attribute-001.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Same comments as ValidityState-valueMissing-001.html.

> LayoutTests/fast/forms/required-attribute-002.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Same comments as ValidityState-valueMissing-001.html.

> LayoutTests/fast/forms/resources/select-live-pseudo-selectors.js:10
> +    var i = document.createElement('select');

Using the name "i" for this is not reasonable.

> WebCore/html/HTMLSelectElement.cpp:115
> +    if (!isRequiredFormControl() || disabled() || multiple() || size() > 1)

We don't need to check disabled() here.  It's responsibility of willValidate().

We should not return false for multiple() unconditionally.  The specification says we never have placeholder label option for multiple, but required should work with multiple.

> WebCore/html/HTMLSelectElement.cpp:297
> +    bool result = SelectElement::appendFormData(m_data, this, list);
> +    if (result)
> +        setNeedsValidityCheck();

Why is this needed?

> WebCore/html/HTMLSelectElement.cpp:417
> +    setNeedsValidityCheck();

We don't need this.  setAttribute() calls parseMappedAttribute().

> WebCore/html/HTMLSelectElement.cpp:423
> +    setNeedsValidityCheck();

We should handle this in parseMappedAttribtue().

> WebCore/html/HTMLSelectElement.cpp:502
> +    setNeedsValidityCheck();

Why is this needed?

> WebCore/html/HTMLSelectElement.h:48
> +    bool valueMissing(const String&) const;
> +    bool isValidValue(const String& candidate) const { return !valueMissing(candidate); }

isValidValue() is not used.  We don't need to add it.
So, the parameter of valueMissing() is not needed.  It's always this->value().

[Dai Mikurube]

Comment on attachment 75465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75465&action=review

For tests, later.

>> WebCore/html/HTMLSelectElement.cpp:115
>> +    if (!isRequiredFormControl() || disabled() || multiple() || size() > 1)
> 
> We don't need to check disabled() here.  It's responsibility of willValidate().
> 
> We should not return false for multiple() unconditionally.  The specification says we never have placeholder label option for multiple, but required should work with multiple.

Thanks, the code is wrong. Revised.

>> WebCore/html/HTMLSelectElement.cpp:297
>> +        setNeedsValidityCheck();
> 
> Why is this needed?

I misunderstood the role of appendFormData(). It looks better to change the first argument, data, of appendForData to const.

>> WebCore/html/HTMLSelectElement.cpp:417
>> +    setNeedsValidityCheck();
> 
> We don't need this.  setAttribute() calls parseMappedAttribute().

Thanks. Removed.

>> WebCore/html/HTMLSelectElement.cpp:502
>> +    setNeedsValidityCheck();
> 
> Why is this needed?

Exactly. Removed. It looks better to change data to const, too.

>> WebCore/html/HTMLSelectElement.h:48
>> +    bool isValidValue(const String& candidate) const { return !valueMissing(candidate); }
> 
> isValidValue() is not used.  We don't need to add it.
> So, the parameter of valueMissing() is not needed.  It's always this->value().

Removed. It followed HTMLInputElement and HTMLTextAreaElement. Isn't it required? It looks a kind of inconsistent.
HTMLTextAreaElement::isValidValue() looks not called. (It is called from WebKit/chromium as for HTMLInputElement.)

[Dai Mikurube]

Comment on attachment 75465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75465&action=review

>>> WebCore/html/HTMLSelectElement.cpp:115
>>> +    if (!isRequiredFormControl() || disabled() || multiple() || size() > 1)
>> 
>> We don't need to check disabled() here.  It's responsibility of willValidate().
>> 
>> We should not return false for multiple() unconditionally.  The specification says we never have placeholder label option for multiple, but required should work with multiple.
> 
> Thanks, the code is wrong. Revised.

wilValidate() is checked at checkValidity(), but not checked when directly reading properties validity.*. So it is required to check disable() (or willValidate()) here.

[Dai Mikurube]

Created attachment 75491 [details]
Patch

[Dai Mikurube]

Created attachment 75494 [details]
Patch

[Dai Mikurube]

I missed the description on optgroups and disabled :
>  the value of the first option element  ... and that option is not disabled, and, finally, that option element's parent node is the select element (and not an optgroup element),

[Kent Tamura]

Comment on attachment 75494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75494&action=review

> LayoutTests/fast/forms/ValidityState-valueMissing-001-expected.txt:2
> -There are two form control elements below, both required and blank: validity.valueMissing should be true in both cases.
> +This test checks validity.valueMissing with blank values, blank options selected, or nothing selected.
>  

The purpose of ValidityState-valueMissing-001.html was test cases for validity.valueMissing==true.
If you follow it, you should remove cases with valueMissing==false.

> LayoutTests/fast/forms/ValidityState-valueMissing-001.html:71
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

We don't need to call waitUntilDone() and notifyDone().  The document text is dumped automatically when the document loading is completed.

> LayoutTests/fast/forms/ValidityState-valueMissing-001.html:91
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/ValidityState-valueMissing-002.html:53
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

ditto

> LayoutTests/fast/forms/ValidityState-valueMissing-002.html:69
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/ValidityState-valueMissing-003-expected.txt:15
> +FAIL valueMissingFor("select-none-selected-multiple") should be false. Was true.
> +PASS valueMissingFor("select-fake-placeholder-selected-multiple") is false
> +PASS valueMissingFor("select-without-fake-placeholder-multiple") is false
> +FAIL valueMissingFor("select-none-selected-size2-multiple") should be false. Was true.

The result contains FAIL lines.

> LayoutTests/fast/forms/ValidityState-valueMissing-003.html:61
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

waitUntilDone() and notifyDone() are not needed.

> LayoutTests/fast/forms/ValidityState-valueMissing-003.html:79
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/checkValidity-002-expected.txt:11
> +PASS checkValidityFor("select-non-placeholder") is true

checkValidity-002.html was for cases with checkValidity()==false.  You shouldn't include checkValidity()==true case if you follow it.

> LayoutTests/fast/forms/required-attribute-001.html:25
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

waitUntilDone() and notifyDone() are not needed.

> LayoutTests/fast/forms/required-attribute-001.html:34
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/required-attribute-002.html:25
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

ditto.

> LayoutTests/fast/forms/required-attribute-002.html:46
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/resources/select-live-pseudo-selectors.js:76
> +// --------------------------------
> +//     value change
> +// --------------------------------

We need more test cases for this.
You added some setNeedsValidityCheck() calls.  The test should cover all of code paths with them.

> WebCore/dom/SelectElement.cpp:217
> -void SelectElement::scrollToSelection(SelectElementData& data, Element* element)
> +void SelectElement::scrollToSelection(const SelectElementData& data, Element* element)

Adding "const" to existing code is not related to the bug.  Please make a separated bug and patch.

> WebCore/html/HTMLSelectElement.cpp:119
> +    // The number of the first option selected.
> +    int index = selectedIndex();

The variable name should be "firstSelectionIndex" and the comment should be removed.

[Kent Tamura]

(In reply to comment #10)
> >>> WebCore/html/HTMLSelectElement.cpp:115
> >>> +    if (!isRequiredFormControl() || disabled() || multiple() || size() > 1)
> >> 
> >> We don't need to check disabled() here.  It's responsibility of willValidate().

> wilValidate() is checked at checkValidity(), but not checked when directly reading properties validity.*. So it is required to check disable() (or willValidate()) here.

Oh, your understanding of the current code is correct.
I found the current code is wrong.  If willValidate is false, ValidityState::valueMissing() should return false and we should not call HTMLSelectElement::valueMissing().  Each of exposed functions of ValidityState should check willValidate() at their beginning like ValidityState::validationMessage() is doing.
Would you fix ValidityState::valueMissing() in this bug, and fix other functions of ValidityState in another bug please?

[Kent Tamura]

(In reply to comment #9)
> >> WebCore/html/HTMLSelectElement.h:48
> >> +    bool isValidValue(const String& candidate) const { return !valueMissing(candidate); }
> > 
> > isValidValue() is not used.  We don't need to add it.
> > So, the parameter of valueMissing() is not needed.  It's always this->value().
> 
> Removed. It followed HTMLInputElement and HTMLTextAreaElement. Isn't it required? It looks a kind of inconsistent.
> HTMLTextAreaElement::isValidValue() looks not called. (It is called from WebKit/chromium as for HTMLInputElement.)

isValidValue() was introduced in orde that browsers can set valid values by their auto-fill / completion features.  Setting valid value to <select> is easy because it's just setting an index, and checking a string value doesn't make much sense.

[Dai Mikurube]

Comment on attachment 75494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75494&action=review

>> LayoutTests/fast/forms/ValidityState-valueMissing-001-expected.txt:2
>>  
> 
> The purpose of ValidityState-valueMissing-001.html was test cases for validity.valueMissing==true.
> If you follow it, you should remove cases with valueMissing==false.

I considered it as test cases for validity.valueMissing when blank values are given.
"required" attributes are common in these elements, though they may have different results in some conditions.

So, IFMO, collecting cases by similar conditions (e.g. with empty values / with some non-empty values) may be convenient than separating them by their results to refer and compare them later.
Do you think it is better to keep "valueMissing==false" ?

>> LayoutTests/fast/forms/ValidityState-valueMissing-003-expected.txt:15
>> +FAIL valueMissingFor("select-none-selected-size2-multiple") should be false. Was true.
> 
> The result contains FAIL lines.

Oh, I missed to replace the the test case after changing the source and my understanding of the spec. "true" is correct for the FAIL cases, without any selected options.

>> LayoutTests/fast/forms/checkValidity-002-expected.txt:11
>> +PASS checkValidityFor("select-non-placeholder") is true
> 
> checkValidity-002.html was for cases with checkValidity()==false.  You shouldn't include checkValidity()==true case if you follow it.

As ValidityState-valueMissing-001.html, this test collects cases for checkValidity().

>> LayoutTests/fast/forms/resources/select-live-pseudo-selectors.js:76
>> +// --------------------------------
> 
> We need more test cases for this.
> You added some setNeedsValidityCheck() calls.  The test should cover all of code paths with them.

Ok, adding the cases.

>> WebCore/html/HTMLSelectElement.cpp:119
>> +    int index = selectedIndex();
> 
> The variable name should be "firstSelectionIndex" and the comment should be removed.

Exactly, done it. Thanks.

[Kent Tamura]

(In reply to comment #17)
> > The purpose of ValidityState-valueMissing-001.html was test cases for validity.valueMissing==true.
> > If you follow it, you should remove cases with valueMissing==false.
> 
> I considered it as test cases for validity.valueMissing when blank values are given.
> "required" attributes are common in these elements, though they may have different results in some conditions.

Probably I was wrong.  Your policy for this looks better.  It's ok to go ahead as is.

[Dai Mikurube]

(In reply to comment #15)
> Oh, your understanding of the current code is correct.
> I found the current code is wrong.  If willValidate is false, ValidityState::valueMissing() should return false and we should not call HTMLSelectElement::valueMissing().  Each of exposed functions of ValidityState should check willValidate() at their beginning like ValidityState::validationMessage() is doing.
> Would you fix ValidityState::valueMissing() in this bug, and fix other functions of ValidityState in another bug please?

Ok, I'll fix them in this and another bugs.


(In reply to comment #16)
> isValidValue() was introduced in orde that browsers can set valid values by their auto-fill / completion features.  Setting valid value to <select> is easy because it's just setting an index, and checking a string value doesn't make much sense.

Thanks, I understand. Keep it removed.

[Dai Mikurube]

(In reply to comment #17)
I'm adding test cases for calling sites of setNeedsValidityCheck(). But I found that no existing tests for HTMLSelectElement::listBoxSelectItem() and updateListBoxSelection(). (No existing tests have passed these functions.)

Do you have any ideas to test them?

For updateListBoxSelection(), I found it is called in "autoscroll" by mouse-dragging from WebCore/renderer/RenderListBox.cpp. But I'm not sure how to cause it explicitly (by a kind of events?).

For listBoxSelectItem, I could not find when it is called...

[Kent Tamura]

(In reply to comment #20)
> (In reply to comment #17)
> I'm adding test cases for calling sites of setNeedsValidityCheck(). But I found that no existing tests for HTMLSelectElement::listBoxSelectItem() and updateListBoxSelection(). (No existing tests have passed these functions.)
> 
> Do you have any ideas to test them?
> 
> For updateListBoxSelection(), I found it is called in "autoscroll" by mouse-dragging from WebCore/renderer/RenderListBox.cpp. But I'm not sure how to cause it explicitly (by a kind of events?).

I'm not sure how updateListBoxSelection() is called.  Anyway, if you can make it be called by mouse operations, DumpRenderTree can simulate it by window.eventSender.
If it's very hard to write a test for updateListBoxSelection(), you may skip to make it.

> For listBoxSelectItem, I could not find when it is called...

http://trac.webkit.org/changeset/56180
It seems that it's not called yet.  We can ignore it.

[Dai Mikurube]

Created attachment 75788 [details]
Patch

[Dai Mikurube]

(In reply to comment #21)
Hi Kent-san,

Thanks,

> I'm not sure how updateListBoxSelection() is called.  Anyway, if you can make it be called by mouse operations, DumpRenderTree can simulate it by window.eventSender.
> If it's very hard to write a test for updateListBoxSelection(), you may skip to make it.

Finally I've skipped to make that test. I could not tell the condition to occur "autoscroll".

> > For listBoxSelectItem, I could not find when it is called...
> 
> http://trac.webkit.org/changeset/56180
> It seems that it's not called yet.  We can ignore it.

Ok, I've ignored it.

[Kent Tamura]

Comment on attachment 75788 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75788&action=review

Please rebase.  Unfortunately, another patch which conflicts with this has been committed.

> WebCore/html/HTMLSelectElement.cpp:134
> +    // If nothing is selected, it's value-missing.

Translating simple code to English is not helpful.  Please remove the comment.
We usually add a comment which describes a reason of unclear code.  So the above comment about size() is good.

> WebCore/html/HTMLSelectElement.cpp:137
> +    // If a non-placeholer label option is selected, it's not value-missing.

This is a good comment.  It explains why 0 is not included.

> WebCore/html/HTMLSelectElement.cpp:142
> +    // It may be a placeholder label option.
> +    // Check if the option is not disabled and not under optgroup elements, and the value is the empty string.

This comment is not good because it doesn't explain the reason of the code, but the code is unclear for one who doesn't know "placeholder label option" concept.
So, we should introduce a new function, hasPlaceholderLabelOption(), and should remove the comment.

> WebCore/html/HTMLSelectElement.cpp:206
> -String HTMLSelectElement::value()
> +String HTMLSelectElement::value() const

This change is not related to this bug.

[Dai Mikurube]

Comment on attachment 75788 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75788&action=review

>> WebCore/html/HTMLSelectElement.cpp:206
>> +String HTMLSelectElement::value() const
> 
> This change is not related to this bug.

Actually, it is required for the new function bool HTMLSelectElement::valueMissing() const.
Without this const, we have to make valueMissing() non-const, because of "value().isEmtpy()" at the end of valueMissing(). But it looks unreasonable.

[Dai Mikurube]

Created attachment 75795 [details]
Patch

[Kent Tamura]

(In reply to comment #25)
> >> WebCore/html/HTMLSelectElement.cpp:206
> >> +String HTMLSelectElement::value() const
> > 
> > This change is not related to this bug.
> 
> Actually, it is required for the new function bool HTMLSelectElement::valueMissing() const.
> Without this const, we have to make valueMissing() non-const, because of "value().isEmtpy()" at the end of valueMissing(). But it looks unreasonable.

ok, I understand.  So WebCore/ChangeLog should have an item for HTMLSelectElement::value, and it has this explanation.  The ChangeLog seems out of sync.

[Dai Mikurube]

Created attachment 75797 [details]
Patch

[Dai Mikurube]

(In reply to comment #27)
Thanks for the feedback. I've added it to WebCore/ChangeLog.

[Kent Tamura]

Comment on attachment 75795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75795&action=review

> WebCore/html/HTMLSelectElement.cpp:121
> +bool HTMLSelectElement::hasPlaceholderLabelOption() const
> +{
> +    int listIndex = optionToListIndex(0);
> +    ASSERT(listIndex >= 0);
> +    if (listIndex < 0)
> +        return false;
> +    HTMLOptionElement* option = static_cast<HTMLOptionElement*>(listItems()[listIndex]);
> +    return !option->disabled() && !listIndex && value().isEmpty();
> +}

value() should not be used here.  It should be the value of the "option".

Because this function might be called by others in the future, the implementation had better follow the definition of the specification as possible.
- multiple()-check should be moved here.
- size()-check should be moved here.

Then, valueMissing() implementation can be cleaner like:
return firstSelectionIndex < 0 || (!firstSelectedIndex && hasPlaceholderLabelOption());

> WebCore/html/ValidityState.cpp:109
>  bool ValidityState::valueMissing() const
>  {
> +    if (!m_control->willValidate())
> +        return false;
> +
>      if (m_control->hasTagName(inputTag)) {

This part conflicts with the latest WebKit.  Please rebase.

[WebKit Review Bot]

Attachment 75797 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75797 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75797 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75797 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dai Mikurube]

Created attachment 75861 [details]
Patch

[Dai Mikurube]

(In reply to comment #30)
Thank you for comments. Rebased again and modified.

[Kent Tamura]

Comment on attachment 75861 [details]
Patch

Almost OK.  We should have test cases for <select> without any options in ValidityState-valueMissing-xxx.

[Dai Mikurube]

Created attachment 75863 [details]
Patch

[Dai Mikurube]

(In reply to comment #37)
Ok, I've added "select-no-option" for ValidityState-valueMissing-001 and ValidityState-valueMissing-003.

[Kent Tamura]

Comment on attachment 75863 [details]
Patch

ok

[WebKit Review Bot]

Attachment 75797 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 75863 [details]
Patch

Clearing flags on attachment: 75863

Committed r73494: <http://trac.webkit.org/changeset/73494>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

(In reply to comment #42)
> (From update of attachment 75863 [details])
> Clearing flags on attachment: 75863
> 
> Committed r73494: <http://trac.webkit.org/changeset/73494>

It caused an assertion failure, and was rolled out.

[Dai Mikurube]

Created attachment 76013 [details]
Patch

[Dai Mikurube]

(In reply to comment #44)
Fixed and checked with assertion.

It was because changing "multiple" attribute causes validity check inside SelectElement::parseMultipleAttribute(). It's to replace a menulist with a listbox when changed.

I've added calling HTMLSelectElement::setNeedsValidityCheck() in SelectElement::parseMultipleAttribute() here. But I'd like to make sure that it is ok to call WebCore/html functions from WebCore/dom.

[Kent Tamura]

(In reply to comment #46)
> I've added calling HTMLSelectElement::setNeedsValidityCheck() in SelectElement::parseMultipleAttribute() here. But I'd like to make sure that it is ok to call WebCore/html functions from WebCore/dom.

It's not ok. The element can be HTMLKeygenElement or WMLSelectElement.

Is is too late to call setNeedsValdityCheck() in HTMLSelectElement::parseMappedAttribute() just after calling parseMultipleAttribute()?
If it doesn't work, we should add pure virtual setNeedsValidityCheck() to SelectElement interface and add an empty implemention to WMLSelectElement.

[Dai Mikurube]

(In reply to comment #47)
> Is is too late to call setNeedsValdityCheck() in HTMLSelectElement::parseMappedAttribute() just after calling parseMultipleAttribute()?
> If it doesn't work, we should add pure virtual setNeedsValidityCheck() to SelectElement interface and add an empty implemention to WMLSelectElement.

Unfortunately, it doesn't work. setNeedsValidityCheck() is required between data.setMultiple(...) and element->attach().
In addition, HTMLSelectElement's setNeedsValidityCheck() comes from HTMLFormControlElementWithState. HTMLSelectElement extends both SelectElement and HTMLFormControlElementWithState. Then, adding a function "setNeedsValidityCheck" to SelectElement makes a conflict with HTMLFormControlElementWithState.

Another option is to check "element" is HTMLSelectElement or not in SelectElement::parseMultipleAttribute(). What do you think on it?

Separating SelectElement::parseMultipleAttribute into two functions may be an option. But it requires to change all sites calling parseMultipleAttribute().

[Kent Tamura]

(In reply to comment #48)
> Unfortunately, it doesn't work. setNeedsValidityCheck() is required between data.setMultiple(...) and element->attach().

ok.

> In addition, HTMLSelectElement's setNeedsValidityCheck() comes from HTMLFormControlElementWithState. HTMLSelectElement extends both SelectElement and HTMLFormControlElementWithState. Then, adding a function "setNeedsValidityCheck" to SelectElement makes a conflict with HTMLFormControlElementWithState.

So, it's ok to change the function name.
- Add pure virtual SelectElement::updateValidity().
- Implement an empty updateValidity() in WMLSelectElement
- Implement HTMLSelectElement::updateValidity(), which just calls setNeedsValidityCheck().

[Dai Mikurube]

(In reply to comment #49)
Ok, no need to add empty updateValidity() in HTMLKeygenElement to do nothing?

[Kent Tamura]

(In reply to comment #50)
> (In reply to comment #49)
> Ok, no need to add empty updateValidity() in HTMLKeygenElement to do nothing?

It should not be needed.  HTMLKeygenElement is a subclass of HTMLSelectElement.

[Dai Mikurube]

Created attachment 76017 [details]
Patch

[Dai Mikurube]

(In reply to comment #51)
> It should not be needed.  HTMLKeygenElement is a subclass of HTMLSelectElement.

Ok, fixed it.

[Kent Tamura]

Comment on attachment 76017 [details]
Patch

ok

[WebKit Commit Bot]

Comment on attachment 76017 [details]
Patch

Clearing flags on attachment: 76017

Committed r73606: <http://trac.webkit.org/changeset/73606>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.