Bug 24900 - Render empty optgroup elements.
Summary: Render empty optgroup elements.
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2009-03-27 14:55 PDT by Adam Langley
Modified: 2009-05-07 00:42 PDT (History)
0 users

See Also:

patch (4.47 KB, patch)
2009-03-27 14:55 PDT, Adam Langley
no flags Details | Formatted Diff | Diff
patch (4.53 KB, patch)
2009-03-27 15:34 PDT, Adam Langley
no flags Details | Formatted Diff | Diff
patch (5.05 KB, patch)
2009-04-02 10:47 PDT, Adam Langley
eric: review-
Details | Formatted Diff | Diff
patch (5.89 KB, patch)
2009-04-16 15:55 PDT, Adam Langley
eric: review-
Details | Formatted Diff | Diff
patch (6.25 KB, patch)
2009-04-27 18:34 PDT, Adam Langley
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Langley 2009-03-27 14:55:05 PDT
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.
Comment 1 Adam Langley 2009-03-27 14:55:44 PDT
Created attachment 29025 [details]
Comment 2 Adam Langley 2009-03-27 15:01:03 PDT
Comment on attachment 29025 [details]

tc points out that one might be able to open the select box using window.eventSender.
Comment 3 Adam Langley 2009-03-27 15:34:35 PDT
Created attachment 29027 [details]
Comment 4 Adam Langley 2009-03-27 15:35:26 PDT
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 5 Darin Adler 2009-04-01 13:46:57 PDT
Comment on attachment 29027 [details]

> -        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?
Comment 6 Adam Langley 2009-04-02 10:47:28 PDT
Created attachment 29198 [details]
Comment 7 Adam Langley 2009-04-02 10:48:34 PDT
> 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 8 Darin Adler 2009-04-02 10:48:57 PDT
Comment on attachment 29198 [details]

The code change looks fine, but where's the test case that would have failed with the old patch?
Comment 9 Adam Langley 2009-04-08 19:06:13 PDT
(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 10 Oliver Hunt 2009-04-15 16:07:58 PDT
Comment on attachment 29198 [details]

I would think hyatt should review this rather than darin
Comment 11 Eric Seidel (no email) 2009-04-15 16:27:47 PDT
Comment on attachment 29198 [details]

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.
Comment 12 Adam Langley 2009-04-16 15:55:36 PDT
Created attachment 29559 [details]

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 13 Eric Seidel (no email) 2009-04-23 17:06:00 PDT
Comment on attachment 29559 [details]

+        // 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.
Comment 14 Adam Langley 2009-04-27 18:34:18 PDT
Created attachment 29833 [details]

Have updated the comment.
Comment 15 Eric Seidel (no email) 2009-05-07 00:41:03 PDT
Comment on attachment 29833 [details]

Comment 16 Eric Seidel (no email) 2009-05-07 00:42:44 PDT
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