RESOLVED FIXED Bug 69503
CSS text-transform should apply to select elements 
https://bugs.webkit.org/show_bug.cgi?id=69503
Summary CSS text-transform should apply to select elements
Ben Wells
Reported 2011-10-05 23:14:09 PDT
CSS text-transform should apply to select elements
Attachments
Patch (28.38 KB, patch)
2011-10-05 23:24 PDT, Ben Wells 		no flags
DetailsFormatted DiffDiff
Patch (37.35 KB, patch)
2011-10-06 23:32 PDT, Ben Wells 		no flags
DetailsFormatted DiffDiff
Patch (37.35 KB, patch)
2011-10-07 15:14 PDT, Ben Wells 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Ben Wells
Comment 1 2011-10-05 23:24:43 PDT
Created attachment 109920 [details] Patch
Tony Chang
Comment 2 2011-10-06 11:39:33 PDT
In the popup case, it seems a bit weird to me to apply the text transform, but not apply other styles like bold, color, background-color, etc. Some of these may be limitations of the OS that draws the popup. In the RenderListBox case, maybe we should be using a RenderText object instead of trying to apply these styles manually? Maybe there's some history as to why we don't do that now. Can you add a test case with the German essen (ß)? It changes length when being capitalized. View in context: https://bugs.webkit.org/attachment.cgi?id=109920&action=review > Source/WebCore/rendering/style/RenderStyle.h:525 > + void applyTextTransform(String& text, UChar previousCharacter) const; RenderStyle is the wrong place for this function. I would keep it in RenderText.h and either make it static (pass in texttransform value) or just make it a global function like toRenderText.
Tony Chang
Comment 3 2011-10-06 11:39:42 PDT
Comment on attachment 109920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109920&action=review > Source/WebCore/rendering/style/RenderStyle.h:525 > + void applyTextTransform(String& text, UChar previousCharacter) const; RenderStyle is the wrong place for this function. I would keep it in RenderText.h and either make it static (pass in texttransform value) or just make it a global function like toRenderText.
Ben Wells
Comment 4 2011-10-06 23:32:11 PDT
Created attachment 110096 [details] Patch
Ben Wells
Comment 5 2011-10-06 23:59:30 PDT
(In reply to comment #4) > Created an attachment (id=110096) [details] > Patch This patch has the following changes: - function moved back to RenderText - German essen used in the test - updates to get widths right (this was obviously wrong after the change to the test). Outstanding questions: - should the menu style also apply font-style, font-weight etc. I don't know if this is possible with Aqua. Some possible actions - do nothing, log an orthogonal bug, fix this as part of this bug. - should RenderListBox / RenderMenuList use a RenderText object to transform text? Note: With my limited knowledge of German I assumed the capitalization transform applied to the essen was wrong, but after reading http://en.wikipedia.org/wiki/German_alphabet#Sharp_s I think the behaviour is probably correct.
Simon Fraser (smfr)
Comment 6 2011-10-07 08:55:38 PDT
Comment on attachment 110096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110096&action=review > Source/WebCore/rendering/RenderMenuList.cpp:385 > + itemString = optionElement->textIndentedToRespectGroupLabel(); > + applyTextTransform(style(), itemString, ' '); > + > + return itemString; I'd prefer the blank line before the applyTextTransform
Ojan Vafai
Comment 7 2011-10-07 11:19:59 PDT
Comment on attachment 110096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110096&action=review Just a few nits about the test. They're nits, so feel free to take them or leave them. > LayoutTests/fast/css/text-transform-select.html:3 > +<meta charset="iso-8859-1"> do you need the meta tag? > LayoutTests/fast/css/text-transform-select.html:8 > + .lower * { text-transform: lowercase; } nit: there's no official style guide for tests, but we use four-space indents everywhere else, may as well use them in tests as well. it lets people keep their text editors to always insert 4 space indents. > LayoutTests/fast/css/text-transform-select.html:11 > +</head> > +<body> While we're at it, you can drop the html, head and body elements as well. > LayoutTests/fast/css/text-transform-select.html:14 > +<div class="upper"> nit: this test would be easier to read if you indented the contents of the divs.
Ben Wells
Comment 8 2011-10-07 15:14:28 PDT
Created attachment 110224 [details] Patch
WebKit Review Bot
Comment 9 2011-10-07 16:28:57 PDT
Comment on attachment 110224 [details] Patch Clearing flags on attachment: 110224 Committed r96987: <http://trac.webkit.org/changeset/96987>
WebKit Review Bot
Comment 10 2011-10-07 16:29:02 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.