Bug 115710

Summary: REGRESSION: Disabled multiline select element now responds to (certain) clicks
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: New BugsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, darin, simon.fraser, tkent, webkit-bug-importer, yael
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fixes the bug
tkent: review-
Patch
none
Updated patch
none
Patch none

Description Ryosuke Niwa 2013-05-06 21:47:28 PDT
Disabled select element should not fire onchange event
Comment 1 Ryosuke Niwa 2013-05-06 21:52:22 PDT
Created attachment 200870 [details]
Fixes the bug
Comment 3 Alexey Proskuryakov 2013-05-06 22:04:34 PDT
It looks like a regression from Safari 6.0.4 that the disabled element changes at all.

When did this behavior change?
Comment 4 Kent Tamura 2013-05-06 22:15:10 PDT
Comment on attachment 200870 [details]
Fixes the bug

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

> Source/WebCore/html/HTMLSelectElement.cpp:640
>  void HTMLSelectElement::listBoxOnChange()
>  {
>      ASSERT(!usesMenuList() || m_multiple);
> +    if (isDisabledFormControl())
> +        return;

This just disables event dispatching. The root cause is that selection state is updated even though the <select> is disabled.
Comment 5 Ryosuke Niwa 2013-05-06 22:23:14 PDT
(In reply to comment #4)
> (From update of attachment 200870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200870&action=review
> 
> > Source/WebCore/html/HTMLSelectElement.cpp:640
> >  void HTMLSelectElement::listBoxOnChange()
> >  {
> >      ASSERT(!usesMenuList() || m_multiple);
> > +    if (isDisabledFormControl())
> > +        return;
> 
> This just disables event dispatching. The root cause is that selection state is updated even though the <select> is disabled.

That’s what I thought :/. Apparently blink reviewer needs to be more careful.
Comment 6 Radar WebKit Bug Importer 2013-05-06 22:24:12 PDT
<rdar://problem/13824045>
Comment 7 Yael 2013-05-07 09:30:48 PDT
(In reply to comment #3)
> It looks like a regression from Safari 6.0.4 that the disabled element changes at all.
> 
> When did this behavior change?

I think http://trac.webkit.org/changeset/140286 introduced this regression.
Sorry for the incomplete solution, I will try to rework it today.
Comment 8 Yael 2013-05-07 15:24:08 PDT
Created attachment 200986 [details]
Patch
Comment 9 Ryosuke Niwa 2013-05-07 15:31:08 PDT
Comment on attachment 200986 [details]
Patch

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

> Source/WebCore/html/HTMLSelectElement.cpp:640
> +    if (isDisabledFormControl())
> +        return;

Do we really need this change still?
Comment 10 Ryosuke Niwa 2013-05-07 15:32:31 PDT
Comment on attachment 200986 [details]
Patch

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

> LayoutTests/fast/forms/select/listbox-disabled-scroll-no-onchange.html:9
> +<p id="description"></p>
> +<select id="test" disabled size=3">

Please use the modified test from my patch. This test has a lot of bugs and stylistic issues.
Comment 11 Yael 2013-05-07 15:38:25 PDT
(In reply to comment #9)
> (From update of attachment 200986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200986&action=review
> 
> > Source/WebCore/html/HTMLSelectElement.cpp:640
> > +    if (isDisabledFormControl())
> > +        return;
> 
> Do we really need this change still?

Yes, we do.
Without this, HTMLSelectElement::listBoxOnChange() will generate a onchange event, because m_lastOnChangeSelection is empty.
Comment 12 Yael 2013-05-07 15:59:02 PDT
(In reply to comment #10)
> (From update of attachment 200986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200986&action=review
> 
> > LayoutTests/fast/forms/select/listbox-disabled-scroll-no-onchange.html:9
> > +<p id="description"></p>
> > +<select id="test" disabled size=3">
> 
> Please use the modified test from my patch. This test has a lot of bugs and stylistic issues.
Oops, I did not realize that the testing style diverged so much already . Will do.
Comment 13 Yael 2013-05-07 16:21:11 PDT
Created attachment 200990 [details]
Updated patch

Updated the tests
Comment 15 Ryosuke Niwa 2013-05-08 08:39:26 PDT
Comment on attachment 201062 [details]
Patch

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

> LayoutTests/fast/forms/select/listbox-disabled-no-autoscroll.html:23
> +if (window.testRunner) {
> +    testRunner.waitUntilDone();
> +}

No curly brackets around a single line statement.

> LayoutTests/fast/forms/select/listbox-disabled-no-autoscroll.html:34
> +    }, 100);

Why do we need to wait 100ms? Are we waiting for a repaint?
Comment 16 Yael 2013-05-08 09:15:43 PDT
(In reply to comment #15)
> (From update of attachment 201062 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201062&action=review
> 
> > LayoutTests/fast/forms/select/listbox-disabled-no-autoscroll.html:23
> > +if (window.testRunner) {
> > +    testRunner.waitUntilDone();
> > +}
> 
> No curly brackets around a single line statement.
> 
> > LayoutTests/fast/forms/select/listbox-disabled-no-autoscroll.html:34
> > +    }, 100);
> 
> Why do we need to wait 100ms? Are we waiting for a repaint?
We are waiting for the autoscroll timer to fire.
Comment 17 Kent Tamura 2013-05-09 15:38:54 PDT
Comment on attachment 201062 [details]
Patch

ok
Comment 18 WebKit Commit Bot 2013-05-09 16:54:43 PDT
Comment on attachment 201062 [details]
Patch

Clearing flags on attachment: 201062

Committed r149856: <http://trac.webkit.org/changeset/149856>
Comment 19 WebKit Commit Bot 2013-05-09 16:54:47 PDT
All reviewed patches have been landed.  Closing bug.