Bug 8351 - display:none has no effect on <option> element
: display:none has no effect on <option> element
Status: NEW
Product: WebKit
Classification: Unclassified
Component: Forms
: 420+
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To: Nobody
:
Depends on: 49169
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-12 20:34 PDT by David Richardson
Modified: 2013-05-14 09:32 PDT (History)
13 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description David Richardson 2006-04-12 20:34:10 PDT
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 Alexey Proskuryakov 2006-04-28 12:35:21 PDT
Created attachment 8024 [details]
test case

Same test case as an attachment.
Comment 2 Alexey Proskuryakov 2006-04-28 12:38:59 PDT
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 Rob Buis 2006-06-23 07:26:32 PDT
Created attachment 8979 [details]
First attempt
Comment 4 Rob Buis 2006-06-23 07:29:25 PDT
Created attachment 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 Darin Adler 2006-06-23 20:52:57 PDT
Comment on attachment 8979 [details]
First attempt

Removing duplicate.
Comment 6 Darin Adler 2006-06-23 21:04:17 PDT
Comment on attachment 8980 [details]
First attempt

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 Rob Buis 2006-06-25 03:08:32 PDT
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 Rob Buis 2006-06-25 03:09:32 PDT
Created attachment 9015 [details]
Improved patch
Comment 9 Darin Adler 2006-06-25 09:58:44 PDT
Comment on attachment 9015 [details]
Improved patch

 #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 Rob Buis 2006-07-08 00:49:32 PDT
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 mitz@webkit.org 2006-12-17 10:38:06 PST
See also bug 11128
Comment 12 Alexey Proskuryakov 2008-06-18 12:15:27 PDT
*** Bug 11128 has been marked as a duplicate of this bug. ***
Comment 13 Darin Adler 2009-11-30 09:36:58 PST
*** Bug 29074 has been marked as a duplicate of this bug. ***
Comment 14 Alexey Proskuryakov 2011-05-03 11:28:36 PDT
*** Bug 60039 has been marked as a duplicate of this bug. ***
Comment 15 Tim Kirchner 2011-06-02 12:47:07 PDT
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 Tim Kirchner 2011-06-02 12:48:43 PDT
Created attachment 95787 [details]
Demonstrates the bug through use of classes to apply display:none
Comment 17 Alexey Proskuryakov 2011-06-17 14:21:24 PDT
*** Bug 62900 has been marked as a duplicate of this bug. ***
Comment 18 lvschie 2011-06-18 00:24:05 PDT
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 Oleg Smirnov 2013-03-18 00:39:52 PDT
Created attachment 193498 [details]
patch
Comment 20 WebKit Review Bot 2013-03-18 00:40:37 PDT
Comment on attachment 193498 [details]
patch

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 Oleg Smirnov 2013-03-18 00:50:00 PDT
Let's fix it...so old issue.
Comment 22 Oleg Smirnov 2013-03-18 00:55:45 PDT
Created attachment 193499 [details]
Patch for display:none style for PopupMenuItem
Comment 23 Oleg Smirnov 2013-03-18 04:07:20 PDT
Created attachment 193522 [details]
Patch for display:none style for PopupMenuItem
Comment 24 Oleg Smirnov 2013-03-18 04:44:17 PDT
Created attachment 193530 [details]
Patch for display:none style for PopupMenuItem
Comment 25 Oleg Smirnov 2013-03-18 05:40:54 PDT
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 Alexey Proskuryakov 2013-03-18 08:34:20 PDT
This bug was reported about Mac. I don't think that this patch addresses the issue on Mac.
Comment 27 Alexey Proskuryakov 2013-03-18 08:37:51 PDT
Comment on attachment 193530 [details]
Patch for display:none style for PopupMenuItem

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 Oleg Smirnov 2013-03-18 12:15:18 PDT
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 Oleg Smirnov 2013-03-20 00:42:38 PDT
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 Oleg Smirnov 2013-05-14 07:58:50 PDT
My patch is prepared, but could anybody help me with Mac build?
What is wrong in linking stage?
Comment 31 Alexey Proskuryakov 2013-05-14 09:32:08 PDT
Oleg, is your question about bug 81882? I'm confused why you are asking it here.