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.
Created attachment 30258 [details] Proposed fix for 25742
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.
Created attachment 30281 [details] Proposed fix for 25742 (rev 2)
(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?
> 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.
(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.
WebKit tests reside in the LayoutTests directory in your WebKit checkout. Your WebKit checkout is separate from your Chromium checkout.
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.)
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.
(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.
Created attachment 30466 [details] Layout test Layout test draft. To be added to the patch with Leopard result and Chromium result.
Created attachment 30467 [details] Expected results for layout test on Mac
I'm surprised there aren't already layout tests which cover backgrounds on forms. But maybe not? In either case, the test looks fine.
(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.
Created attachment 30494 [details] Proposed fix for 25742 (rev 3) Added new test cases and expected results for them.
Comment on attachment 30494 [details] Proposed fix for 25742 (rev 3) Fantastic. I'll land this right now.
(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.
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
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.
Fixed bug urls: Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog M WebCore/ChangeLog Committed r43906
(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.
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,
Created attachment 30993 [details] Patch for rebaselining the expected image Created a patch for rebaselining the expected image.
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.
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.
(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
http://trac.webkit.org/changeset/43903 http://trac.webkit.org/changeset/43904 http://trac.webkit.org/changeset/43906