Bug 30245 – [Chromium] Decoration of "search" input field is wrongly rendered

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Yuta Kitamura]

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

[Yuta Kitamura]

Created attachment 40933 [details]
Fix decoration position in search input field.

[Yuta Kitamura]

FYI: Patch and try result can also be viewed from:
http://codereview.chromium.org/269030/show

[Eric Seidel (no email)]

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...

[Yuta Kitamura]

Created attachment 41087 [details]
Screenshot of current and fixed rendering

[Yuta Kitamura]

(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.

[Yuta Kitamura]

Created attachment 41262 [details]
Fix decoration position in search input field. (Fixed function name)

[Adam Barth]

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?

[Eric Seidel (no email)]

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.

[Yuta Kitamura]

Thanks, I'll try to make another patch. Dropping r? flag.

[Yuta Kitamura]

Created attachment 47502 [details]
Fix decoration position in search input field. (v3: Totally rewritten)

[Eric Seidel (no email)]

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.

[Yuta Kitamura]

(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.

[Yuta Kitamura]

Created attachment 48005 [details]
Fix decoration position in search input field. (v4: removed consts)

[Eric Seidel (no email)]

Comment on attachment 48005 [details]
Fix decoration position in search input field. (v4: removed consts)

OK.

[Yuta Kitamura]

(In reply to comment #14)
> (From update of attachment 48005 [details])
> OK.

Thank you for your review!

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.