Bug 25244

Summary: <option> elements inside an <optgroup> are bold on Chromium
Product: WebKit Reporter: Adam Langley <agl>
Component: Layout and RenderingAssignee: Adam Langley <agl>
Severity: Normal CC: adele, mitz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Description Flags
adele: review-
patch fishd: review+

Description Adam Langley 2009-04-16 14:14:14 PDT
On Chromium (Windows and Linux) the following is currently misrendered:

  <optgroup label="Should be bold">
    <option>Should not be bold</option>

This occurs because the renderStyle of the element is used as the base style
for rendering. In html4.css we have:

optgroup {
  font-weight: bolder;

option {
  font-weight: normal;

However, in themeWin.css:

/* Options always use the selects' fonts */
option {
    font: inherit !important;

This stops the font-weight: normal in html4.css from taking effect. The
overriding rule was added in https://bugs.webkit.org/show_bug.cgi?id=24762
because otherwise a differnt font on the <option> could make the text wider
than the <select>. This isn't an issue for the <optgroup> as they can't be

Because of this we switch Chromium from using the element's style to using the
menuStyle() and implementing the bolding in code.

platform/mac/PopupMenuMac.mm: uses the itemStyle. However, themeWin.css isn't
  in play here so everything works out.
platform/gtk/PopupMenuGtk.cpp: renders using GTK. Doesn't appear to observe the
  style or type save for |itemIsEnabled|.
platform/win/PopupMenuWin.cpp: uses the menuStyle and handles bold itself.
platform/chromium/PopupMenuChromium.cpp: changed in this patch to observe
platform/wx/PopupMenuWx.cpp: renders with WX. Doesn't appear to observe the
  style or type.
platform/qt/PopupMenuQt.cpp: code to observe the style is #if 0'ed out
Comment 1 Adam Langley 2009-04-16 14:20:41 PDT
Created attachment 29550 [details]
Comment 2 Adele Peterson 2009-04-16 15:01:53 PDT
Comment on attachment 29550 [details]

The code change is fine.

I don't think those comments belong in html4.css though.  If anything, we need a comment in themeWin.css about why we're forcing the font to be inherited.  Something like "Option font must be inherited because we depend on computing the size of the <select> based on the size of the options, and they must use the same font for that computation to be correct".

I hesitate to say anything about the Changelog, because I think we all usually say too little in the logs, but I think you can probably have a more concise explanation here.

This is really an r+, but I'd like you to edit the comments & ChangeLog before you checkin.  If you do that, pretty much anyone can give you the final r+.

Nice work!
Comment 3 Adam Langley 2009-04-16 15:16:03 PDT
Created attachment 29553 [details]

(Addressing adele's comments)
Comment 4 Adam Langley 2009-04-16 15:17:14 PDT
Created attachment 29555 [details]

(missed bug link in the ChangeLog)
Comment 5 Darin Fisher (:fishd, Google) 2009-04-16 15:26:17 PDT
Comment on attachment 29555 [details]

r+ based on adele's previous comments.
Comment 6 Darin Fisher (:fishd, Google) 2009-04-16 15:29:47 PDT
Landed as:  http://trac.webkit.org/changeset/42597