Bug 69503

Summary:

CSS text-transform should apply to select elements

Product:

WebKit

Reporter:

Ben Wells <benwells>

Component:

New Bugs

Assignee:

Ben Wells <benwells>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

adele, aroben, jamesr, ojan, simon.fraser, tkent, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (37.35 KB, patch) 2011-10-06 23:32 PDT, Ben Wells	no flags	Details Formatted Diff Diff
Patch (37.35 KB, patch) 2011-10-07 15:14 PDT, Ben Wells	no flags	Details Formatted Diff Diff
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.