Bug 25742 – Fix RenderThemeChromiumWin::paintTextFieldInternal() not to hide background image.

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

[Takeshi Yoshino]

Fix RenderThemeChromiumWin::paintTextFieldInternal().

We shouldn't paint the content area of text fields when o->style() has background image or transparent background color. paintTextFieldInternal() is used for painting inner area of HTML option elements by Chromium.

When we pass fillContentArea = true to ChromiumBridge::paintTextField, it hides the background image rendered by RenderBoxModelObject. So, we should set fillContentArea = false in such case.

Besides, when background-color:transparent is specified for CSS property, o->style().backgroundColor returns black color with alpha channel == 0. But since ThemeEngine for Windows behind ChromiumBridge::paintTextField cannot recognize alpha channel, it fills the rect with black. I made workaround to set fillContentArea = false when alpha channel == 0 to avoid this.

And more, I'd like to fallback the color passed to ChromiumBridge to white when o->style()->backgroundColor() is invalid.

[Takeshi Yoshino]

Created attachment 30258 [details]
Proposed fix for 25742

[Eric Seidel (no email)]

Comment on attachment 30258 [details]
Proposed fix for 25742

Brett Wilson is not a webkit reviewer.  You also do not need to mark the requestee field for WebKit reviews.

I would have written this line:

Color backgroundColor = o->style()->backgroundColor().isValid() ? o->style()->backgroundColor() : Color::white;

Does this match other platforms' behaviors here?

This needs test cases.

[Takeshi Yoshino]

Created attachment 30281 [details]
Proposed fix for 25742 (rev 2)

[Takeshi Yoshino]

(In reply to comment #2)

Thank you for review.

> (From update of attachment 30258 [details] [review])
> Brett Wilson is not a webkit reviewer.  You also do not need to mark the
> requestee field for WebKit reviews.
> 

I see.

> I would have written this line:
> 
> Color backgroundColor = o->style()->backgroundColor().isValid() ?
> o->style()->backgroundColor() : Color::white;
> 

Done.

> Does this match other platforms' behaviors here?
> 

What do you mean by platform here? This code seems to be specific for Chromium/Windows.

> This needs test cases.
> 

A lot of chromium specific test for code in WebKit repository seems to be staying in chromium repository not in WebKit repository yet. There is a test case that can be extended to check the case that I'm going to fix by this patch. Isn't it OK to just add tests in chromium trunk?

[Darin Fisher (:fishd, Google)]

> A lot of chromium specific test for code in WebKit repository seems to be
> staying in chromium repository not in WebKit repository yet. There is a test
> case that can be extended to check the case that I'm going to fix by this
> patch. Isn't it OK to just add tests in chromium trunk?

Our normal mode of operation is to add new layout tests to WebKit.  Then, when necessary we create Chromium specific results for the new layout tests, and we presently store those in the Chromium repository.  In time (hopefully soon), we will also store the Chromium specific results in the WebKit repository.

[Takeshi Yoshino]

(In reply to comment #5)
> > A lot of chromium specific test for code in WebKit repository seems to be
> > staying in chromium repository not in WebKit repository yet. There is a test
> > case that can be extended to check the case that I'm going to fix by this
> > patch. Isn't it OK to just add tests in chromium trunk?
> 
> Our normal mode of operation is to add new layout tests to WebKit.  Then, when
> necessary we create Chromium specific results for the new layout tests, and we
> presently store those in the Chromium repository.  In time (hopefully soon), we
> will also store the Chromium specific results in the WebKit repository.
> 

I'm not sure if I'm understanding you. Tests under http://src.chromium.org/viewvc/chrome/trunk/src/webkit/data/layout_tests/chrome/ is also not in the WebKit repository as well as the Chromium specific results for them in
- http://src.chromium.org/viewvc/chrome/trunk/src/webkit/data/layout_tests/chrome/
- http://src.chromium.org/viewvc/chrome/trunk/src/webkit/data/layout_tests/platform/chromium-*/chrome.

I thought that http://src.chromium.org/viewvc/chrome/trunk/src/webkit/data/layout_tests/chrome/is the right place to add new test cases to exercise my patch. Isn't it right?

If no, could you tell me where is the right place to put new tests in the WebKit repository?

Thanks.

[Eric Seidel (no email)]

WebKit tests reside in the LayoutTests directory in your WebKit checkout.  Your WebKit checkout is separate from your Chromium checkout.

[Eric Seidel (no email)]

Comment on attachment 30281 [details]
Proposed fix for 25742 (rev 2)

r- for lack of a layout test.  (Mostly marking this so it's out of the review queue.)

[Ojan Vafai]

The tests in layout_tests/pending and layout_tests/chrome are there for historical reasons. Eventually, we intend to move all those tests into the WebKit tree. As such, we should not be adding new tests there. All new tests should go directly into the LayoutTests directory of the WebKit tree.

[Takeshi Yoshino]

(In reply to comment #9)
> The tests in layout_tests/pending and layout_tests/chrome are there for
> historical reasons. Eventually, we intend to move all those tests into the
> WebKit tree. As such, we should not be adding new tests there. All new tests
> should go directly into the LayoutTests directory of the WebKit tree.
> 

All right. I'll add tests to the WebKit tree, and then request review again.
Thank you and sorry for troubling you.

[Takeshi Yoshino]

Created attachment 30466 [details]
Layout test

Layout test draft. To be added to the patch with Leopard result and Chromium result.

[Takeshi Yoshino]

Created attachment 30467 [details]
Expected results for layout test on Mac

[Eric Seidel (no email)]

I'm surprised there aren't already layout tests which cover backgrounds on forms.  But maybe not?  In either case, the test looks fine.

[Takeshi Yoshino]

(In reply to comment #13)
> I'm surprised there aren't already layout tests which cover backgrounds on
> forms.  But maybe not?  In either case, the test looks fine.
> 

The first case in the html I just attached is already in the tree.
http://svn.webkit.org/repository/webkit/trunk/LayoutTests/fast/forms/select-style.html
But I couldn't find any test to test various cases related to background-* properties.
I've extended it.

I'll update the patch to include this.

[Takeshi Yoshino]

Created attachment 30494 [details]
Proposed fix for 25742 (rev 3)

Added new test cases and expected results for them.

[Eric Seidel (no email)]

Comment on attachment 30494 [details]
Proposed fix for 25742 (rev 3)

Fantastic.  I'll land this right now.

[Takeshi Yoshino]

(In reply to comment #16)
> (From update of attachment 30494 [details] [review])
> Fantastic.  I'll land this right now.
> 

Thank you Eric and all for review and support.

[Eric Seidel (no email)]

Fantastic patch.  Thank you!

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/forms/select-style.html
	M	LayoutTests/platform/mac/fast/forms/select-style-expected.checksum
	M	LayoutTests/platform/mac/fast/forms/select-style-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderThemeChromiumWin.cpp
Committed r43903

[Eric Seidel (no email)]

I missed a couple files when landing:

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        A      
LayoutTests/platform/mac/fast/forms/search-styled-expected.checksum
        A       LayoutTests/platform/mac/fast/forms/search-styled-expected.png
Committed r43904

Turns out the Bug URL in teh changelog is also wrong.  I'll fix it.... sec.

[Eric Seidel (no email)]

Fixed bug urls:
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	WebCore/ChangeLog
Committed r43906

[Takeshi Yoshino]

(In reply to comment #19)
> I missed a couple files when landing:
> 
> Committing to http://svn.webkit.org/repository/webkit/trunk ...
>         A      
> LayoutTests/platform/mac/fast/forms/search-styled-expected.checksum
>         A       LayoutTests/platform/mac/fast/forms/search-styled-expected.png
> Committed r43904
> 
> Turns out the Bug URL in teh changelog is also wrong.  I'll fix it.... sec.
> 

Ah, sorry for that. I was looking at the bug for reference in writing the change log and copied as is by mistake.

[Takeshi Yoshino]

Hi Eric,

Could you also replace the png file?
I didn't include it in the patch, but it's in the attached tar ball. https://bugs.webkit.org/attachment.cgi?id=30467

Thanks,

[Takeshi Yoshino]

Created attachment 30993 [details]
Patch for rebaselining the expected image

Created a patch for rebaselining the expected image.

[Eric Seidel (no email)]

I'm confused.  You just attached a patch for review to a bug which is closed.  Was that intentional?  Generally once bugs are closed we use new bugs for new patches.

[Takeshi Yoshino]

As said in comment #22, this rebaselining corresponds to the fix committed in this bug. I forgot to use svn-create-patch so missed adding image binary to the patch.

I follow you and creates a new bug. Thanks.

[Takeshi Yoshino]

(In reply to comment #25)
> As said in comment #22, this rebaselining corresponds to the fix committed in
> this bug. I forgot to use svn-create-patch so missed adding image binary to the
> patch.
> 
> I follow you and creates a new bug. Thanks.
> 

Here's the new entry.
https://bugs.webkit.org/show_bug.cgi?id=26207

[David Kilzer (:ddkilzer)]

http://trac.webkit.org/changeset/43903
http://trac.webkit.org/changeset/43904
http://trac.webkit.org/changeset/43906