Bug 79764 – [Forms] The option element should not be form associated element.

• Component: Forms
• Status: RESOLVED FIXED

Imported from http://crbug.com/90094

[Alexey Proskuryakov]

Duplicate of bug 77502? What does profiler say?

Yes, root cause of this bug and bug 77502 is same.

I changed bug summary to clarify what I will do == make HTMLOptionElement not to inherit FormAssociateElement.

I'll do same thing other non-form associate elements, such as legend, optgroup, etc.

[Kent Tamura]

(In reply to comment #0)
> Imported from http://crbug.com/90094

This is unfriendly bug description.  You should have written what was the bug instead of the URL.

(In reply to comment #1)
> Duplicate of bug 77502? 
No, see comment #2

>What does profiler say?

I measure running time of my desktop 
http://jsfiddle.net/pcc7Z/

Insert into div takes 319ms
Insert into form takes 2113ms

Created attachment 132553 [details]
Patch 1

[Kent Tamura]

Comment on attachment 132553 [details]
Patch 1

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

> Source/WebCore/css/CSSStyleSelector.cpp:1349
> +    if (element->hasTagName(optionTag))
> +        return false;
> +

This is wrong.  It means we don't share a style for <option> elements in any cases.
We should check:
 if both of m_element and element are optionTag,
    if enabled state is different, false
    if checked state is different, false
    otherwise, fall-through (means we can share the style)

> LayoutTests/fast/forms/resources/select-live-pseudo-selectors.js:55
> +    if (typeof(el) == 'string') {
> +      el = document.getElementById(el);

inconsistent indentation.

Created attachment 132558 [details]
Patch 2

[Kent Tamura]

Comment on attachment 132558 [details]
Patch 2

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

> Source/WebCore/css/CSSStyleSelector.cpp:1355
> +        if (thisOptionElement->disabled() != otherOptionElement->disabled())
> +            return false;

nit: We should use isEnabledFormControl() for consistency with SelectorChecker.cpp.

Created attachment 132942 [details]
Patch 3

[Kent Tamura]

Comment on attachment 132942 [details]
Patch 3

ok

[WebKit Review Bot]

Comment on attachment 132942 [details]
Patch 3

Clearing flags on attachment: 132942

Committed r111497: <http://trac.webkit.org/changeset/111497>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke 2 tests on all platform:
 - fast/css/text-transform-select.html
 - fast/forms/select/menulist-disabled-option.html

[Csaba Osztrogonác]

(In reply to comment #13)
> Reopen, because it broke 2 tests on all platform:
>  - fast/css/text-transform-select.html
>  - fast/forms/select/menulist-disabled-option.html

See https://bugs.webkit.org/show_bug.cgi?id=81746 for details.

(In reply to comment #6)
> (From update of attachment 132553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132553&action=review
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:1349
> > +    if (element->hasTagName(optionTag))
> > +        return false;
> > +
> 
> This is wrong.  It means we don't share a style for <option> elements in any cases.
> We should check:
>  if both of m_element and element are optionTag,
>     if enabled state is different, false
>     if checked state is different, false
>     otherwise, fall-through (means we can share the style)
> 

It seems this changes current behavior.
Current implementation doesn't share style for HTMLOptionElement, HTMLOptionElement isn't HTMLInputElement.

bool CSSStyleSelector::canShareStyleWithControl(StyledElement* element) const
{
    HTMLInputElement* thisInputElement = element->toInputElement();
    HTMLInputElement* otherInputElement = m_element->toInputElement();

    if (!thisInputElement || !otherInputElement)
        return false;

[Kent Tamura]

(In reply to comment #15)
> (In reply to comment #6)
> > (From update of attachment 132553 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=132553&action=review
> > 
> > > Source/WebCore/css/CSSStyleSelector.cpp:1349
> > > +    if (element->hasTagName(optionTag))
> > > +        return false;
> > > +
> > 
> > This is wrong.  It means we don't share a style for <option> elements in any cases.
> > We should check:
> >  if both of m_element and element are optionTag,
> >     if enabled state is different, false
> >     if checked state is different, false
> >     otherwise, fall-through (means we can share the style)
> > 
> 
> It seems this changes current behavior.
> Current implementation doesn't share style for HTMLOptionElement, HTMLOptionElement isn't HTMLInputElement.

ok, let's revert this part.
Did you reproduced the test failures with r111497, and confirmed they were fixed by reverting this part?

> > > > Source/WebCore/css/CSSStyleSelector.cpp:1349
> > > > +    if (element->hasTagName(optionTag))
> > > > +        return false;
> > > > +
> > > 
> > > This is wrong.  It means we don't share a style for <option> elements in any cases.
> > > We should check:
> > >  if both of m_element and element are optionTag,
> > >     if enabled state is different, false
> > >     if checked state is different, false
> > >     otherwise, fall-through (means we can share the style)
> > > 
> > 
> > It seems this changes current behavior.
> > Current implementation doesn't share style for HTMLOptionElement, HTMLOptionElement isn't HTMLInputElement.
> 
> ok, let's revert this part.
> Did you reproduced the test failures with r111497, and confirmed they were fixed by reverting this part?

Confirmed on Mac port. I'm not sure why Chromium-Linux port passed test but not others. (><)

Created attachment 133196 [details]
Patch 4

[WebKit Review Bot]

Comment on attachment 133196 [details]
Patch 4

Clearing flags on attachment: 133196

Committed r111659: <http://trac.webkit.org/changeset/111659>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.