Option, checkbox and radio should support ':default' selector

Option, checkbox and radio should support ':default' selector

Spec: https://html.spec.whatwg.org/multipage/scripting.html#selector-default

This patch adds support for the following to match :default
pseudo class
a)input elements to which the checked attribute applies and
that have a checked attribute
b)option elements that have a selected attribute

This patch does not support handling dynamic state changes
to input types to add/remove :default class.

BUG=591226
Committed: https://crrev.com/6e3a05d6080ba6ae25af2835573ff9e2406b90c0
Cr-Commit-Position: refs/heads/master@{#379279}

[ramya.v, 2016-03-03 06:36:32]

ramya.v@samsung.com changed reviewers:
+ dpranke@chromium.org, timloh@chromium.org

[ramya.v, 2016-03-03 06:36:32]

PTAL!

Thanks

[tkent, 2016-03-03 06:46:01]

tkent@chromium.org changed reviewers:
+ tkent@chromium.org

[tkent, 2016-03-03 06:46:02]

You need to add code to support dynamic changes of conditions.  e.g. Changing
input type correctly add/remove :default style.  You can copy some code from
https://codereview.chromium.org/1763553002/ .

You need to add more tests. 
web-platform-tests/html/semantics/selectors/pseudo-classes/default.html doesn't
have enough coverage.

https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/SelectorChecker.cpp:836: if
(isHTMLInputElement(element)) {
You shouldn't add code here.  You should override
HTMLOptionElement::isDefaultButtonForForm and
HTMLInputElement::isDefaultButtonForForm.

https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/SelectorChecker.cpp:838: if
(inputElement.shouldAppearChecked() &&
!inputElement.shouldAppearIndeterminate())
The specification doesn't ask to check them.  It says "have a checked attribute"

https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/SelectorChecker.cpp:840: } else if
(isHTMLOptionElement(element) && toHTMLOptionElement(element).selected()) {
The specification doesn't ask to check |selected| IDL attribute.  It says "have
a selected attribute".

[ramya.v, 2016-03-03 10:29:00]

On 2016/03/03 06:46:02, tkent wrote:
> You need to add code to support dynamic changes of conditions.  e.g. Changing
> input type correctly add/remove :default style.  You can copy some code from
> https://codereview.chromium.org/1763553002/ .
> 
> You need to add more tests. 
> web-platform-tests/html/semantics/selectors/pseudo-classes/default.html
doesn't
> have enough coverage.
> 
>
https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/SelectorChecker.cpp (right):
> 
>
https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/SelectorChecker.cpp:836: if
> (isHTMLInputElement(element)) {
> You shouldn't add code here.  You should override
> HTMLOptionElement::isDefaultButtonForForm and
> HTMLInputElement::isDefaultButtonForForm.
> 
>
https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/SelectorChecker.cpp:838: if
> (inputElement.shouldAppearChecked() &&
> !inputElement.shouldAppearIndeterminate())
> The specification doesn't ask to check them.  It says "have a checked
attribute"
> 
>
https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/SelectorChecker.cpp:840: } else if
> (isHTMLOptionElement(element) && toHTMLOptionElement(element).selected()) {
> The specification doesn't ask to check |selected| IDL attribute.  It says
"have
> a selected attribute".

Thank you for the review comments :).
Updated with new patch-set.

You need to add code to support dynamic changes of conditions.  e.g. Changing
input type correctly add/remove :default style.  You can copy some code from
https://codereview.chromium.org/1763553002/ .

>> Since this patch is under review, once this is landed, will make similar
changes in subsequent patch after this,
for checkbox/radio/option element when they are dynamically updated.

You need to add more tests. 
web-platform-tests/html/semantics/selectors/pseudo-classes/default.html doesn't
have enough coverage.

>> Have added test to check basic support of :default pseudo class for
checkbox/radio/option element. 
Once I add patch for dynamic handling, will add more test cases for the same. 
Please let me know if I have to check any other corner cases.

PTAL!
Thanks

[ramya.v, 2016-03-03 10:29:19]

https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/SelectorChecker.cpp:836: if
(isHTMLInputElement(element)) {
On 2016/03/03 06:46:02, tkent wrote:
> You shouldn't add code here.  You should override
> HTMLOptionElement::isDefaultButtonForForm and
> HTMLInputElement::isDefaultButtonForForm.

Done.

https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/SelectorChecker.cpp:838: if
(inputElement.shouldAppearChecked() &&
!inputElement.shouldAppearIndeterminate())
On 2016/03/03 06:46:02, tkent wrote:
> The specification doesn't ask to check them.  It says "have a checked
attribute"

Done.

https://codereview.chromium.org/1756483005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/SelectorChecker.cpp:840: } else if
(isHTMLOptionElement(element) && toHTMLOptionElement(element).selected()) {
On 2016/03/03 06:46:02, tkent wrote:
> The specification doesn't ask to check |selected| IDL attribute.  It says
"have
> a selected attribute".

Done.

[tkent, 2016-03-03 13:28:57]

On 2016/03/03 at 10:29:00, ramya.v wrote:
> >> Since this patch is under review, once this is landed, will make similar
changes in subsequent patch after this,
> for checkbox/radio/option element when they are dynamically updated.

ok.  Please mention it in the CL description.

[tkent, 2016-03-03 13:29:08]

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html (right):

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html:1: <!DOCTYPE
HTML PUBLIC "-//IETF//DTD HTML//EN">
Please do not add |-005| to the file name.  It's hard to understand what's
tested.
Separate it to pseudo-default-checkbox-radio.html and
pseudo-default-option.html.

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html:4: <script
src="../../resources/js-test.js"></script>
I recommend to use testharness.js for new conformance tests.

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html:18: <input
name="y" type ="radio" checked />
Add more cases:
  - other input types with 'checked' attribute  (This Patch Set fails for them)
  - checkbox/radio without 'checked' attribute, but checked by 'checked'
property

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html:21: <option
name="y" selected>2</option>
Add another case:
  - OPTION without selected attribute, but selected by 'selected' property.

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html:30:
shouldBe("document.defaultView.getComputedStyle(u[i],
null).getPropertyValue('background-color')", "'rgb(255, 0, 0)'");
You can simply check |shouldBeFalse("u[i].matches(':default')")|.

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:1660: if
(fastHasAttribute(checkedAttr))
The specification says "input elements to which the checked attribute applies"
but this code doesn't check it.  

Please add InputType::matchesDefaultPseudoClass(), then
 - HTMLInputElement::isDefaultButtonForForm() just calls
m_inputType->matchesDefaultPseudoClass().
 - BaseCheckableInputType::matchesDefaultPseudoClass() checkes
fastHasAttribute(checkedAttr)
 - BaseButtonInputType::isDefaultButtonForForm() checks
|isSuccessfulSubmitButton() && element().form() &&
element().form()->defaultButton() == &element()| (copy the logic of
HTMLFormControlElement::isDefaultButtonForForm)
 - Copy HTMLFormControlElement::isDefaultButtonForForm to
HTMLButtonElement::isDefaultButtonForForm
 - Remove HTMLFormControlElement::isDefaultButtonForForm

[ramya.v, 2016-03-04 06:30:43]

Description was changed from

==========
Option, checkbox and radio should support ':default' selector

Spec: https://html.spec.whatwg.org/multipage/scripting.html#selector-default

BUG=591226
==========

to

==========
Option, checkbox and radio should support ':default' selector

Spec: https://html.spec.whatwg.org/multipage/scripting.html#selector-default

This patch adds support for the following to match :default
pseudo class
a)input elements to which the checked attribute applies and 
that have a checked attribute
b)option elements that have a selected attribute

This patch does not support handling dynamic state changes 
to input types to add/remove :default class.

BUG=591226
==========

[ramya.v, 2016-03-04 06:31:45]

Updated as per review comments.
PTAL!


Thanks

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html (right):

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html:1: <!DOCTYPE
HTML PUBLIC "-//IETF//DTD HTML//EN">
On 2016/03/03 13:29:07, tkent wrote:
> Please do not add |-005| to the file name.  It's hard to understand what's
> tested.
> Separate it to pseudo-default-checkbox-radio.html and
> pseudo-default-option.html.

Done.

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html:4: <script
src="../../resources/js-test.js"></script>
On 2016/03/03 13:29:07, tkent wrote:
> I recommend to use testharness.js for new conformance tests.

Done.

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html:18: <input
name="y" type ="radio" checked />
On 2016/03/03 13:29:07, tkent wrote:
> Add more cases:
>   - other input types with 'checked' attribute  (This Patch Set fails for
them)
>   - checkbox/radio without 'checked' attribute, but checked by 'checked'
> property

Done.

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html:21: <option
name="y" selected>2</option>
On 2016/03/03 13:29:08, tkent wrote:
> Add another case:
>   - OPTION without selected attribute, but selected by 'selected' property.

Done.

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-005.html:30:
shouldBe("document.defaultView.getComputedStyle(u[i],
null).getPropertyValue('background-color')", "'rgb(255, 0, 0)'");
On 2016/03/03 13:29:08, tkent wrote:
> You can simply check |shouldBeFalse("u[i].matches(':default')")|.

Done.

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/1756483005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:1660: if
(fastHasAttribute(checkedAttr))
On 2016/03/03 13:29:08, tkent wrote:
> The specification says "input elements to which the checked attribute applies"
> but this code doesn't check it.  
> 
> Please add InputType::matchesDefaultPseudoClass(), then
>  - HTMLInputElement::isDefaultButtonForForm() just calls
> m_inputType->matchesDefaultPseudoClass().
>  - BaseCheckableInputType::matchesDefaultPseudoClass() checkes
> fastHasAttribute(checkedAttr)
>  - BaseButtonInputType::isDefaultButtonForForm() checks
> |isSuccessfulSubmitButton() && element().form() &&
> element().form()->defaultButton() == &element()| (copy the logic of
> HTMLFormControlElement::isDefaultButtonForForm)
>  - Copy HTMLFormControlElement::isDefaultButtonForForm to
> HTMLButtonElement::isDefaultButtonForForm
>  - Remove HTMLFormControlElement::isDefaultButtonForForm

Done.

[tkent, 2016-03-04 06:39:13]

tkent@chromium.org changed reviewers:
- dpranke@chromium.org, timloh@chromium.org

[tkent, 2016-03-04 06:39:15]

Almost ok.

Please hold uploading new patch set until
https://codereview.chromium.org/1764063002/ is landed.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/pseudo-default-checkbox-radio.html
(right):

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-checkbox-radio.html:6:
<style>
Remove this STYLE element.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-checkbox-radio.html:45:
assert_false(u[i].matches(':default'));
Your JavaScript code use single-quotes and double-quotes inconsistently.
Please use one of them.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/pseudo-default-option.html (right):

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-option.html:6: <style>
Remove this STYLE element.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-option.html:24:
assert_false(u[i].matches(':default'));
wrong indentation.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-option.html:27:
assert_true(v[i].matches(':default'));
wrong indentation

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLButtonElement.cpp (right):

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLButtonElement.cpp:218: {
Please copy the comment in HTMLFormControlElement::isDefaultButtonForForm too.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/BaseButtonInputType.cpp (right):

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/BaseButtonInputType.cpp:87: {
Please copy the comment in HTMLFormControlElement::isDefaultButtonForForm too.

[ramya.v, 2016-03-04 10:22:40]

PTAL!

Thanks

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/pseudo-default-checkbox-radio.html
(right):

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-checkbox-radio.html:6:
<style>
On 2016/03/04 06:39:14, tkent wrote:
> Remove this STYLE element.

Done.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-checkbox-radio.html:45:
assert_false(u[i].matches(':default'));
On 2016/03/04 06:39:14, tkent wrote:
> Your JavaScript code use single-quotes and double-quotes inconsistently.
> Please use one of them.

Done.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/pseudo-default-option.html (right):

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-option.html:6: <style>
On 2016/03/04 06:39:14, tkent wrote:
> Remove this STYLE element.

Done.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-option.html:24:
assert_false(u[i].matches(':default'));
On 2016/03/04 06:39:14, tkent wrote:
> wrong indentation.

Done.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-default-option.html:27:
assert_true(v[i].matches(':default'));
On 2016/03/04 06:39:14, tkent wrote:
> wrong indentation

Done.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLButtonElement.cpp (right):

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLButtonElement.cpp:218: {
On 2016/03/04 06:39:14, tkent wrote:
> Please copy the comment in HTMLFormControlElement::isDefaultButtonForForm too.

Done.

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/BaseButtonInputType.cpp (right):

https://codereview.chromium.org/1756483005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/BaseButtonInputType.cpp:87: {
On 2016/03/04 06:39:14, tkent wrote:
> Please copy the comment in HTMLFormControlElement::isDefaultButtonForForm too.

Done.

[tkent, 2016-03-04 12:32:35]

The CQ bit was checked by tkent@chromium.org

[tkent, 2016-03-04 12:32:36]

lgtm

[commit-bot: I haz the power, 2016-03-04 12:32:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1756483005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1756483005/100001

[commit-bot: I haz the power, 2016-03-04 13:42:36]

Description was changed from

==========
Option, checkbox and radio should support ':default' selector

Spec: https://html.spec.whatwg.org/multipage/scripting.html#selector-default

This patch adds support for the following to match :default
pseudo class
a)input elements to which the checked attribute applies and 
that have a checked attribute
b)option elements that have a selected attribute

This patch does not support handling dynamic state changes 
to input types to add/remove :default class.

BUG=591226
==========

to

==========
Option, checkbox and radio should support ':default' selector

Spec: https://html.spec.whatwg.org/multipage/scripting.html#selector-default

This patch adds support for the following to match :default
pseudo class
a)input elements to which the checked attribute applies and 
that have a checked attribute
b)option elements that have a selected attribute

This patch does not support handling dynamic state changes 
to input types to add/remove :default class.

BUG=591226
==========

[commit-bot: I haz the power, 2016-03-04 13:42:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-04 13:44:37]

Description was changed from

==========
Option, checkbox and radio should support ':default' selector

Spec: https://html.spec.whatwg.org/multipage/scripting.html#selector-default

This patch adds support for the following to match :default
pseudo class
a)input elements to which the checked attribute applies and 
that have a checked attribute
b)option elements that have a selected attribute

This patch does not support handling dynamic state changes 
to input types to add/remove :default class.

BUG=591226
==========

to

==========
Option, checkbox and radio should support ':default' selector

Spec: https://html.spec.whatwg.org/multipage/scripting.html#selector-default

This patch adds support for the following to match :default
pseudo class
a)input elements to which the checked attribute applies and
that have a checked attribute
b)option elements that have a selected attribute

This patch does not support handling dynamic state changes
to input types to add/remove :default class.

BUG=591226

Committed: https://crrev.com/6e3a05d6080ba6ae25af2835573ff9e2406b90c0
Cr-Commit-Position: refs/heads/master@{#379279}
==========

[commit-bot: I haz the power, 2016-03-04 13:44:38]

Patchset 6 (id:??) landed as
https://crrev.com/6e3a05d6080ba6ae25af2835573ff9e2406b90c0
Cr-Commit-Position: refs/heads/master@{#379279}