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

Ryosuke Niwa
Reported 2013-05-06 21:47:28 PDT
Disabled select element should not fire onchange event
Attachments
Fixes the bug (4.30 KB, patch)
2013-05-06 21:52 PDT, Ryosuke Niwa
tkent: review-
Patch (4.92 KB, patch)
2013-05-07 15:24 PDT, Yael
no flags
Updated patch (4.97 KB, patch)
2013-05-07 16:21 PDT, Yael
no flags
Patch (6.84 KB, patch)
2013-05-08 05:36 PDT, Yael
no flags
Ryosuke Niwa
Comment 1 2013-05-06 21:52:22 PDT
Created attachment 200870 [details] Fixes the bug
Alexey Proskuryakov
Comment 3 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?
Kent Tamura
Comment 4 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.
Ryosuke Niwa
Comment 5 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.
Radar WebKit Bug Importer
Comment 6 2013-05-06 22:24:12 PDT
Yael
Comment 7 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.
Yael
Comment 8 2013-05-07 15:24:08 PDT
Ryosuke Niwa
Comment 9 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?
Ryosuke Niwa
Comment 10 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.
Yael
Comment 11 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.
Yael
Comment 12 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.
Yael
Comment 13 2013-05-07 16:21:11 PDT
Created attachment 200990 [details] Updated patch Updated the tests
Ryosuke Niwa
Comment 15 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?
Yael
Comment 16 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.
Kent Tamura
Comment 17 2013-05-09 15:38:54 PDT
Comment on attachment 201062 [details] Patch ok
WebKit Commit Bot
Comment 18 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>
WebKit Commit Bot
Comment 19 2013-05-09 16:54:47 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.