Bug 57818

Summary: [Chromium]Change menu list background fallback value to transparent to work background:none for HTML select tag.
Product: WebKit Reporter: Naoki Takano <honten>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, evan, honten, isherman, tkent, tony, xiyuan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from cr-jail-3
none
Patch none

Naoki Takano
Reported 2011-04-04 23:47:28 PDT
[Chromium]Change menu list background fallback value to transparent to work background:none for HTML select tag.
Attachments
Patch (5.91 KB, patch)
2011-04-04 23:48 PDT, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Patch (5.56 KB, patch)
2011-04-06 12:02 PDT, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Patch (6.04 KB, patch)
2011-04-10 01:44 PDT, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Patch (9.92 KB, patch)
2011-04-12 23:25 PDT, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from cr-jail-3 (49.61 KB, application/zip)
2011-04-13 09:24 PDT, WebKit Commit Bot 		no flags
Details
Patch (10.82 KB, patch)
2011-04-13 19:29 PDT, Naoki Takano 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Naoki Takano
Comment 1 2011-04-04 23:48:23 PDT
Created attachment 88187 [details] Patch
Naoki Takano
Comment 2 2011-04-04 23:50:43 PDT
Still the patch is not matured. But I want to know the direction is correct or not. Windows has the same problem, so once Linux is fine, I will fix Windows.
Eric Seidel (no email)
Comment 3 2011-04-06 11:06:10 PDT
Comment on attachment 88187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88187&action=review Ideally test cases shoudl be reduced as much as possible. > LayoutTests/fast/forms/select-background-none.html:1 > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> Not needed (or really wanted). > LayoutTests/fast/forms/select-background-none.html:13 > +<html xmlns="http://www.w3.org/1999/xhtml"> > +<head> > +<meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> > +<style type="text/css"> > +body{ > + background:#666; > +} > +.test{ > + background:none; > +} > +</style> > +</head> Most of this isn't needed. > LayoutTests/fast/forms/select-background-none.html:19 > + <option>2</option> > + <option>3</option> > + <option>4</option> Only really need one.
Naoki Takano
Comment 4 2011-04-06 11:07:51 PDT
Thanks, Eric. How about C++ side? (In reply to comment #3) > (From update of attachment 88187 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88187&action=review > > Ideally test cases shoudl be reduced as much as possible. > > > LayoutTests/fast/forms/select-background-none.html:1 > > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> > > Not needed (or really wanted). > > > LayoutTests/fast/forms/select-background-none.html:13 > > +<html xmlns="http://www.w3.org/1999/xhtml"> > > +<head> > > +<meta http-equiv="Content-Type" content="text/html; charset=utf-8" /> > > +<style type="text/css"> > > +body{ > > + background:#666; > > +} > > +.test{ > > + background:none; > > +} > > +</style> > > +</head> > > Most of this isn't needed. > > > LayoutTests/fast/forms/select-background-none.html:19 > > + <option>2</option> > > + <option>3</option> > > + <option>4</option> > > Only really need one.
Naoki Takano
Comment 5 2011-04-06 12:02:53 PDT
Created attachment 88481 [details] Patch
Naoki Takano
Comment 6 2011-04-06 12:03:10 PDT
Comment on attachment 88481 [details] Patch Please review again.
Naoki Takano
Comment 7 2011-04-08 13:57:06 PDT
Could you please review my patch?
Eric Seidel (no email)
Comment 8 2011-04-08 13:58:55 PDT
Comment on attachment 88481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88481&action=review In either case, this looks fine. > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:278 > + extraParams.menuList.backgroundColor = SkColorSetARGB(0, 0, 0, 0); Do we not have a named SkColor for this? Color::transparent would be the WebCore way.
Naoki Takano
Comment 9 2011-04-08 14:01:54 PDT
I see. I'll do that. (In reply to comment #8) > (From update of attachment 88481 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88481&action=review > > In either case, this looks fine. > > > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:278 > > + extraParams.menuList.backgroundColor = SkColorSetARGB(0, 0, 0, 0); > > Do we not have a named SkColor for this? Color::transparent would be the WebCore way.
Eric Seidel (no email)
Comment 10 2011-04-08 14:03:32 PDT
To be clear: I don't need to see the change agian. As is is fine, or changed your call. If you want to use the cq, you can just fill in the reviewer line and upload a patch marked with cq? (or cq+ if your'e a committer) and anyone can cq+ it without further review.
Naoki Takano
Comment 11 2011-04-08 14:05:55 PDT
Cool. But actually, as I wrote here, it is Ok for Linux but not for Win. Because Windows has the same problem. Or should I make another patch for Windows? Thanks, (In reply to comment #10) > To be clear: I don't need to see the change agian. As is is fine, or changed your call. If you want to use the cq, you can just fill in the reviewer line and upload a patch marked with cq? (or cq+ if your'e a committer) and anyone can cq+ it without further review.
Eric Seidel (no email)
Comment 12 2011-04-08 14:07:25 PDT
You could fix both in one patch, assuming your test case works for both. Up to you. :)
Naoki Takano
Comment 13 2011-04-08 14:14:36 PDT
Yes, But I have to generate Windows test results because it includes font information... Can I ask you to generate expected results, once I'm ready for Windows patch? BTW, are you in #webkit irc channel? I can ping you if I'm ready. Thanks, (In reply to comment #12) > You could fix both in one patch, assuming your test case works for both. Up to you. :)
Tony Chang
Comment 14 2011-04-08 15:05:37 PDT
Does this change the color of the background when the drop down menu is open? What happens if background is not specified for the <select>?
Naoki Takano
Comment 15 2011-04-08 15:07:32 PDT
No, this change affects only closed menu. If there no background, the original default color is specified, not transparent color. (In reply to comment #14) > Does this change the color of the background when the drop down menu is open? > > What happens if background is not specified for the <select>?
Naoki Takano
Comment 16 2011-04-10 01:44:22 PDT
Created attachment 88948 [details] Patch
Naoki Takano
Comment 17 2011-04-10 01:46:26 PDT
Comment on attachment 88948 [details] Patch As Eris Seidel wrote here, he doesn't need review again. Originally I wanted to add Windows fix, but not so easy compared to Linux, so I'll work on with separate patch. Please commit.
Eric Seidel (no email)
Comment 18 2011-04-10 07:24:03 PDT
Comment on attachment 88948 [details] Patch I'm entertained that you reverted back to your earlier test case. Looks fine.
WebKit Commit Bot
Comment 19 2011-04-10 10:03:54 PDT
Comment on attachment 88948 [details] Patch Rejecting attachment 88948 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: mlmp . http/tests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ............ 751.63s total testing time 23211 test cases (99%) succeeded 1 test case (<1%) was new 16 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8381530
Naoki Takano
Comment 20 2011-04-10 11:21:46 PDT
Eric, Thank you for your commit. But it looks failed. I guess the patch option looks wrong, right? Because I added new test and result (only Linux though) but the option specified Failed to run "['Tools/Scripts/run-webkit-tests', '--no-new-test-results', '--no-launch-safari', '--exit-after-n..." exit_code: 1 Could you give me your thought? Thanks, (In reply to comment #19) > (From update of attachment 88948 [details]) > Rejecting attachment 88948 [details] from commit-queue. > > Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 > > Last 500 characters of output: > mlmp . > http/tests/xmlhttprequest ................................................................................................................................................................................ > http/tests/xmlhttprequest/web-apps ............... > http/tests/xmlhttprequest/workers ........... > http/tests/xmlviewer . > http/tests/xmlviewer/dumpAsText ............ > 751.63s total testing time > > 23211 test cases (99%) succeeded > 1 test case (<1%) was new > 16 test cases (<1%) had stderr output > > Full output: http://queues.webkit.org/results/8381530
Eric Seidel (no email)
Comment 21 2011-04-10 17:29:05 PDT
Your patch doesn't have any results for the Mac port. So the commit-queue can't land it (since it would make the mac bot turn red): fast/forms/select-background-none.html -> new
Naoki Takano
Comment 22 2011-04-10 17:32:39 PDT
Ok, Maybe I can generate Mac expected results but how about Windows? As I wrote here, Windows still has problem. Do I have to treat special care for Windows? Otherwise, doesn't it fail again? (In reply to comment #21) > Your patch doesn't have any results for the Mac port. So the commit-queue can't land it (since it would make the mac bot turn red): > > fast/forms/select-background-none.html -> new
Eric Seidel (no email)
Comment 23 2011-04-10 17:35:56 PDT
We don't want to land patches which would make the bots red. So if you expect your patch to do that (which it sounds like you do), it's not ready for landing. :)
Naoki Takano
Comment 24 2011-04-10 17:39:00 PDT
I see. But my Windows dev environment is extremely slow... So it is very painful to debug on Windows. Anyway, I want to continue to figure out the windows problem also. Thanks, (In reply to comment #23) > We don't want to land patches which would make the bots red. So if you expect your patch to do that (which it sounds like you do), it's not ready for landing. :)
Kent Tamura
Comment 25 2011-04-12 00:12:15 PDT
Takano-san, You need a Mac result in the patch because commit-queue runs on Mac. You don't need a Windows result at this moment. Port maintainers will commit Windows results later.
Naoki Takano
Comment 26 2011-04-12 00:15:15 PDT
Kent-san, Thank you for your comment. So can I land my patch without Windows porting? That's great news for me;-) Tomorrow, I'll do that. Thanks, (In reply to comment #25) > Takano-san, > You need a Mac result in the patch because commit-queue runs on Mac. You don't need a Windows result at this moment. Port maintainers will commit Windows results later.
Kent Tamura
Comment 27 2011-04-12 00:22:38 PDT
(In reply to comment #26) > So can I land my patch without Windows porting? Yes. I think we may land a test without non-Mac results. Ideally we should land a test with results of all platforms. But it's almost impossible.
Naoki Takano
Comment 28 2011-04-12 23:25:09 PDT
Created attachment 89344 [details] Patch
Naoki Takano
Comment 29 2011-04-12 23:25:55 PDT
Comment on attachment 89344 [details] Patch I added mac expected results. Please try commit again. Thanks,
Eric Seidel (no email)
Comment 30 2011-04-13 06:15:49 PDT
Comment on attachment 89344 [details] Patch These only have the chromium-mac expectations, not the apple mac or "mac" expectations, so the cq will fail again. However all non-chromium ports don't bother to run the pixel tests, so all you need are the -expected.txt results, which I would expect to be identical between mac and chromium mac. As an experiment, I'm going to cq+ this, as the cq should now be smart enough to upload the results.zip file including the failure information for you (as of yesterday).
WebKit Commit Bot
Comment 31 2011-04-13 09:23:59 PDT
Comment on attachment 89344 [details] Patch Rejecting attachment 89344 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2 Last 500 characters of output: mlmp . http/tests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ............ 723.47s total testing time 23260 test cases (99%) succeeded 1 test case (<1%) was new 16 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8399298
WebKit Commit Bot
Comment 32 2011-04-13 09:24:03 PDT
Created attachment 89387 [details] Archive of layout-test-results from cr-jail-3 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: cr-jail-3 Port: Mac Platform: Mac OS X 10.6.6
Tony Chang
Comment 33 2011-04-13 10:16:20 PDT
(In reply to comment #32) > Created an attachment (id=89387) [details] > Archive of layout-test-results from cr-jail-3 > > The attached test failures were seen while running run-webkit-tests on the commit-queue. > Bot: cr-jail-3 Port: Mac Platform: Mac OS X 10.6.6 This failed because the (Apple) mac bot wasn't able to find results for the new test. I suspect the chromium-mac results should be put in platform/mac, although you might want to verify that the tests are in fact the same with mac webkit.
Eric Seidel (no email)
Comment 34 2011-04-13 10:40:29 PDT
So exiciting! My new code works! That's soooo useful!
Naoki Takano
Comment 35 2011-04-13 10:46:07 PDT
Ok, I'll copy only select-background-none-expected.txt into platform/mac/fast/forms/. Is this correct? I assumed "mac" meant "chromium-mac", but there is another "mac" inside... Thanks, (In reply to comment #34) > So exiciting! My new code works! That's soooo useful! (In reply to comment #30) > (From update of attachment 89344 [details]) > These only have the chromium-mac expectations, not the apple mac or "mac" expectations, so the cq will fail again. > > However all non-chromium ports don't bother to run the pixel tests, so all you need are the -expected.txt results, which I would expect to be identical between mac and chromium mac. > > As an experiment, I'm going to cq+ this, as the cq should now be smart enough to upload the results.zip file including the failure information for you (as of yesterday).
Kent Tamura
Comment 36 2011-04-13 18:42:02 PDT
(In reply to comment #35) > I'll copy only select-background-none-expected.txt into platform/mac/fast/forms/. Is this correct? Yes. Please copy select-background-none-actual.txt in Attachment #89387 [details] into platform/mac/fast/forms/select-background-none-expected.txt.
Naoki Takano
Comment 37 2011-04-13 19:29:41 PDT
Created attachment 89518 [details] Patch
Naoki Takano
Comment 38 2011-04-13 19:30:19 PDT
Comment on attachment 89518 [details] Patch Please commit again. I hope this is the last try!!
WebKit Review Bot
Comment 39 2011-04-13 19:32:28 PDT
Comment on attachment 89518 [details] Patch Rejecting attachment 89518 [details] from review queue. takano.naoki@gmail.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
Naoki Takano
Comment 40 2011-04-14 11:11:18 PDT
Hmm... The patch is not landed. Is the bot very busy?
WebKit Commit Bot
Comment 41 2011-04-14 19:06:07 PDT
The commit-queue encountered the following flaky tests while processing attachment 89518 [details]: http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: alice.liu@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 42 2011-04-14 19:09:19 PDT
Comment on attachment 89518 [details] Patch Clearing flags on attachment: 89518 Committed r83930: <http://trac.webkit.org/changeset/83930>
WebKit Commit Bot
Comment 43 2011-04-14 19:09:25 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.