Bug 100932 – User can change a disabled select (drop down box)

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kunihiko Sakamoto]

Imported from http://crbug.com/158802

Focused select element is changeable even if it is disabled.
Example URL: http://jsfiddle.net/langdonx/ymEZR/

If focused element becomes disabled, the element should lose focus. This is expected behavior within HTML5 spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#focus-management

[Kunihiko Sakamoto]

Created attachment 171801 [details]
Patch

[Kent Tamura]

Comment on attachment 171801 [details]
Patch

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

> Source/WebCore/html/HTMLFormControlElement.cpp:154
> +    if (disabled() && focused())
> +        blur();
>      setNeedsWillValidateCheck();

Unfortunately, this is not acceptable.  blur() dispatches a 'blur' event, and Event handler JavaScript code can delete |this| object.

Probably we should make blur/focusout/DOMFocusOut/focus/focusin/DOMFocusIn events scoped-events, like I did in http://trac.webkit.org/changeset/132983

[Alexey Proskuryakov]

When fixing this, please also add a test case for the issue Kent found in review.

[Kunihiko Sakamoto]

Created attachment 172905 [details]
Patch 2

[Kent Tamura]

Comment on attachment 172905 [details]
Patch 2

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

> Source/WebCore/ChangeLog:15
> +        (WebCore::HTMLSelectElement::menuListDefaultEventHandler):
> +        (WebCore::HTMLSelectElement::listBoxDefaultEventHandler):
> +        (WebCore::HTMLSelectElement::defaultEventHandler):

You changes are correct, but I think checking disabled() in the beginning of HTMLSelectElement::defaultEventHandler is enough and simpler.

[Kunihiko Sakamoto]

Created attachment 172911 [details]
Patch 3

[Kunihiko Sakamoto]

Comment on attachment 172905 [details]
Patch 2

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

>> Source/WebCore/ChangeLog:15
>> +        (WebCore::HTMLSelectElement::defaultEventHandler):
> 
> You changes are correct, but I think checking disabled() in the beginning of HTMLSelectElement::defaultEventHandler is enough and simpler.

Done, thanks.

[Kent Tamura]

Comment on attachment 172911 [details]
Patch 3

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

> Source/WebCore/html/HTMLSelectElement.cpp:1443
> +    if (disabled() && (event->type() == eventNames().keydownEvent || event->type() == eventNames().keypressEvent)) {

if (disabled()) {
should be enough.

[Kunihiko Sakamoto]

Created attachment 172918 [details]
Patch 4

[Kunihiko Sakamoto]

Comment on attachment 172911 [details]
Patch 3

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

>> Source/WebCore/html/HTMLSelectElement.cpp:1443
>> +    if (disabled() && (event->type() == eventNames().keydownEvent || event->type() == eventNames().keypressEvent)) {
> 
> if (disabled()) {
> should be enough.

Done.

[Kent Tamura]

Comment on attachment 172918 [details]
Patch 4

ok

[WebKit Review Bot]

Comment on attachment 172918 [details]
Patch 4

Rejecting attachment 172918 [details] from commit-queue.

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Full output: http://queues.webkit.org/results/14766341

[WebKit Review Bot]

Comment on attachment 172918 [details]
Patch 4

Clearing flags on attachment: 172918

Committed r133860: <http://trac.webkit.org/changeset/133860>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.