| 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: | P2 | 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: |
|
||||||||||||||||||||||
Created attachment 8024 [details]
test case
Same test case as an attachment.
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. Created attachment 8979 [details]
First attempt
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 on attachment 8979 [details]
First attempt
Removing duplicate.
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.
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. Created attachment 9015 [details]
Improved patch
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. 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. See also bug 11128 *** Bug 11128 has been marked as a duplicate of this bug. *** *** Bug 29074 has been marked as a duplicate of this bug. *** *** Bug 60039 has been marked as a duplicate of this bug. *** 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. Created attachment 95787 [details]
Demonstrates the bug through use of classes to apply display:none
*** Bug 62900 has been marked as a duplicate of this bug. *** 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. Created attachment 193498 [details]
patch
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. Let's fix it...so old issue. Created attachment 193499 [details]
Patch for display:none style for PopupMenuItem
Created attachment 193522 [details]
Patch for display:none style for PopupMenuItem
Created attachment 193530 [details]
Patch for display:none style for PopupMenuItem
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? This bug was reported about Mac. I don't think that this patch addresses the issue on Mac. 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.
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. 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. My patch is prepared, but could anybody help me with Mac build? What is wrong in linking stage? Oleg, is your question about bug 81882? I'm confused why you are asking it here. 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>. 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.
*** Bug 199011 has been marked as a duplicate of this bug. *** 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. *** Bug 25623 has been marked as a duplicate of this bug. *** https://searchfox.org/wubkat/rev/c4edc10959902651c35cf3cc5e911c757513058e/Source/WebCore/html/HTMLOptionElement.h#75 bool rendererIsNeeded(const RenderStyle&) final { return false; } Option is not stylable currently. *** Bug 49341 has been marked as a duplicate of this bug. *** *** Bug 169039 has been marked as a duplicate of this bug. *** *** Bug 240215 has been marked as a duplicate of this bug. *** *** Bug 256538 has been marked as a duplicate of this bug. *** 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. |
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>