Bug 59825 - Respect fixed text-indent on ::-webkit-input-placeholder
Summary: Respect fixed text-indent on ::-webkit-input-placeholder
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-29 13:45 PDT by Joseph Pecoraro
Modified: 2011-05-02 17:21 PDT (History)
3 users (show)

See Also:


Attachments
[PATCH] Handle fixed text-indent for placeholders (14.95 KB, patch)
2011-04-29 13:50 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Handle fixed text-indent for placeholders (29.71 KB, patch)
2011-04-29 14:10 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[IMAGE-DIFF] For the pixel test (12.81 KB, image/png)
2011-04-29 14:10 PDT, Joseph Pecoraro
no flags Details
[PATCH] Handle fixed text-indent for placeholders (tests rtl) (7.60 KB, patch)
2011-04-29 20:47 PDT, Joseph Pecoraro
ddkilzer: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff
[IMAGE-DIFF] For the pixel test (18.09 KB, image/png)
2011-04-29 20:47 PDT, Joseph Pecoraro
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2011-04-29 13:45:45 PDT
It would be nice if users could specify some text-indent on placeholders.
I don't think it makes much sense to do percentages, but I can add that
if desired as well.
Comment 1 Joseph Pecoraro 2011-04-29 13:50:35 PDT
Created attachment 91726 [details]
[PATCH] Handle fixed text-indent for placeholders

I haven't tested the tolerance level of the png. I may need to make the
inputs larger, or add more (for instance test <input type="search">
as well).

This doesn't handle percentages, because I think while we are already
doing limited styling it seems like unnecessary work. But I can attempt
that if desired.
Comment 2 Joseph Pecoraro 2011-04-29 14:10:03 PDT
Created attachment 91731 [details]
[PATCH] Handle fixed text-indent for placeholders

Increasing the font-size causes the diff to be 0.65% which
is over the default tolerance. Image diff to follow.
Comment 3 Joseph Pecoraro 2011-04-29 14:10:29 PDT
Created attachment 91732 [details]
[IMAGE-DIFF] For the pixel test
Comment 4 Simon Fraser (smfr) 2011-04-29 19:58:01 PDT
Comment on attachment 91731 [details]
[PATCH] Handle fixed text-indent for placeholders

View in context: https://bugs.webkit.org/attachment.cgi?id=91731&action=review

> Source/WebCore/rendering/RenderTextControl.cpp:649
> +            textPoint.setX(tx + width() - textBlockInsetRight() - styleTextIdent - style()->font().width(textRun));

Shouldn't you test the RTL code in the layout test too?
Comment 5 Joseph Pecoraro 2011-04-29 20:32:23 PDT
> Shouldn't you test the RTL code in the layout test too?

Doh, I should. I totally meant to and forgot. Thanks
for pointing it out. I'll be making another patch.
Comment 6 Joseph Pecoraro 2011-04-29 20:47:14 PDT
Created attachment 91793 [details]
[PATCH] Handle fixed text-indent for placeholders (tests rtl)

Add in some tests for rtl. Lowered the test to 30px font size
so it would all fit in the image. Diff was 0.76%.
Comment 7 Joseph Pecoraro 2011-04-29 20:47:44 PDT
Created attachment 91794 [details]
[IMAGE-DIFF] For the pixel test
Comment 8 David Kilzer (:ddkilzer) 2011-05-02 14:46:44 PDT
Comment on attachment 91793 [details]
[PATCH] Handle fixed text-indent for placeholders (tests rtl)

View in context: https://bugs.webkit.org/attachment.cgi?id=91793&action=review

r=me with the typo fixed.

> Source/WebCore/rendering/RenderTextControl.cpp:645
> +        int styleTextIdent = placeholderStyle->textIndent().isFixed() ? placeholderStyle->textIndent().calcMinValue(0) : 0;

Typo?  "Ident" -> "Indent"
Comment 9 Joseph Pecoraro 2011-05-02 17:21:23 PDT
> > Source/WebCore/rendering/RenderTextControl.cpp:645
> > +        int styleTextIdent = placeholderStyle->textIndent().isFixed() ? placeholderStyle->textIndent().calcMinValue(0) : 0;
> 
> Typo?  "Ident" -> "Indent"

Good catch. Fixed.
Comment 10 Joseph Pecoraro 2011-05-02 17:21:56 PDT
Landed in r85560:
http://trac.webkit.org/changeset/85560

Windows will need expected results update (since it falls back to mac).
I will update them once I can pull results from the bots.