[webkit-reviews] review denied: [Bug 8351] display:none has no effect on <option> element : [Attachment 9015] Improved patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 8351: display:none has no effect on <option> element
http://bugzilla.opendarwin.org/show_bug.cgi?id=8351

Attachment 9015: Improved patch
http://bugzilla.opendarwin.org/attachment.cgi?id=9015&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
 #include "DeprecatedRenderSelect.h"
+#include "CSSPropertyNames.h"
+#include "CSSValueKeywords.h"
 
 #include "HTMLNames.h"

New includes should go with the other includes, sorted alphabetically, not up
at the top with the file's own include and the config.h include.

+            if (listItems[listIndex]->style()) {
+                RefPtr<CSSValue> val =
listItems[listIndex]->style()->getPropertyCSSValue(CSS_PROP_DISPLAY);
+                if (val && val->isPrimitiveValue()) {
+                    if (static_cast<CSSPrimitiveValue
*>(val.get())->getIdent() == CSS_VAL_NONE)
+                    continue;
+                }
+            }

Need to indent the continue statement one more tab stop. No space between
CSSPrimitiveValue and the *.

This is a strange enough exception to the normal way style works that it needs
a comment. It's also just complicated enough that a helper function might make
it read clearly enough. I suggest this.

    static bool hasDisplayNoneStyle(StyledElement* e)
    {
        CSSStyleDeclaration* style = e->style();
        if (!style)
            return false;
        RefPtr<CSSValue> value = style->getPropertyCSSValue(CSS_PROP_DISPLAY);
        if (!value || !value->isPrimitiveValue())
            return false;
        return static_cast<CSSPrimitiveValue *>(value.get())->getIdent() ==
CSS_VAL_NONE;
    }

... then later on ...

            // Omit items with display:none style as a special case.
            // Eventually we probably want to make this work as a part of the
style system,
            // but an explicit hack for this one style is a good thing for now.

            if (hasDisplayNoneStyle(listItems[listIndex]))
                continue;

We need a test for thisl either a manual or layout test. Maybe a select element
with a size would work best, so we get the list box style and you can see the
items in the pixel test.

> I was under the impression that style resolution could become very costly
when having selects with a lot of option items inside. If that is not the case,
I can give it a go, though I am not sure how the display:none in the default
html.css will affect the outcome.

I think it's unlikely that style resolution would be so slow we couldn't use
it; we already do style resolution for everything else so I can't see why
options would be so much worse. We would have to figure out what to do about
the "display: none" in html4.css of course.

> Also it seemed to me FF doesnt do full style resolution for options either.

Given this remark, the kinds of questions I have are:

    (1) Which styles does Firefox respect? You already said that it seems to do
something for visibility:hidden as well as for display:none. Are there others?
    (2) Where did the idea to treat the styles this way come from? Did someone
just think it would be a neat idea or does the CSS specification call for this
behavior?
    (3) Has any other engine besides Gecko done this yet?

I'd like to understand better the big picture so I know how this one change
fits in. It's definitely not part of the normal way our style system works, so
we need to understand why.

review- for the lack of a test case and partly because of the other issues I
raise here.