Bug 8351

Summary:

<option> element should be stylable

Product:

WebKit

Reporter:

David Richardson <channel200>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

NEW

Severity:

Normal

CC:

adele, ahmad.saleem792, akeerthi, alexey.verlinger, ap, gavinp, gordeon, ian, jab_creations, jasneet, karlcow, kris, Lordfate, lvschie, netdur, oleg_smirnov, pnormand, priyajeet.hora, robert, simon.fraser, tkirchner, tlock.chijin, tonikitoo, webkit-bug-importer, wilsonpjunior, zachlym

Priority:

Keywords:

BrowserCompat, InRadar

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

See Also:

https://bugs.webkit.org/show_bug.cgi?id=81882
https://bugs.webkit.org/show_bug.cgi?id=256538
https://bugs.webkit.org/show_bug.cgi?id=232233
https://bugs.webkit.org/show_bug.cgi?id=240652
https://bugs.webkit.org/show_bug.cgi?id=240771
https://bugs.webkit.org/show_bug.cgi?id=255335

Bug Depends on:

49169

Bug Blocks:

Attachments:

Description	Flags
test case	none
First attempt	none
First attempt	darin: review-
Improved patch	darin: review-
Demonstrates the bug through use of classes to apply display:none	none
patch	none
Patch for display:none style for PopupMenuItem	none
Patch for display:none style for PopupMenuItem	none
Patch for display:none style for PopupMenuItem	ap: review-, ap: commit-queue-

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	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
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.