Bug 8351 - display:none has no effect on <option> element
: display:none has no effect on <option> element
Status: NEW
: WebKit
Forms
: 420+
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
:
:
: 49169
:
  Show dependency treegraph
 
Reported: 2006-04-12 20:34 PST by
Modified: 2013-05-14 09:32 PST (History)


Attachments
test case (760 bytes, text/html)
2006-04-28 12:35 PST, Alexey Proskuryakov
no flags Details
First attempt (1.68 KB, patch)
2006-06-23 07:26 PST, Rob Buis
no flags Review Patch | Details | Formatted Diff | Diff
First attempt (1.68 KB, patch)
2006-06-23 07:29 PST, Rob Buis
darin: review-
Review Patch | Details | Formatted Diff | Diff
Improved patch (1.91 KB, patch)
2006-06-25 03:09 PST, Rob Buis
darin: review-
Review Patch | Details | Formatted Diff | Diff
Demonstrates the bug through use of classes to apply display:none (390 bytes, text/html)
2011-06-02 12:48 PST, Tim Kirchner
no flags Details
patch (17.62 KB, patch)
2013-03-18 00:39 PST, Oleg Smirnov
no flags Review Patch | Details | Formatted Diff | Diff
Patch for display:none style for PopupMenuItem (17.62 KB, patch)
2013-03-18 00:55 PST, Oleg Smirnov
no flags Review Patch | Details | Formatted Diff | Diff
Patch for display:none style for PopupMenuItem (17.81 KB, patch)
2013-03-18 04:07 PST, Oleg Smirnov
no flags Review Patch | Details | Formatted Diff | Diff
Patch for display:none style for PopupMenuItem (17.89 KB, patch)
2013-03-18 04:44 PST, Oleg Smirnov
ap: review-
ap: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2006-04-12 20:34:10 PST
The display:none style has no effect on <option> elements. This really should be fixed sooner rather than waiting for fully-styled form elements to implement. As it stands, dynamically-filtered <select> lists are needlessly complex.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
        "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
    <head>
        <title>
            Annoying Deficiency
        </title>
        <meta name="generator" content="BBEdit 8.2" />
    </head>
    <body>
        <form method="get">
        <select size="2">
            <option label="Item1" value="Item1" style="display:none">this item should be hidden!
            </option>
            <option label="Item2" value="Item2">Item2
            </option>
        </select>
    </form>
</body>
</html>
------- Comment #1 From 2006-04-28 12:35:21 PST -------
Created an attachment (id=8024) [details]
test case

Same test case as an attachment.
------- Comment #2 From 2006-04-28 12:38:59 PST -------
The test case works in Firefox, but not in WinIE for me.

Please note that the titles of the options are displayed incorrectly in both stock Safari and ToT. This is highly related to bug 8398, but I have also noticed that the behavior in stock Safari depends on the parsing mode (i.e. the presence of a DOCTYPE). Probably worth a separate bug report.
------- Comment #3 From 2006-06-23 07:26:32 PST -------
Created an attachment (id=8979) [details]
First attempt
------- Comment #4 From 2006-06-23 07:29:25 PST -------
Created an attachment (id=8980) [details]
First attempt

A simple fix for the problem :) It is really hard to catch display:none in a stylesheet
for this. First html4.css "overwrites" it, and checking this for each option would be costly.
Also I checked Firefox and it doesn't handle display:none on options when specified in a stylesheet.
Cheers,

Rob.
------- Comment #5 From 2006-06-23 20:52:57 PST -------
(From update of attachment 8979 [details])
Removing duplicate.
------- Comment #6 From 2006-06-23 21:04:17 PST -------
(From update of attachment 8980 [details])
Ideally this would not be done inside the renderer since the renderer is going away, but I suppose Adele can just consider it when re-implementing it.

It seems very strange to check the style attached to an object directly without doing any style resolution. If we are going to do it, we should not be converting it to a string. We should be getting the CSSValue object and then calling isPrimitiveValue(), casting to a CSSPrimitiveValue, calling getIdent() on it, and then checking against CSS_VAL_NONE.

Besides not doing any style resolution, it also seems strange to single out display:none and not implement any other properties, like visibility:hidden or other stranger display values.

I'm going to review- this because at the very least we should use getPropertyCSSValue instead of getPropertyValue.
------- Comment #7 From 2006-06-25 03:08:32 PST -------
Hi Darin,
(In reply to comment #6)
> (From update of attachment 8980 [details] [edit])
> Ideally this would not be done inside the renderer since the renderer is going
> away, but I suppose Adele can just consider it when re-implementing it.

I do not know the exact plans, though I noticed the Deprecated bit ofcourse.

> It seems very strange to check the style attached to an object directly without
> doing any style resolution. If we are going to do it, we should not be

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 displa:none in the default html.css
will affect the outcome.
Also it seemed to me FF doesnt do full style resolution for options either.

> converting it to a string. We should be getting the CSSValue object and then
> calling isPrimitiveValue(), casting to a CSSPrimitiveValue, calling getIdent()
> on it, and then checking against CSS_VAL_NONE.

Right, does seem better.

> Besides not doing any style resolution, it also seems strange to single out
> display:none and not implement any other properties, like visibility:hidden or
> other stranger display values.

Well, that is not in the bug description :) Could be added, note that FF does insert
the item when having visibility:hidden, but gives it no title/label.

> I'm going to review- this because at the very least we should use
> getPropertyCSSValue instead of getPropertyValue.

I'll attach a patch that does this. Please indicate how to go on with this bug fix, may
need chatting with Adele?
Cheers,

Rob.
------- Comment #8 From 2006-06-25 03:09:32 PST -------
Created an attachment (id=9015) [details]
Improved patch
------- Comment #9 From 2006-06-25 09:58:44 PST -------
(From update of attachment 9015 [details])
 #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.
------- Comment #10 From 2006-07-08 00:49:32 PST -------
Hi Darin,

(In reply to comment #9)
> (From update of attachment 9015 [details] [edit])
> 
> 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.
> 

The discussion is very interesting, however because of the (nice!) new code for options
and optgroups by adele, my patch is getting outdated. So after discussing with adele on
irc we decide to wait for adele to cover this bug while fixing <select> lists. Ofcourse any
part of this patch can be reused if needed.
Cheers,

Rob.
------- Comment #11 From 2006-12-17 10:38:06 PST -------
See also bug 11128
------- Comment #12 From 2008-06-18 12:15:27 PST -------
*** Bug 11128 has been marked as a duplicate of this bug. ***
------- Comment #13 From 2009-11-30 09:36:58 PST -------
*** Bug 29074 has been marked as a duplicate of this bug. ***
------- Comment #14 From 2011-05-03 11:28:36 PST -------
*** Bug 60039 has been marked as a duplicate of this bug. ***
------- Comment #15 From 2011-06-02 12:47:07 PST -------
Besides simply removing duplicates, has there been any traction on this issue?

It's worth noting that if you apply the display:none; via a class, it's the same effect.  The web app I'm working on actually programatically turns on/off a stylessheet (that contains such a class definition), which is how I stumbled across this bug.

Attaching another test case that demonstrates the issue, but using classes instead of inline styles.
------- Comment #16 From 2011-06-02 12:48:43 PST -------
Created an attachment (id=95787) [details]
Demonstrates the bug through use of classes to apply display:none
------- Comment #17 From 2011-06-17 14:21:24 PST -------
*** Bug 62900 has been marked as a duplicate of this bug. ***
------- Comment #18 From 2011-06-18 00:24:05 PST -------
All these "marked duplicates" date back to 2006. Ergo this bug has been there for 5 years now. Is there any chance of this getting fixed at all? 

It's all here, the proof, the fixes etc.

This makes creating dynamic lists so much easier.
------- Comment #19 From 2013-03-18 00:39:52 PST -------
Created an attachment (id=193498) [details]
patch
------- Comment #20 From 2013-03-18 00:40:37 PST -------
(From update of attachment 193498 [details])
Rejecting attachment 193498 [details] from review queue.

oleg.smirnov@lge.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.
------- Comment #21 From 2013-03-18 00:50:00 PST -------
Let's fix it...so old issue.
------- Comment #22 From 2013-03-18 00:55:45 PST -------
Created an attachment (id=193499) [details]
Patch for display:none style for PopupMenuItem
------- Comment #23 From 2013-03-18 04:07:20 PST -------
Created an attachment (id=193522) [details]
Patch for display:none style for PopupMenuItem
------- Comment #24 From 2013-03-18 04:44:17 PST -------
Created an attachment (id=193530) [details]
Patch for display:none style for PopupMenuItem
------- Comment #25 From 2013-03-18 05:40:54 PST -------
Hi folks,

Build for patch is failed for MAC and QT platforms.
This patch is correct as using PopupMenuStyle as main style for WebPopupItem*.
instead of several duplictate class members...

I can propagate this patch for other platforms too or only for GTK WK2.

What is better?
------- Comment #26 From 2013-03-18 08:34:20 PST -------
This bug was reported about Mac. I don't think that this patch addresses the issue on Mac.
------- Comment #27 From 2013-03-18 08:37:51 PST -------
(From update of attachment 193530 [details])
The right way to proceed with a Gtk only fix is to file two new bugs, one for cross-platform plumbing, and another for Gtk parts.
------- Comment #28 From 2013-03-18 12:15:18 PST -------
Alexey, sure. I will separate patch on several parts. will prepare first part of patch patch for Gtk backend only for issue 81882. After as changes will be landed it can be posible to create patch for Mac backend, Efl and QT.
------- Comment #29 From 2013-03-20 00:42:38 PST -------
Hi,
You can see attahced patch to GTK+WK2 issue: https://bugs.webkit.org/show_bug.cgi?id=81882

But Mac builds can not resolve linking symbols for new added code.
For example: _ZN6WebKit12WebPopupItemC2ENS0_4TypeERKN3WTF6StringERKN7WebCore14PopupMenuStyleES5_S5_bbb in WebPopupItem.o

Could can help me?
How can i reconfigure WebKit2.order file or similar as needed for Mac build?
I have no XCode.

After these changes build tree will be green for all platforms, an i can continue working for this issue and for all issues related to stuling of WebPopupItem.

Thanks.
------- Comment #30 From 2013-05-14 07:58:50 PST -------
My patch is prepared, but could anybody help me with Mac build?
What is wrong in linking stage?
------- Comment #31 From 2013-05-14 09:32:08 PST -------
Oleg, is your question about bug 81882? I'm confused why you are asking it here.