Bug 75860

Summary: [Chromium Mac] no background is drawn for input elements
Product: WebKit Reporter: Cary Clark <caryclark>
Component: Layout and RenderingAssignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, avi, dglazkov, dslomov, mark, rsesek, sullivan, thakis, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
URL: data:text/html;charset=utf-8,<input style="background-color:blue"/>
Attachments:
Description Flags
patch dglazkov: review+, webkit.review.bot: commit-queue-

Cary Clark
Reported 2012-01-09 08:32:53 PST
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)
Attachments
patch (2.55 KB, patch)
2012-01-09 10:55 PST, Avi Drissman 		dglazkov: review+
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Dimitri Glazkov (Google)
Comment 1 2012-01-09 08:52:35 PST
I checked, Safari port is not affected. Chromium bug here: code.google.com/p/chromium/issues/detail?id=109567
Avi Drissman
Comment 2 2012-01-09 09:35:41 PST
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?)
Avi Drissman
Comment 3 2012-01-09 09:45:51 PST
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.
Dimitri Glazkov (Google)
Comment 4 2012-01-09 09:50:55 PST
(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
Nico Weber
Comment 5 2012-01-09 10:00:54 PST
Bug 75742 was caused by this revision as well.
Dimitri Glazkov (Google)
Comment 6 2012-01-09 10:10:51 PST
(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.
Nico Weber
Comment 7 2012-01-09 10:26:47 PST
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).
Avi Drissman
Comment 8 2012-01-09 10:32:45 PST
(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.
Avi Drissman
Comment 9 2012-01-09 10:55:27 PST
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.)
WebKit Review Bot
Comment 10 2012-01-09 11:01:33 PST
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.
Nico Weber
Comment 11 2012-01-09 11:02:56 PST
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? :-)
Nico Weber
Comment 12 2012-01-09 11:26:10 PST
*** Bug 75742 has been marked as a duplicate of this bug. ***
Avi Drissman
Comment 13 2012-01-09 11:28:23 PST
(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.
Avi Drissman
Comment 14 2012-01-09 11:45:28 PST
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.
WebKit Review Bot
Comment 15 2012-01-09 13:21:57 PST
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
Adam Barth
Comment 16 2012-01-09 13:23:38 PST
Found no modified ChangeLogs, cannot create a commit message.
Adam Barth
Comment 17 2012-01-09 13:24:32 PST
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.
Avi Drissman
Comment 18 2012-01-09 13:25:16 PST
(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.
Adam Barth
Comment 19 2012-01-09 15:13:06 PST
Committed r104494: <http://trac.webkit.org/changeset/104494>
Avi Drissman
Comment 20 2012-01-09 16:52:35 PST
(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.
Adam Barth
Comment 21 2012-01-09 17:08:11 PST
No problem. The issue was just that the patch was incorrectly based. We use -p1 but the patch was using -p0.
Note You need to log in before you can comment on or make changes to this bug.