Select when disabled should not allow value change and popup is disabled

Do not change the value of a disabled SELECT element.

parseAttributes closes popup when the element becomes disabled.
    
We need to add some isDisabledFormControl checks to
menuListDefaultEventHandler because user events can be delivered to
disabled elements.

R=tkent
BUG=241079
TEST=automated

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167563

[Habib Virji, 2014-02-11 14:05:04]

Select when disabled should not allow value change
    
A check if current element is select and disabled, should not allow value of the
select element to be changed. isDisabledFormControl is called by RenderMenuList
when selecting a element.
    
ContainerNode is used to derive if current node is a select tag, as color and
date picker both call isDisabledFormControl.
    
Tests that select element when disabled, value cannot be changed.

[tkent, 2014-02-12 08:15:43]

https://codereview.chromium.org/159743002/diff/1/Source/core/html/HTMLOptionE...
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/159743002/diff/1/Source/core/html/HTMLOptionE...
Source/core/html/HTMLOptionElement.cpp:335: status =
ownerSelectElement()->isDisabledOrReadOnly();
We can't do this change.  This function should match to the HTML specification.

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-idioms.htm...
> option elements that are disabled

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-elemen...
> The disabled attribute is a boolean attribute. An option element is disabled
if its disabled attribute is present or if it is a child of an optgroup element
whose disabled attribute is present.

An option in <select disabled><option>...</select> is NOT disabled according to
the specification.

[tkent, 2014-02-12 08:17:49]

> BUG=241079

The bug number is wrong.

[tkent, 2014-02-12 08:18:28]

On 2014/02/12 08:17:49, tkent wrote:
> > BUG=241079
> 
> The bug number is wrong.

My bad.  It was correct.

[Habib Virji, 2014-02-12 19:31:34]

Is  then bug invalid?

[tkent, 2014-02-14 00:06:44]

On 2014/02/12 19:31:34, Habib Virji wrote:
> Is  then bug invalid?

The bug is valid.
- The test in this CL tests with listbox <select>, but the bug is about menulist
<select>.
- The proper fix is that we should close the popup menu when the <select> gets
disabled.

[Habib Virji, 2014-02-14 16:55:15]

On 2014/02/14 00:06:44, tkent wrote:
> On 2014/02/12 19:31:34, Habib Virji wrote:
> > Is  then bug invalid?
> 
> The bug is valid.
> - The test in this CL tests with listbox <select>, but the bug is about
menulist
> <select>.
> - The proper fix is that we should close the popup menu when the <select> gets
> disabled.

Thanks for the comments, have uploaded new patch which remove previous changes
and close popup when select element is disabled.

[tkent, 2014-02-17 00:14:30]

https://codereview.chromium.org/159743002/diff/140001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/select/select-disabled.html (right):

https://codereview.chromium.org/159743002/diff/140001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/select-disabled.html:59: select3.blur();
blur() closes the popup.  So this test makes no sense.

https://codereview.chromium.org/159743002/diff/140001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/159743002/diff/140001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1065: if (!isDisabledOrReadOnly())
* <select> can not be read-only.

* This check has no effect.  If the SELECT element is disabled, it can't be
focused and any events are not delivered to the SELECT element.
As I already wrote, we need to close the popup when the SELECT gets disabled. 
That is to say, we need to update parseAttribute() implementation so that we can
know 'disabled' attribute change.

[Habib Virji, 2014-02-18 10:23:21]

parseAttributes handles closing of popup if visible and setting validity flag
for FormControl.
    
isDisabledFormControl() is required for rendering drop down as disable and for
various local checks. 

HTMLFormControlElement::isDisabledFormControl was used previously which did not
check select disable attribute checking.
    
menuListDefaultEventHandler has isDisabledFormControl check due to threading
issue. Both event thread and javascript thread are called simultaneously. 

After defaultEventHandler initiates call to menuListener, it appears
parseAttribute is called by JS thread. Resulting in showing of popup. A check
just before showing popup gets the correct state for the menuList. 
    
Test updated with select on disable, hides the popup and value cannot be
changed.

https://codereview.chromium.org/159743002/diff/140001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/select/select-disabled.html (right):

https://codereview.chromium.org/159743002/diff/140001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/select/select-disabled.html:59: select3.blur();
On 2014/02/17 00:14:30, tkent wrote:
> blur() closes the popup.  So this test makes no sense.

Done. Removed blur.

https://codereview.chromium.org/159743002/diff/140001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/159743002/diff/140001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1065: if (!isDisabledOrReadOnly())
On 2014/02/17 00:14:30, tkent wrote:
> * <select> can not be read-only.
> 
> * This check has no effect.  If the SELECT element is disabled, it can't be
> focused and any events are not delivered to the SELECT element.
> As I already wrote, we need to close the popup when the SELECT gets disabled. 
> That is to say, we need to update parseAttribute() implementation so that we
can
> know 'disabled' attribute change.

Done.
"<select> can not be read-only."
--> Select now only checks isDisabledFormControl.
 
* "This check has no effect.  If the SELECT element is disabled, it can't be
 focused and any events are not delivered to the SELECT element."
--> That's true. But found that both event handler and attribute setting threads
are called simultaneously resulting in menuListHandler being invoked before
parseAttribute is completed. A check is added to ensure a proper disable
attribute checking before showing popup.

"As I already wrote, we need to close the popup when the SELECT gets disabled."
--> Added in parseAttribute.

"That is to say, we need to update parseAttribute() implementation so that we
can know 'disabled' attribute change."
--> Done.

[tkent, 2014-02-19 04:17:51]

Correction: If isDisabledFormControl() returns true, only mouse events are not
delivered to the element.  It will receive keyboard events.

> menuListDefaultEventHandler has isDisabledFormControl check due to threading
issue. Both event thread and javascript thread are called simultaneously. 

They are in a single thread, and we never run them concurrently.  However, it
might be possible that events are already in an event queue when the element
becomes disabled.

https://codereview.chromium.org/159743002/diff/240001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/159743002/diff/240001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:306: setNeedsWillValidateCheck();
You should call HTMLFormControlElemenetWithState::parseAttribute(name, value)
here.

https://codereview.chromium.org/159743002/diff/240001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:318: bool
HTMLSelectElement::isDisabledFormControl() const
You should not override isDisabledFormControl.  This will break <fieldset
disabled><select>...</fieldset>

https://codereview.chromium.org/159743002/diff/240001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1079: if (!isDisabledFormControl())
We had better check isDisabledFormControl earlier.
We have some checks like below:
  if (!renderer() || ...)
      return;
We can change them to:
  if (!renderer() || ... || isDisabledFormControl())
       return;

[Habib Virji, 2014-02-19 08:26:43]

Updated as per review comments.

https://codereview.chromium.org/159743002/diff/240001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/159743002/diff/240001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:306: setNeedsWillValidateCheck();
On 2014/02/19 04:17:51, tkent wrote:
> You should call HTMLFormControlElemenetWithState::parseAttribute(name, value)
> here.

Done.

https://codereview.chromium.org/159743002/diff/240001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:318: bool
HTMLSelectElement::isDisabledFormControl() const
On 2014/02/19 04:17:51, tkent wrote:
> You should not override isDisabledFormControl.  This will break <fieldset
> disabled><select>...</fieldset>

Done.

https://codereview.chromium.org/159743002/diff/240001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1079: if (!isDisabledFormControl())
On 2014/02/19 04:17:51, tkent wrote:
> We had better check isDisabledFormControl earlier.
> We have some checks like below:
>   if (!renderer() || ...)
>       return;
> We can change them to:
>   if (!renderer() || ... || isDisabledFormControl())
>        return;

Done.

[tkent, 2014-02-19 14:05:03]

The test failed on Mac.
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/19...

[Habib Virji, 2014-02-19 15:27:27]

On 2014/02/19 14:05:03, tkent wrote:
> The test failed on Mac.
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/19...

I have updated test by separating popup tests and value change tests. I have
tried using instead of focus, clickElement as used in
LayoutTests/platform/linux/fast/forms/select/popup-closes-on-blur-expected.txt. 

isSelectPopupVisible does not seem to work on mac as can be seen in
LayoutTests/fast/forms/select/popup-closes-on-blur-expected.txt. There is
different expected file in
LayoutTests/platform/linux/fast/forms/select/popup-closes-on-blur-expected.txt.

Unfortunately I do not have mac that's why wanted to get permission for try bot
server.

[tkent, 2014-02-21 00:56:16]

lgtm.

> Select disable attribute handling in parseAttribute

We should summarize what this CL changes in the first line of the CL
description.  If the CL changes a behavior, the behavior change should be
summarized.

I'd say "Do not change SELECT value if the SELECT element is disabled" or "Fix a
bug that users can update the value of a disabled SELECT."

I'll update the CL description, and send this to CQ.

[tkent, 2014-02-21 01:01:01]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-02-21 01:01:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/159743002/440001

[commit-bot: I haz the power, 2014-02-21 03:09:26]

Change committed as 167563

[Habib Virji, 2014-02-21 09:20:25]

On 2014/02/21 00:56:16, tkent wrote:
> lgtm.
> 
> > Select disable attribute handling in parseAttribute
> 
> We should summarize what this CL changes in the first line of the CL
> description.  If the CL changes a behavior, the behavior change should be
> summarized.
> 
> I'd say "Do not change SELECT value if the SELECT element is disabled" or "Fix
a
> bug that users can update the value of a disabled SELECT."
> 
> I'll update the CL description, and send this to CQ.

Thank tkent, will keep in mind for other issues. 
"Select disable attribute handling in parseAttribute" was actually my commit
line.