Bug 24762 - Text-indent is not supported on <option> elements
Summary: Text-indent is not supported on <option> elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ada Chan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-23 11:57 PDT by Ada Chan
Modified: 2009-03-23 16:46 PDT (History)
0 users

See Also:


Attachments
Patch (6.91 KB, patch)
2009-03-23 12:06 PDT, Ada Chan
no flags Details | Formatted Diff | Diff
Second try. (8.39 KB, patch)
2009-03-23 16:15 PDT, Ada Chan
adele: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ada Chan 2009-03-23 11:57:40 PDT
Text-indent is not supported on option elements.

For example:
<select>
  <option>no text-indent</option>
  <option style="text-indent: 100px;">100px text-indent</option>
</select>

The second option should be much wider due to the text-indent, causing the select to be wider also.
Comment 1 Ada Chan 2009-03-23 12:06:52 PDT
Created attachment 28858 [details]
Patch
Comment 2 mitz 2009-03-23 12:39:40 PDT
Comment on attachment 28858 [details]
Patch

Setting the review flag
Comment 3 mitz 2009-03-23 13:40:05 PDT
Comment on attachment 28858 [details]
Patch

I think you can make the PopupMenuStyle changes in all platform, even if only Windows uses the extra members now. For the RenderMenuList.cpp change, the problem is that not all Windows platform ports use PopupMenuWin.cpp and themeWin.css. Perhaps it should be a RenderTheme decision whether to support text-indent. Then RenderMenuList can as the theme if it should apply the indent to the width, and PopupMenuWin could do the same.
Comment 4 mitz 2009-03-23 13:40:41 PDT
(In reply to comment #3)
> Then RenderMenuList can as the theme

s/as/ask/
Comment 5 Ada Chan 2009-03-23 16:15:33 PDT
Created attachment 28872 [details]
Second try.

Added RenderTheme::popupOptionSupportsTextIndent().
Comment 6 Adele Peterson 2009-03-23 16:32:38 PDT
Comment on attachment 28872 [details]
Second try.

I think you have extra spacing on the last line here.

> +            maxOptionWidth = max(maxOptionWidth, optionWidth);
> +        } else if (!text.isEmpty())
> +                maxOptionWidth = max(maxOptionWidth, style()->font().floatWidth(text));
>      }

Looks good!  r=me.
Comment 7 Ada Chan 2009-03-23 16:46:59 PDT
Fixed in r41923.