Bug 87614 – REGRESSION(r109729): The optgroup element's "disabled" attribute has no effect to rendering and selection

• Component: Forms
• Status: RESOLVED FIXED

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

[Kent Tamura]

Safari 5.1.5 doesn't have this issue though WebKit nightly has.

Created attachment 144306 [details]
Patch 1

Note: This doesn't fix rendering performance of many optgroup elements (BUG-874666).

Comment on attachment 144306 [details]
Patch 1

Could you review this patch?
Thanks in advance.

[Kent Tamura]

Can you identify what revision made the regression and rename this bug to "REGRESSION(rNNNNN): ..." please?  It would be helpful for release engineering.

r109729 made this regression.

Created attachment 144309 [details]
Patch 2

Comment on attachment 144309 [details]
Patch 2

Update ChangeLog files to add revision number which caused regression.

Could you review again?
Thanks in advance.

[Kent Tamura]

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.

Created attachment 144312 [details]
Patch 3

Comment on attachment 144312 [details]
Patch 3

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

Could you review again?
Thanks in advance.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Darin Adler]

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.

[Kent Tamura]

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

Created attachment 144445 [details]
Patch 4

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.

[Kent Tamura]

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

[Kent Tamura]

Comment on attachment 144445 [details]
Patch 4

r- because of test coverage

Created attachment 144471 [details]
Patch 5

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.

[Kent Tamura]

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?

Created attachment 144483 [details]
Patch 6

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!

[Kent Tamura]

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.

Created attachment 144499 [details]
Patch 7

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.

[Kent Tamura]

Comment on attachment 144499 [details]
Patch 7

Looks ok.

[WebKit Review Bot]

Comment on attachment 144499 [details]
Patch 7

Clearing flags on attachment: 144499

Committed r118772: <http://trac.webkit.org/changeset/118772>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/11548057>

[Jessie Berlin]

(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