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
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.
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.
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.
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
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 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 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.
(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 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.
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!
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.
Comment on attachment 144499 [details] Patch 7 Looks ok.
Comment on attachment 144499 [details] Patch 7 Clearing flags on attachment: 144499 Committed r118772: <http://trac.webkit.org/changeset/118772>
All reviewed patches have been landed. Closing bug.
<rdar://problem/11548057>
(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