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)
I checked, Safari port is not affected. Chromium bug here: code.google.com/p/chromium/issues/detail?id=109567
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?)
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.
(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
Bug 75742 was caused by this revision as well.
(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.
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).
(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.
Created attachment 121690 [details] 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.)
Attachment 121690 [details] 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.
Comment on attachment 121690 [details] 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? :-)
*** Bug 75742 has been marked as a duplicate of this bug. ***
(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.
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.
Comment on attachment 121690 [details] patch Rejecting attachment 121690 [details] 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
Found no modified ChangeLogs, cannot create a commit message.
Comment on attachment 121690 [details] patch Strange. The patch does seem to have a ChangeLog. The style-queue has similar problems. Maybe we should land this manually.
(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.
Committed r104494: <http://trac.webkit.org/changeset/104494>
(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.
No problem. The issue was just that the patch was incorrectly based. We use -p1 but the patch was using -p0.