57818 – [Chromium]Change menu list background fallback value to transparent to work background:none for HTML select tag.

RESOLVED FIXED 57818

[Chromium]Change menu list background fallback value to transparent to work background:none for HTML select tag.

https://bugs.webkit.org/show_bug.cgi?id=57818

Summary [Chromium]Change menu list background fallback value to transparent to work b...

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	Details Formatted Diff Diff
Patch (5.56 KB, patch) 2011-04-06 12:02 PDT, Naoki Takano	no flags	Details Formatted Diff Diff
Patch (6.04 KB, patch) 2011-04-10 01:44 PDT, Naoki Takano	no flags	Details Formatted Diff Diff
Patch (9.92 KB, patch) 2011-04-12 23:25 PDT, Naoki Takano	no flags	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Other

OS OS X 10.5

Product WebKit

Component New Bugs

Assignee

Nobody

Reported

2011-04-04 23:47 PDT

Modified

2011-04-14 19:09 PDT History

CC List

8 users Show

URL

Keywords

Depends on

Blocks