Bug 115710 – REGRESSION: Disabled multiline select element now responds to (certain) clicks

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryosuke Niwa]

Disabled select element should not fire onchange event

[Ryosuke Niwa]

Created attachment 200870 [details]
Fixes the bug

[Ryosuke Niwa]

Merge https://chromium.googlesource.com/chromium/blink/+/ac9ec3aa697055ddee506e00ca18fbfda7b946ea

[Alexey Proskuryakov]

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 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]

(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]

<rdar://problem/13824045>

[Yael]

(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]

Created attachment 200986 [details]
Patch

[Ryosuke Niwa]

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 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]

(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]

(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]

Created attachment 200990 [details]
Updated patch

Updated the tests

[Yael]

Created attachment 201062 [details]
Patch

Update to match https://src.chromium.org/viewvc/blink?view=rev&revision=149951

[Ryosuke Niwa]

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]

(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 on attachment 201062 [details]
Patch

ok

[WebKit Commit Bot]

Comment on attachment 201062 [details]
Patch

Clearing flags on attachment: 201062

Committed r149856: <http://trac.webkit.org/changeset/149856>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.