Bug 87614

Summary: REGRESSION(r109729): The optgroup element's "disabled" attribute has no effect to rendering and selection
Product: WebKit Reporter: yosin
Component: FormsAssignee: yosin
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dglazkov, jberlin, macpherson, menard, mitz, slewis, tkent, webkit-bug-importer, webkit.review.bot
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://jsfiddle.net/X7Tpj/
Bug Depends on:    
Bug Blocks: 87748    
Attachments:
Description Flags
Patch 1
none
Patch 2
none
Patch 3
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch 4
none
Patch 5
none
Patch 6
none
Patch 7 none

Description yosin 2012-05-27 21:20:55 PDT
It seems "disabled" attribute in the "optgroup" element does nothing.
See attached URI.

* FireFox: options are disabled with visual effect. We can't select them
* IE: options are disabled with visual effect. We can't select them
* Opera: options are disabled with visual effect. We can't select them
Comment 1 Kent Tamura 2012-05-27 21:34:34 PDT
Safari 5.1.5 doesn't have this issue though WebKit nightly has.
Comment 2 yosin 2012-05-28 01:50:11 PDT
Created attachment 144306 [details]
Patch 1
Comment 3 yosin 2012-05-28 02:06:43 PDT
Note: This doesn't fix rendering performance of many optgroup elements (BUG-874666).
Comment 4 yosin 2012-05-28 02:17:11 PDT
Comment on attachment 144306 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 5 Kent Tamura 2012-05-28 02:19:57 PDT
Can you identify what revision made the regression and rename this bug to "REGRESSION(rNNNNN): ..." please?  It would be helpful for release engineering.
Comment 6 yosin 2012-05-28 02:22:57 PDT
r109729 made this regression.
Comment 7 yosin 2012-05-28 02:24:09 PDT
Created attachment 144309 [details]
Patch 2
Comment 8 yosin 2012-05-28 02:24:58 PDT
Comment on attachment 144309 [details]
Patch 2

Update ChangeLog files to add revision number which caused regression.

Could you review again?
Thanks in advance.
Comment 9 Kent Tamura 2012-05-28 02:38:27 PDT
Comment on attachment 144309 [details]
Patch 2

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

> Source/WebCore/html/HTMLOptGroupElement.cpp:56
> +    if (m_disabled)

m_disabled is not needed. fastHasAttribute(disabledAttr) is enough.

> Source/WebCore/html/HTMLOptGroupElement.cpp:97
> -    HTMLElement::parseAttribute(attribute);
> +    if (attribute.name() == disabledAttr) {
> +        bool oldDisabled = m_disabled;
> +        m_disabled = !attribute.isNull();
> +        if (oldDisabled != m_disabled) {
> +            setNeedsStyleRecalc();
> +            if (renderer() && renderer()->style()->hasAppearance())
> +                renderer()->theme()->stateChanged(renderer(), EnabledState);
> +        }
> +    } else
> +        HTMLElement::parseAttribute(attribute);

This change is not needed because <optgroup> never has a renderer, and appearance update by disabled change is handled in recalcSelectOptions().


> Source/WebCore/html/HTMLOptGroupElement.h:63
> +    bool m_disabled;

m_disabled is not needed.
Comment 10 yosin 2012-05-28 02:46:50 PDT
Created attachment 144312 [details]
Patch 3
Comment 11 yosin 2012-05-28 02:48:07 PDT
Comment on attachment 144312 [details]
Patch 3

* Changed HTMLOptGroupElement::disabled to use fastHasAttribute.
* Drop HTMLOptGroupElement::m_disabled

Could you review again?
Thanks in advance.
Comment 12 WebKit Review Bot 2012-05-28 02:59:40 PDT
Comment on attachment 144312 [details]
Patch 3

Attachment 144312 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12844175

New failing tests:
compositing/geometry/clipping-foreground.html
Comment 13 WebKit Review Bot 2012-05-28 02:59:44 PDT
Created attachment 144315 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 14 Darin Adler 2012-05-28 03:01:25 PDT
Comment on attachment 144312 [details]
Patch 3

I think this is OK.

But the patch puts the HTMLOptGroupElement into a slightly confusing state. The DOM attribute named disabled will reflect only the status of the content attribute on the element itself. But the C++ disabled function reflects the disabled state of the select element as well.

Is this the correct behavior for the DOM getter? If it is, then we should follow up this patch with a name change for the virtual function in the class Node to eliminate this name conflict between the disabled DOM attribute and the internal function we have named disabled that is used for CSS style matching. If it is not, then we need an additional code change for correctness.

There are a few different things we need to test:

1) That "disabled" CSS style matching works correctly; I believe this is decided by code that calls Node::disabled. That's the only thing that the regression test here covers.

2) That the item is treated as disabled in the form user interface; I believe this is done by code that calls isEnabledFormControl. It would be good to have a test that covers this.

3) That the item is treated as disabled when trying to select it explicitly with DOM code. I believe this is also done with isEnabledFormControl. Again, would be good to test this.

4) That the item returns true from the DOM disabled attribute. This is currently handled in the IDL with the [Reflect] attribute, but if that was removed then it would call HTMLOptGroupElement::disabled. It would be good to have a test case that covers this.
Comment 15 Kent Tamura 2012-05-28 03:22:51 PDT
Comment on attachment 144312 [details]
Patch 3

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

> Source/WebCore/html/HTMLOptGroupElement.cpp:58
> +
> +    HTMLSelectElement* select = ownerSelectElement();
> +    return select && select->disabled();

Do we need this?  Is this compatible with other browsers?

The specification says:

> :enabled
>     ...
>     optgroup elements that do not have a disabled attribute
Comment 16 yosin 2012-05-28 22:22:04 PDT
Created attachment 144445 [details]
Patch 4
Comment 17 yosin 2012-05-28 22:44:57 PDT
Comment on attachment 144445 [details]
Patch 4

* Change HTMLOptGroupElement::disabled to check only DOM attribute "disabled".
* Add HTMLOptGroupElement::isEnabledFormControl for CSS pseudo-class :disabled/:enabled
* Update SelectorChecker for pseudo-class :disabled/:enabled to check the "optgroup" element.

Could you review again?
Thanks in advance.
Comment 18 Kent Tamura 2012-05-28 23:04:14 PDT
(In reply to comment #17)
> (From update of attachment 144445 [details])
> * Change HTMLOptGroupElement::disabled to check only DOM attribute "disabled".
> * Add HTMLOptGroupElement::isEnabledFormControl for CSS pseudo-class :disabled/:enabled
> * Update SelectorChecker for pseudo-class :disabled/:enabled to check the "optgroup" element.
> 
> Could you review again?
> Thanks in advance.

Would you add
 - a rendering test for <optgroup disabled>
 - a selection test by clicking an <option> in a <optgroup disabled>
please? (2 and 3 of Darin's comment)
Comment 19 Kent Tamura 2012-05-28 23:04:38 PDT
Comment on attachment 144445 [details]
Patch 4

r- because of test coverage
Comment 20 yosin 2012-05-29 01:15:37 PDT
Created attachment 144471 [details]
Patch 5
Comment 21 yosin 2012-05-29 01:29:58 PDT
Comment on attachment 144471 [details]
Patch 5

* Add rendering test
* Add DOM select test
* Add mouse click for listbox test
* Add keyboard for menulist test

Could you review again?
Thanks in advance.
Comment 22 Kent Tamura 2012-05-29 01:44:30 PDT
Comment on attachment 144471 [details]
Patch 5

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

> LayoutTests/ChangeLog:12
> +        * fast/forms/select/optgroup-rendering.html: Added.

Please add the following line to platform/chromium/test_expectations.txt

BUGWK87614 : fast/forms/select/optgroup-rendering.html = FAIL MISSING

> LayoutTests/fast/forms/select/optgroup-clicking-expected.txt:11
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> +
> +
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +
> +Click enabled option
> +PASS $("listbox").selectedIndex is 1

'TEST COMPLETE' then showing tests is wrong.  Please follow the comments below.

> LayoutTests/fast/forms/select/optgroup-clicking.html:47
> +window.jsTestIsAsync;

This makes no sense.
It should be:
    window.jsTestIsAsync = true;

> LayoutTests/fast/forms/select/optgroup-clicking.html:52
> +    if (!window.layoutTestController)
> +        return;

You need to show a message.  'This test needs layoutTestController.' is ok.

> LayoutTests/fast/forms/select/optgroup-clicking.html:75
> +    shouldBe('$("menulist").selectedIndex', '8');
> +}

You should add finishJSTest();.

> LayoutTests/fast/forms/select/optgroup-clicking.html:77
> +<script src="../../js/resources/js-test-post-async.js"></script>

You should load js-test-post.js.

> LayoutTests/fast/forms/select/optgroup-rendering.html:6
> +    optbox:disabled { color: pink; }
> +    optbox:enabled { color: green; }

They should optgroup:disabled and optgroup:enabled, right?
Comment 23 yosin 2012-05-29 02:04:13 PDT
Created attachment 144483 [details]
Patch 6
Comment 24 yosin 2012-05-29 02:07:34 PDT
Comment on attachment 144483 [details]
Patch 6

* Fix typo in optgroup-rendering.html
* Fix bogus things in optgroup-click.html
* Add MISSING entry to platform/chromium/test_expectations.txt

Could you review again?
Thanks in advance!
Comment 25 Kent Tamura 2012-05-29 02:41:19 PDT
Comment on attachment 144483 [details]
Patch 6

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

> LayoutTests/ChangeLog:14
> +        * platform/chromium-linux/fast/forms/select/optgroup-rendering-expected.png: Added.

The image looks wrong.  Would you add a correct image to avoid confusion for WebKit gardeners please?

> LayoutTests/platform/chromium/test_expectations.txt:3776
>  BUGWK87671 : fast/layers/clip-rects-assertion-expected.txt = MISSING
> +
> +BUGWK87614 : fast/forms/select/optgroup-rendering.html = FAIL MISSING
> +

I recommend add an entry at not the end of file to avoid a conflict.
Comment 26 yosin 2012-05-29 03:05:31 PDT
Created attachment 144499 [details]
Patch 7
Comment 27 yosin 2012-05-29 03:18:10 PDT
Comment on attachment 144499 [details]
Patch 7

* Reorder test_expectation.txt to avoid patch failure
* Update image
** We'll update image due by BUG-87719 "option" element regression

Could you review again?
Thanks in advance.
Comment 28 Kent Tamura 2012-05-29 03:19:57 PDT
Comment on attachment 144499 [details]
Patch 7

Looks ok.
Comment 29 WebKit Review Bot 2012-05-29 07:00:54 PDT
Comment on attachment 144499 [details]
Patch 7

Clearing flags on attachment: 144499

Committed r118772: <http://trac.webkit.org/changeset/118772>
Comment 30 WebKit Review Bot 2012-05-29 07:01:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Radar WebKit Bug Importer 2012-05-29 08:27:38 PDT
<rdar://problem/11548057>
Comment 32 Jessie Berlin 2012-05-29 08:40:14 PDT
(In reply to comment #29)
> (From update of attachment 144499 [details])
> Clearing flags on attachment: 144499
> 
> Committed r118772: <http://trac.webkit.org/changeset/118772>

You did not add expectations for the Mac or Windows ports for fast/forms/select/optgroup-rendering.html, and that is making the bots red:

http://build.webkit.org/results/Lion%20Debug%20(Tests)/r118772%20(7161)/results.html