Bug 79764 - [Forms] The option element should not be form associated element.
Summary: [Forms] The option element should not be form associated element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL: http://jsfiddle.net/pcc7Z/
Keywords:
Depends on: 80089 81746 81774
Blocks: 80381 80088
  Show dependency treegraph
 
Reported: 2012-02-28 01:07 PST by yosin
Modified: 2012-03-22 01:04 PDT (History)
6 users (show)

See Also:


Attachments
Patch 1 (18.90 KB, patch)
2012-03-19 00:29 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (19.56 KB, patch)
2012-03-19 01:47 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (19.47 KB, patch)
2012-03-20 18:17 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (18.99 KB, patch)
2012-03-21 23:50 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-02-28 01:07:27 PST
Imported from http://crbug.com/90094
Comment 1 Alexey Proskuryakov 2012-02-28 12:57:04 PST
Duplicate of bug 77502? What does profiler say?
Comment 2 yosin 2012-02-28 20:49:09 PST
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.
Comment 3 Kent Tamura 2012-02-28 23:11:22 PST
(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.
Comment 4 yosin 2012-02-28 23:15:45 PST
(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
Comment 5 yosin 2012-03-19 00:29:43 PDT
Created attachment 132553 [details]
Patch 1
Comment 6 Kent Tamura 2012-03-19 01:32:14 PDT
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.
Comment 7 yosin 2012-03-19 01:47:31 PDT
Created attachment 132558 [details]
Patch 2
Comment 8 Kent Tamura 2012-03-19 02:01:46 PDT
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.
Comment 9 yosin 2012-03-20 18:17:42 PDT
Created attachment 132942 [details]
Patch 3
Comment 10 Kent Tamura 2012-03-20 20:27:01 PDT
Comment on attachment 132942 [details]
Patch 3

ok
Comment 11 WebKit Review Bot 2012-03-20 21:46:16 PDT
Comment on attachment 132942 [details]
Patch 3

Clearing flags on attachment: 132942

Committed r111497: <http://trac.webkit.org/changeset/111497>
Comment 12 WebKit Review Bot 2012-03-20 21:46:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Csaba Osztrogonác 2012-03-21 06:05:12 PDT
Reopen, because it broke 2 tests on all platform:
 - fast/css/text-transform-select.html
 - fast/forms/select/menulist-disabled-option.html
Comment 14 Csaba Osztrogonác 2012-03-21 06:17:25 PDT
(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.
Comment 15 yosin 2012-03-21 19:29:04 PDT
(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;
Comment 16 Kent Tamura 2012-03-21 19:55:03 PDT
(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?
Comment 17 yosin 2012-03-21 23:47:19 PDT
> > > > 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. (><)
Comment 18 yosin 2012-03-21 23:50:44 PDT
Created attachment 133196 [details]
Patch 4
Comment 19 WebKit Review Bot 2012-03-22 01:04:27 PDT
Comment on attachment 133196 [details]
Patch 4

Clearing flags on attachment: 133196

Committed r111659: <http://trac.webkit.org/changeset/111659>
Comment 20 WebKit Review Bot 2012-03-22 01:04:33 PDT
All reviewed patches have been landed.  Closing bug.