NEW 8351
<option> element should be stylable 
https://bugs.webkit.org/show_bug.cgi?id=8351
Summary <option> element should be stylable
David Richardson
Reported 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>
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
DetailsFormatted DiffDiff
First attempt (1.68 KB, patch)
2006-06-23 07:29 PDT, Rob Buis 		darin: review-
DetailsFormatted DiffDiff
Improved patch (1.91 KB, patch)
2006-06-25 03:09 PDT, Rob Buis 		darin: review-
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch for display:none style for PopupMenuItem (17.62 KB, patch)
2013-03-18 00:55 PDT, Oleg Smirnov 		no flags
DetailsFormatted DiffDiff
Patch for display:none style for PopupMenuItem (17.81 KB, patch)
2013-03-18 04:07 PDT, Oleg Smirnov 		no flags
DetailsFormatted DiffDiff
Patch for display:none style for PopupMenuItem (17.89 KB, patch)
2013-03-18 04:44 PDT, Oleg Smirnov 		ap: review-
ap: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2006-04-28 12:35:21 PDT
Created attachment 8024 [details] test case Same test case as an attachment.
Alexey Proskuryakov
Comment 2 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.
Rob Buis
Comment 3 2006-06-23 07:26:32 PDT
Created attachment 8979 [details] First attempt
Rob Buis
Comment 4 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.
Darin Adler
Comment 5 2006-06-23 20:52:57 PDT
Comment on attachment 8979 [details] First attempt Removing duplicate.
Darin Adler
Comment 6 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.
Rob Buis
Comment 7 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.
Rob Buis
Comment 8 2006-06-25 03:09:32 PDT
Created attachment 9015 [details] Improved patch
Darin Adler
Comment 9 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.
Rob Buis
Comment 10 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.
mitz
Comment 11 2006-12-17 10:38:06 PST
See also bug 11128
Alexey Proskuryakov
Comment 12 2008-06-18 12:15:27 PDT
*** Bug 11128 has been marked as a duplicate of this bug. ***
Darin Adler
Comment 13 2009-11-30 09:36:58 PST
*** Bug 29074 has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 14 2011-05-03 11:28:36 PDT
*** Bug 60039 has been marked as a duplicate of this bug. ***
Tim Kirchner
Comment 15 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.
Tim Kirchner
Comment 16 2011-06-02 12:48:43 PDT
Created attachment 95787 [details] Demonstrates the bug through use of classes to apply display:none
Alexey Proskuryakov
Comment 17 2011-06-17 14:21:24 PDT
*** Bug 62900 has been marked as a duplicate of this bug. ***
lvschie
Comment 18 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.
Oleg Smirnov
Comment 19 2013-03-18 00:39:52 PDT
Created attachment 193498 [details] patch
WebKit Review Bot
Comment 20 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.
Oleg Smirnov
Comment 21 2013-03-18 00:50:00 PDT
Let's fix it...so old issue.
Oleg Smirnov
Comment 22 2013-03-18 00:55:45 PDT
Created attachment 193499 [details] Patch for display:none style for PopupMenuItem
Oleg Smirnov
Comment 23 2013-03-18 04:07:20 PDT
Created attachment 193522 [details] Patch for display:none style for PopupMenuItem
Oleg Smirnov
Comment 24 2013-03-18 04:44:17 PDT
Created attachment 193530 [details] Patch for display:none style for PopupMenuItem
Oleg Smirnov
Comment 25 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?
Alexey Proskuryakov
Comment 26 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.
Alexey Proskuryakov
Comment 27 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.
Oleg Smirnov
Comment 28 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.
Oleg Smirnov
Comment 29 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.
Oleg Smirnov
Comment 30 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?
Alexey Proskuryakov
Comment 31 2013-05-14 09:32:08 PDT
Oleg, is your question about bug 81882? I'm confused why you are asking it here.
Antonio Gomes
Comment 32 2016-03-28 06:07:47 PDT
As of 2016/Mar/28, bug is still valid: Safari still displays the <option> that it is not suppose to. Firefox 44 and Chrome 49 do not show the <option>.
indolering
Comment 33 2016-06-21 12:17:23 PDT
This bug makes data-binding (ala Polymer) to lists very difficult. A simple filtered list like the following: <select> <option hidden="{{!advanced}}">Advanced</option> <option>Simple</option> </select> Now requires setting up listeners to global variables and creating/destroying options manually.
Alexey Proskuryakov
Comment 34 2019-06-20 01:33:59 PDT
*** Bug 199011 has been marked as a duplicate of this bug. ***
John A. Bilicki III
Comment 35 2021-02-23 12:00:45 PST
We are moving to use optgroup for the states/provinces of each country we provide services for and simply removing class="hidden" from the appropriate optgroup. - Blink 88 works. - Gecko 56 works. - Presto 12.5 fails. - Trident 18 works. - WebKit 14.0 fails. Since Presto can't support some reasonably modern syntax we've unofficially dropped support for Opera. We simply don't have enough people or time to spend adding yet another script to fix browser bugs, especially ones from the mid-2000s. We'd literally have to scan all of the optgroup elements and move them back and forth between the select elements (yes, plural) and a hidden container. Additionally unlike other systems the forms with the select elements only need to be filled out once in many scenarios further reducing the need to spend more time on scripting for bugs. Some of our users will encounter this nonetheless and we'd appreciate a fix for it.
Radar WebKit Bug Importer
Comment 36 2022-02-25 10:40:08 PST
<rdar://problem/89481800>
Ahmad Saleem
Comment 37 2022-09-18 00:28:37 PDT
*** Bug 25623 has been marked as a duplicate of this bug. ***
Karl Dubost
Comment 38 2023-05-15 00:38:21 PDT
https://searchfox.org/wubkat/rev/c4edc10959902651c35cf3cc5e911c757513058e/Source/WebCore/html/HTMLOptionElement.h#75 bool rendererIsNeeded(const RenderStyle&) final { return false; } Option is not stylable currently.
Karl Dubost
Comment 39 2023-05-15 00:46:37 PDT
*** Bug 49341 has been marked as a duplicate of this bug. ***
Karl Dubost
Comment 40 2023-05-15 00:50:49 PDT
*** Bug 169039 has been marked as a duplicate of this bug. ***
Karl Dubost
Comment 41 2023-05-15 00:57:32 PDT
*** Bug 240215 has been marked as a duplicate of this bug. ***
Karl Dubost
Comment 42 2023-05-15 01:02:20 PDT
*** Bug 256538 has been marked as a duplicate of this bug. ***
Ahmad Saleem
Comment 43 2023-05-30 12:45:48 PDT
For 'display:non' - https://src.chromium.org/viewvc/blink?view=revision&revision=171457 ^ Blink commit. Following commit deleted, nonRendererStyle: https://github.com/WebKit/WebKit/commit/bb21df0fe805253108e49f72f3d4027d763dd385 I am able to compile with HTMLOptionElement & HTMLSelectElement parts but HTMLOptGroup would be easy as well. Only bit difficult (for me) would be 'RenderListBox' changes. Just wanted to share this as reference.
Note You need to log in before you can comment on or make changes to this bug.