Bug 30245

Summary: [Chromium] Decoration of "search" input field is wrongly rendered
Product: WebKit Reporter: Yuta Kitamura <yutak>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Fix decoration position in search input field.
none
Screenshot of current and fixed rendering
none
Fix decoration position in search input field. (Fixed function name)
none
Fix decoration position in search input field. (v3: Totally rewritten)
none
Fix decoration position in search input field. (v4: removed consts) none

Description Yuta Kitamura 2009-10-09 00:15:41 PDT
In LayoutTests/fast/forms/search-transformed.html, Chromium renders decoration of "search" input field (loupe icon and close button) at wrong position.

This is caused due to a bug in WebCore/rendering/RenderThemeChromiumSkia.cpp.

Chromium bug:
Layout Test failure: type=search input has disappearing (x)
http://code.google.com/p/chromium/issues/detail?id=20439
Comment 1 Yuta Kitamura 2009-10-09 00:20:42 PDT
Created attachment 40933 [details]
Fix decoration position in search input field.
Comment 2 Yuta Kitamura 2009-10-09 00:22:48 PDT
FYI: Patch and try result can also be viewed from:
http://codereview.chromium.org/269030/show
Comment 3 Eric Seidel (no email) 2009-10-09 09:20:58 PDT
Comment on attachment 40933 [details]
Fix decoration position in search input field.

Where can I see the resulting rendered image of the new search box?

Style:
 154         IntRect calcParentBoxOfSearchFieldItem(RenderObject* o, const IntRect& r);

calc is a strange abbreviation.  Normally we use full english words, although "calc" might be OK.

Again, full words are generally preferred over letters:
 392     IntRect bounds = r;


You're trying to get the content rect of the parent box in the grandparent's coordinates?  I feel like there is an easier way to do that...
Comment 4 Yuta Kitamura 2009-10-12 23:19:53 PDT
Created attachment 41087 [details]
Screenshot of current and fixed rendering
Comment 5 Yuta Kitamura 2009-10-12 23:31:54 PDT
(In reply to comment #3)
> (From update of attachment 40933 [details])
> Where can I see the resulting rendered image of the new search box?

Uploaded.

> 
> Style:
>  154         IntRect calcParentBoxOfSearchFieldItem(RenderObject* o, const
> IntRect& r);
> 
> calc is a strange abbreviation.  Normally we use full english words, although
> "calc" might be OK.
> 

Sure. I'll update the patch accordingly.

> Again, full words are generally preferred over letters:
>  392     IntRect bounds = r;
> 
> 
> You're trying to get the content rect of the parent box in the grandparent's
> coordinates?  I feel like there is an easier way to do that...

We need to find the *drawing position* of parent box. Since the coordinate is relative to the current layer (created by -webkit-transform), it's not same as the absolute position. I tried to find more adequate way to calculate this but I could not. Suggestions are welcome.
Comment 6 Yuta Kitamura 2009-10-15 21:06:59 PDT
Created attachment 41262 [details]
Fix decoration position in search input field. (Fixed function name)
Comment 7 Adam Barth 2009-10-18 23:12:11 PDT
This patch seems reasonable, but it involves drawing to the screen, which I know nothing about.  Eric, would you be willing to have another look?
Comment 8 Eric Seidel (no email) 2009-10-19 08:39:52 PDT
You may already be familiar with this, but this is the box model to which I refer:
http://www.w3.org/TR/CSS21/box.html

Most of those accessors are available on RenderObject or RenderBoxModelObject.

I still don't really like r and o for names.  Particularly r, because it doesn't tell me anythign about what coordinate system the rect your passing in should be in.

damageRectInLocalCoords would be a much better name if it happens to be the damage rect in content box coordinates relative to "o".  I'd have to stare harder to figure out what you're actually trying to do here.
Comment 9 Yuta Kitamura 2009-10-26 18:17:54 PDT
Thanks, I'll try to make another patch. Dropping r? flag.
Comment 10 Yuta Kitamura 2010-01-26 23:27:18 PST
Created attachment 47502 [details]
Fix decoration position in search input field. (v3: Totally rewritten)
Comment 11 Eric Seidel (no email) 2010-02-01 15:21:35 PST
Comment on attachment 47502 [details]
Fix decoration position in search input field. (v3: Totally rewritten)

I don't understand why you're adding all the const declarations?  That's unrelated to this patch and should not be part of this.  WebKit seems to have mixed feelings on the value of const anyway.
Comment 12 Yuta Kitamura 2010-02-03 01:44:04 PST
(In reply to comment #11)
> (From update of attachment 47502 [details])
> I don't understand why you're adding all the const declarations?  That's
> unrelated to this patch and should not be part of this.  WebKit seems to have
> mixed feelings on the value of const anyway.

It's because the first argument of RenderThemeChromiumSkia::convertToPaintingRect 
is marked as const. The signature of this function is originally taken from
RenderThemeMac::convertToPaintingRect, whose first argument is also marked as
const.

I can certainly strip away these consts, but, to be honest, I really think
there is no reason to leave the arguments of offsetContainer non-const.
I believe it's common and important in C++ to distinguish const pointer variables
from non-const ones.
Comment 13 Yuta Kitamura 2010-02-03 02:48:30 PST
Created attachment 48005 [details]
Fix decoration position in search input field. (v4: removed consts)
Comment 14 Eric Seidel (no email) 2010-02-17 14:41:52 PST
Comment on attachment 48005 [details]
Fix decoration position in search input field. (v4: removed consts)

OK.
Comment 15 Yuta Kitamura 2010-02-21 21:44:05 PST
(In reply to comment #14)
> (From update of attachment 48005 [details])
> OK.

Thank you for your review!
Comment 16 WebKit Commit Bot 2010-03-06 04:26:14 PST
Comment on attachment 48005 [details]
Fix decoration position in search input field. (v4: removed consts)

Clearing flags on attachment: 48005

Committed r55617: <http://trac.webkit.org/changeset/55617>
Comment 17 WebKit Commit Bot 2010-03-06 04:26:18 PST
All reviewed patches have been landed.  Closing bug.