Render empty optgroup elements. Currently, optgroup elements which are empty are not rendered. The HTML specification gives no guidance on this situation. However, the test for being empty is that they have no children, thus this will not render: <optgroup label="test"></optgroup> while this /will/ render (because of the text node child): <optgroup label="test"> </optgroup> This patch will cause both cases to render which matches IE's and Firefox's behaviour. The difference only appears when opening the select element, does not appear in the render tree and there's no way to open a select from Javascript. Thus, a manual layout-test is required.
Created attachment 29025 [details] patch
Comment on attachment 29025 [details] patch tc points out that one might be able to open the select box using window.eventSender.
Created attachment 29027 [details] patch
Even with eventSender the pop-up is, of course, a different top level window and so isn't included in the pixel tests anyway.
Comment on attachment 29027 [details] patch > - if (current->hasTagName(optgroupTag) && current->firstChild()) { > - // FIXME: It doesn't make sense to add an optgroup to the list items, > - // when it has children, but not to add it if it happens to have, > - // children (say some comment nodes or text nodes), yet that's what > - // this code does! > + // optgroup tags may not nest. However, both FireFox and IE will > + // flatten the tree automatically, so we follow suit. > + // (http://www.w3.org/TR/html401/interact/forms.html#h-17.6) > + if (current->hasTagName(optgroupTag)) { > m_listItems.append(static_cast<HTMLElement*>(current)); > - current = current->firstChild(); > - // FIXME: It doesn't make sense to handle an <optgroup> inside another <optgroup> > - // if it's not the first child, but not handle it if it happens to be the first > - // child, yet that's what this code does! > + if (current->firstChild()) > + current = current->firstChild(); > } This doesn't look right. What if current->firstChild() is another optgroup?
Created attachment 29198 [details] patch
> This doesn't look right. What if current->firstChild() is another optgroup? You're perfectly correct; it would skip in optgroup immediately inside another. Thanks. Patch updated: moved updating current from the for statement to the bottom of the loop so that a continue can jump to the start without updating. Added an <optgroup><optgroup> to the test.
Comment on attachment 29198 [details] patch The code change looks fine, but where's the test case that would have failed with the old patch?
(sorry about the delay, didn't get the email for some reason). Since the drop-down doesn't appear in the render tree nor pixel test, it's a manual test I'm afraid. In the manual test there's "Nested group 3" directly inside "Nested group 2". Previously, group 3 would have been ignored.
Comment on attachment 29198 [details] patch I would think hyatt should review this rather than darin
Comment on attachment 29198 [details] patch I talked with Adam about this in person. Firefox and IE both allow optgroups inside a <div> element inside the select (i.e. not as direct children to the select), so we should support tha too. Adam also decided to throw some other crazy tests at it and see what IE and FF will do. He's gonna fix/document those differences and upload a new patch. Otherwise this looked fine.
Created attachment 29559 [details] patch After chatting with Eric, I updated the manual test to check a number of other corner cases. In order to parse inside a <div> it appeared that |traverseNextSibling| should be changed to |traverseNextNode|. However, the <div> is removed from the tree before this code runs, so it doesn't actually need any changes to work.
Comment on attachment 29559 [details] patch + // Odd tags like <div> will already have been removed from the tree. + current = current->traverseNextSibling(this); This needs a "so what?" explanation. // We traverse nextSibling instead of nextNode because... why? Otherwise this looks fine. If you update that comment you're good to land IMO. If you post an updated comment I'm happy to land this for you.
Created attachment 29833 [details] patch Have updated the comment.
Comment on attachment 29833 [details] patch LGTM.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/html/HTMLSelectElement.cpp A WebCore/manual-tests/optgroup-empty-and-nested.html Committed r43337