Bug 25742 - Fix RenderThemeChromiumWin::paintTextFieldInternal() not to hide background image.
Summary: Fix RenderThemeChromiumWin::paintTextFieldInternal() not to hide background i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P2 Normal
Assignee: Takeshi Yoshino
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-12 16:40 PDT by Takeshi Yoshino
Modified: 2009-06-05 08:06 PDT (History)
3 users (show)

See Also:


Attachments
Proposed fix for 25742 (3.35 KB, patch)
2009-05-12 17:58 PDT, Takeshi Yoshino
eric: review-
Details | Formatted Diff | Diff
Proposed fix for 25742 (rev 2) (3.31 KB, patch)
2009-05-13 11:37 PDT, Takeshi Yoshino
eric: review-
Details | Formatted Diff | Diff
Layout test (1.15 KB, text/html)
2009-05-19 05:10 PDT, Takeshi Yoshino
no flags Details
Expected results for layout test on Mac (41.48 KB, application/x-gzip)
2009-05-19 05:28 PDT, Takeshi Yoshino
no flags Details
Proposed fix for 25742 (rev 3) (10.90 KB, patch)
2009-05-19 21:23 PDT, Takeshi Yoshino
eric: review+
Details | Formatted Diff | Diff
Patch for rebaselining the expected image (68.51 KB, patch)
2009-06-04 22:30 PDT, Takeshi Yoshino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takeshi Yoshino 2009-05-12 16:40:18 PDT
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.
Comment 1 Takeshi Yoshino 2009-05-12 17:58:37 PDT
Created attachment 30258 [details]
Proposed fix for 25742
Comment 2 Eric Seidel (no email) 2009-05-12 19:14:50 PDT
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.
Comment 3 Takeshi Yoshino 2009-05-13 11:37:33 PDT
Created attachment 30281 [details]
Proposed fix for 25742 (rev 2)
Comment 4 Takeshi Yoshino 2009-05-13 11:46:39 PDT
(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?
Comment 5 Darin Fisher (:fishd, Google) 2009-05-13 14:22:37 PDT
> 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.
Comment 6 Takeshi Yoshino 2009-05-13 15:16:15 PDT
(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.
Comment 7 Eric Seidel (no email) 2009-05-14 21:53:52 PDT
WebKit tests reside in the LayoutTests directory in your WebKit checkout.  Your WebKit checkout is separate from your Chromium checkout.
Comment 8 Eric Seidel (no email) 2009-05-14 21:54:29 PDT
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.)
Comment 9 Ojan Vafai 2009-05-14 23:17:29 PDT
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.
Comment 10 Takeshi Yoshino 2009-05-17 16:27:03 PDT
(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.
Comment 11 Takeshi Yoshino 2009-05-19 05:10:01 PDT
Created attachment 30466 [details]
Layout test

Layout test draft. To be added to the patch with Leopard result and Chromium result.
Comment 12 Takeshi Yoshino 2009-05-19 05:28:24 PDT
Created attachment 30467 [details]
Expected results for layout test on Mac
Comment 13 Eric Seidel (no email) 2009-05-19 05:39:39 PDT
I'm surprised there aren't already layout tests which cover backgrounds on forms.  But maybe not?  In either case, the test looks fine.
Comment 14 Takeshi Yoshino 2009-05-19 06:40:58 PDT
(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.
Comment 15 Takeshi Yoshino 2009-05-19 21:23:16 PDT
Created attachment 30494 [details]
Proposed fix for 25742 (rev 3)

Added new test cases and expected results for them.
Comment 16 Eric Seidel (no email) 2009-05-19 22:58:16 PDT
Comment on attachment 30494 [details]
Proposed fix for 25742 (rev 3)

Fantastic.  I'll land this right now.
Comment 17 Takeshi Yoshino 2009-05-19 23:09:56 PDT
(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.
Comment 18 Eric Seidel (no email) 2009-05-20 05:34:19 PDT
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
Comment 19 Eric Seidel (no email) 2009-05-20 05:39:23 PDT
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.
Comment 20 Eric Seidel (no email) 2009-05-20 05:42:41 PDT
Fixed bug urls:
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	WebCore/ChangeLog
Committed r43906
Comment 21 Takeshi Yoshino 2009-05-20 18:57:27 PDT
(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.
Comment 22 Takeshi Yoshino 2009-05-20 22:47:01 PDT
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,
Comment 23 Takeshi Yoshino 2009-06-04 22:30:14 PDT
Created attachment 30993 [details]
Patch for rebaselining the expected image

Created a patch for rebaselining the expected image.
Comment 24 Eric Seidel (no email) 2009-06-05 00:00:57 PDT
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.
Comment 25 Takeshi Yoshino 2009-06-05 00:30:14 PDT
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.

Comment 26 Takeshi Yoshino 2009-06-05 00:34:47 PDT
(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