Bug 24900 – Render empty optgroup elements.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Langley]

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.

[Adam Langley]

Created attachment 29025 [details]
patch

[Adam Langley]

Comment on attachment 29025 [details]
patch

tc points out that one might be able to open the select box using window.eventSender.

[Adam Langley]

Created attachment 29027 [details]
patch

[Adam Langley]

Even with eventSender the pop-up is, of course, a different top level window and so isn't included in the pixel tests anyway.

[Darin Adler]

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?

[Adam Langley]

Created attachment 29198 [details]
patch

[Adam Langley]

> 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.

[Darin Adler]

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?

[Adam Langley]

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

[Oliver Hunt]

Comment on attachment 29198 [details]
patch

I would think hyatt should review this rather than darin

[Eric Seidel (no email)]

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.

[Adam Langley]

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.

[Eric Seidel (no email)]

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.

[Adam Langley]

Created attachment 29833 [details]
patch

Have updated the comment.

[Eric Seidel (no email)]

Comment on attachment 29833 [details]
patch

LGTM.

[Eric Seidel (no email)]

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