Summary: | REGRESSION: Disabled multiline select element now responds to (certain) clicks | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
Component: | New Bugs | Assignee: | 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
Ryosuke Niwa
2013-05-06 21:47:28 PDT
Created attachment 200870 [details]
Fixes the bug
It looks like a regression from Safari 6.0.4 that the disabled element changes at all. When did this behavior change? 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. (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. (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. Created attachment 200986 [details]
Patch
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 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. (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. (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. Created attachment 200990 [details]
Updated patch
Updated the tests
Created attachment 201062 [details] Patch Update to match https://src.chromium.org/viewvc/blink?view=rev&revision=149951 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? (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 on attachment 201062 [details]
Patch
ok
Comment on attachment 201062 [details] Patch Clearing flags on attachment: 201062 Committed r149856: <http://trac.webkit.org/changeset/149856> All reviewed patches have been landed. Closing bug. |