75860 2012-01-09 08:32:53 -0800 [Chromium Mac] no background is drawn for input elements 2012-01-09 17:08:11 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Mac OS X 10.6 RESOLVED FIXED data:text/html;charset=utf-8,<input style="background-color:blue"/> P1 Normal --- 1 caryclark bdakin abarth avi dglazkov dslomov mark rsesek sullivan thakis webkit.review.bot oldest_to_newest 532211 0 caryclark 2012-01-09 08:32:53 -0800 This change: https://bugs.webkit.org/show_bug.cgi?id=75654 breaks <input> elements on Chromium on Mac (all versions). It is broken when using either CoreGraphics or Skia as the graphics engine. The break appears to be connected to these lines in RenderThemeMac.mm (2111) // Setting a clear background on the cell is necessary for CSS-styled backgrounds // to show through. Ideally, there would be a better way to do this. [m_textField.get() setDrawsBackground:YES]; [m_textField.get() setBackgroundColor:[NSColor clearColor]]; Locally changing these to: [m_textField.get() setDrawsBackground:NO]; [m_textField.get() setBackgroundColor:[NSColor whiteColor]]; while incorrect, causes <input> to work (albeit hard-coded to a background of white) 532223 1 dglazkov 2012-01-09 08:52:35 -0800 I checked, Safari port is not affected. Chromium bug here: code.google.com/p/chromium/issues/detail?id=109567 532244 2 avi 2012-01-09 09:35:41 -0800 Cary: I agree with your analysis. I'm curious about the comment re "CSS-styled backgrounds", as this implies a different painting model than us. We're just redrawing the control; does Safari do a full back-to-front repaint? Dmitri: "Safari port is not affected": Does the Safari port use RenderThemeMac, or RenderThemeSafari? (BTW, when is RenderThemeSafari used?) 532246 3 avi 2012-01-09 09:45:51 -0800 We always have the short-term option of overriding paintTextField in RenderThemeChromiumMac and doing it our way if we can't figure out what's going on. BTW, WKDrawBezeledTextFieldCell is just a wrapper over _NSDrawCarbonThemeBezel, so it's not crystal clear what the old behavior was. 532247 4 dglazkov 2012-01-09 09:50:55 -0800 (In reply to comment #2) > Cary: I agree with your analysis. I'm curious about the comment re "CSS-styled backgrounds", as this implies a different painting model than us. We're just redrawing the control; does Safari do a full back-to-front repaint? > > Dmitri: "Safari port is not affected": Does the Safari port use RenderThemeMac, or RenderThemeSafari? (BTW, when is RenderThemeSafari used?) RenderThemeMac -> Safari Mac RenderThemeSafari -> Safari Windows 532254 5 thakis 2012-01-09 10:00:54 -0800 Bug 75742 was caused by this revision as well. 532262 6 dglazkov 2012-01-09 10:10:51 -0800 (In reply to comment #3) > We always have the short-term option of overriding paintTextField in RenderThemeChromiumMac and doing it our way if we can't figure out what's going on. > > BTW, WKDrawBezeledTextFieldCell is just a wrapper over _NSDrawCarbonThemeBezel, so it's not crystal clear what the old behavior was. +1 on a quick and dirty fix for now. 532277 7 thakis 2012-01-09 10:26:47 -0800 I can't repro this when I build on Lion, with the 10.6 SDK. So this problem might be triggered by us using the 10.5 sdk (which we have to, since we ship on 10.5). 532289 8 avi 2012-01-09 10:32:45 -0800 (In reply to comment #7) > So this problem might be triggered by us using the 10.5 sdk (which we have to, since we ship on 10.5). Oh joy, one of those "let's change the behavior of an API call depending on what version you linked". It's reproducible for me with the 10.5 SDK. I'm OK with overriding as a QND fix, but the question is now what to override it with. It would suck if we had to use the SPI again. I'm playing around with explicitly filling the background. 532319 9 121690 avi 2012-01-09 10:55:27 -0800 Created attachment 121690 patch Sigh. This does work, and I'd be OK with it for a quick-n-dirty fix. In the long run, what to do? If the official Cocoa code works with the 10.6 SDK, do we just let this go until that's our minimum? (BTW, this patch is off my Git checkout with some massaging. I hope the CQ can cope.) 532325 10 webkit.review.bot 2012-01-09 11:01:33 -0800 Attachment 121690 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style. 532326 11 121690 thakis 2012-01-09 11:02:56 -0800 Comment on attachment 121690 patch View in context: https://bugs.webkit.org/attachment.cgi?id=121690&action=review Looks fine. Can you check if this fixes fast/forms/input-disabled-color.html (the bug I linked to above) too? > Source/WebCore/rendering/RenderThemeChromiumMac.h:36 > + virtual bool paintTextField(RenderObject*, const PaintInfo&, const IntRect&); OVERRIDE? :-) 532358 12 thakis 2012-01-09 11:26:10 -0800 *** Bug 75742 has been marked as a duplicate of this bug. *** 532361 13 avi 2012-01-09 11:28:23 -0800 (In reply to comment #11) > Looks fine. Can you check if this fixes fast/forms/input-disabled-color.html (the bug I linked to above) too? That was my test to see if it was a functional patch. So yes. > OVERRIDE? :-) I don't consider this code as Chromium code for the purposes of the style guide. 532381 14 avi 2012-01-09 11:45:28 -0800 We might as well leave this fix in until we drop 10.5 support. Poking around the guts of NSTextFieldCell doesn't sound very fun, and since this is apparently fixed with the 10.6 SDK it can join the cruft-we-keep-around-for-Leopard party. 532473 15 121690 webkit.review.bot 2012-01-09 13:21:57 -0800 Comment on attachment 121690 patch Rejecting attachment 121690 from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 10314 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 46>At revision 10314. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/11184655 532476 16 abarth 2012-01-09 13:23:38 -0800 Found no modified ChangeLogs, cannot create a commit message. 532478 17 121690 abarth 2012-01-09 13:24:32 -0800 Comment on attachment 121690 patch Strange. The patch does seem to have a ChangeLog. The style-queue has similar problems. Maybe we should land this manually. 532480 18 avi 2012-01-09 13:25:16 -0800 (In reply to comment #17) > (From update of attachment 121690 [details]) > Strange. The patch does seem to have a ChangeLog. The style-queue has similar problems. Maybe we should land this manually. This was a diff from my git repro. Let me recreate it in a real svn repo and repost. 532553 19 abarth 2012-01-09 15:13:06 -0800 Committed r104494: <http://trac.webkit.org/changeset/104494> 532659 20 avi 2012-01-09 16:52:35 -0800 (In reply to comment #19) > Committed r104494: <http://trac.webkit.org/changeset/104494> Thanks! I had to go, and my WebKit checkout was so old that svn up was still cranking after half an hour. 532675 21 abarth 2012-01-09 17:08:11 -0800 No problem. The issue was just that the patch was incorrectly based. We use -p1 but the patch was using -p0. 121690 2012-01-09 10:55:27 -0800 2012-01-09 13:24:32 -0800 patch x text/plain 2612 avi LS0tIFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZworKysgU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9n CkBAIC0xLDMgKzEsMTcgQEAKKzIwMTItMDEtMDkgIEF2aSBEcmlzc21hbiAgPGF2aUBjaHJvbWl1 bS5vcmc+CisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTc1ODYwCisgICAgICAgIFtDaHJvbWl1bSBNYWNdIG5vIGJhY2tncm91bmQgaXMgZHJhd24gZm9y IGlucHV0IGVsZW1lbnRzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgUmV2ZXJ0cyByMTA0MjQwIGZvciBDaHJvbWl1bS4gVW5mb3J0dW5hdGVseSB0aGUg Y29kZSB0aGF0IHVzZXMgQ29jb2EKKyAgICAgICAgQVBJIG1pc2JlaGF2ZXMgd2hlbiBidWlsdCB3 aXRoIHRoZSAxMC41IFNESywgc28gd2UgdXNlIFNQSS4gRm9yIG5vdy4KKworICAgICAgICAqIHJl bmRlcmluZy9SZW5kZXJUaGVtZUNocm9taXVtTWFjLmg6CisgICAgICAgICogcmVuZGVyaW5nL1Jl bmRlclRoZW1lQ2hyb21pdW1NYWMubW06CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJUaGVtZUNo cm9taXVtTWFjOjpwYWludFRleHRGaWVsZCk6CisKIDIwMTItMDEtMDcgIENocmlzIE1hcnJpbiAg PGNtYXJyaW5AYXBwbGUuY29tPgogCiAgICAgICAgIEZpeGVkIEFOR0xFIGJ1aWxkIGZvciBHTlUg YW5kIFFUIGJyb2tlbiBpbiBodHRwczovL3RyYWMud2Via2l0Lm9yZy9jaGFuZ2VzZXQvMTA0MzYz Ci0tLSBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGhlbWVDaHJvbWl1bU1hYy5oCisr KyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGhlbWVDaHJvbWl1bU1hYy5oCkBAIC0z Myw2ICszMyw3IEBAIHB1YmxpYzoKICAgICBzdGF0aWMgUGFzc1JlZlB0cjxSZW5kZXJUaGVtZT4g Y3JlYXRlKCk7CiAKIHByb3RlY3RlZDoKKyAgICB2aXJ0dWFsIGJvb2wgcGFpbnRUZXh0RmllbGQo UmVuZGVyT2JqZWN0KiwgY29uc3QgUGFpbnRJbmZvJiwgY29uc3QgSW50UmVjdCYpOwogI2lmIEVO QUJMRShWSURFTykKICAgICB2aXJ0dWFsIHZvaWQgYWRqdXN0TWVkaWFTbGlkZXJUaHVtYlNpemUo UmVuZGVyU3R5bGUqKSBjb25zdDsKICAgICB2aXJ0dWFsIGJvb2wgcGFpbnRNZWRpYVBsYXlCdXR0 b24oUmVuZGVyT2JqZWN0KiwgY29uc3QgUGFpbnRJbmZvJiwgY29uc3QgSW50UmVjdCYpOwotLS0g U291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRoZW1lQ2hyb21pdW1NYWMubW0KKysrIFNv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUaGVtZUNocm9taXVtTWFjLm1tCkBAIC0xOSwx MCArMTksMTIgQEAKICAqLwogCiAjaW1wb3J0ICJjb25maWcuaCIKKyNpbXBvcnQgIkxvY2FsQ3Vy cmVudEdyYXBoaWNzQ29udGV4dC5oIgogI2ltcG9ydCAiUmVuZGVyVGhlbWVDaHJvbWl1bU1hYy5o IgogI2ltcG9ydCAiUGFpbnRJbmZvLmgiCiAjaW1wb3J0ICJQbGF0Zm9ybVN1cHBvcnQuaCIKICNp bXBvcnQgIlJlbmRlck1lZGlhQ29udHJvbHNDaHJvbWl1bS5oIgorI2ltcG9ydCAiV2ViQ29yZVN5 c3RlbUludGVyZmFjZS5oIgogI2ltcG9ydCAiVXNlckFnZW50U3R5bGVTaGVldHMuaCIKICNpbXBv cnQgPENhcmJvbi9DYXJib24uaD4KICNpbXBvcnQgPENvY29hL0NvY29hLmg+CkBAIC0xMjUsNiAr MTI3LDIzIEBAIGJvb2wgUmVuZGVyVGhlbWVDaHJvbWl1bU1hYzo6c2hvdWxkU2hvd1BsYWNlaG9s ZGVyV2hlbkZvY3VzZWQoKSBjb25zdAogICAgIHJldHVybiB0cnVlOwogfQogCitib29sIFJlbmRl clRoZW1lQ2hyb21pdW1NYWM6OnBhaW50VGV4dEZpZWxkKFJlbmRlck9iamVjdCogbywgY29uc3Qg UGFpbnRJbmZvJiBwYWludEluZm8sIGNvbnN0IEludFJlY3QmIHIpCit7CisgICAgLy8gVXNpbmcg dGhlIENvY29hIGNvZGUgdG8gcGFpbnQgdGhlIHRleHQgZmllbGQgYXMgUmVuZGVyVGhlbWVNYWMg ZG9lcyBydW5zCisgICAgLy8gaW50byBhIGJ1ZyB3aGVuIGJ1aWxkaW5nIGFnYWluc3QgdGhlIDEw LjUgU0RLLCBzbyB3ZSBhcmUgZm9yY2VkIGhlcmUgdG8KKyAgICAvLyB1c2UgdGhlIFNQSS4gVGhp cyBvdmVycmlkZSBvZiBwYWludFRleHRGaWVsZCBzaG91bGQgYmUgYWJsZSB0byBiZSByZW1vdmVk CisgICAgLy8gd2hlbiAxMC42IGlzIHRoZSBtaW5pbXVtIHJlcXVpcmVkIHN5c3RlbSBhbmQgQ2hy b21pdW0gaXMgYnVpbHQgYWdhaW5zdAorICAgIC8vIHRoZSAxMC42IFNESy4KKyAgICAvLworICAg IC8vIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03NTg2MAorICAgIC8v IGh0dHA6Ly9jb2RlLmdvb2dsZS5jb20vcC9jaHJvbWl1bS9pc3N1ZXMvZGV0YWlsP2lkPTEwOTU2 NworCisgICAgTG9jYWxDdXJyZW50R3JhcGhpY3NDb250ZXh0IGxvY2FsQ29udGV4dChwYWludElu Zm8uY29udGV4dCk7CisgICAgd2tEcmF3QmV6ZWxlZFRleHRGaWVsZENlbGwociwgaXNFbmFibGVk KG8pICYmICFpc1JlYWRPbmx5Q29udHJvbChvKSk7CisKKyAgICByZXR1cm4gZmFsc2U7Cit9CisK ICNpZiBFTkFCTEUoVklERU8pCiAKIHZvaWQgUmVuZGVyVGhlbWVDaHJvbWl1bU1hYzo6YWRqdXN0 TWVkaWFTbGlkZXJUaHVtYlNpemUoUmVuZGVyU3R5bGUqIHN0eWxlKSBjb25zdAo=