RESOLVED FIXED 82131
[Mac] REGRESSION (r110480): Text field that specifies background-color (or is auto-filled) gets un-themed border
https://bugs.webkit.org/show_bug.cgi?id=82131
Summary [Mac] REGRESSION (r110480): Text field that specifies background-color (or is...
mitz
Reported 2012-03-24 12:03:15 PDT
After <http://trac.webkit.org/r110480> (the fix for bug 80888), text fields lose their Mac look if they specify a background color.
Attachments
Patch (8.05 KB, patch)
2012-05-10 11:32 PDT, Beth Dakin
mjs: review+
mitz
Comment 1 2012-03-24 12:03:49 PDT
mitz
Comment 2 2012-03-24 12:06:12 PDT
The change made us consider the text field to be “styled”.
Beth Dakin
Comment 3 2012-05-10 11:32:31 PDT
Beth Dakin
Comment 4 2012-05-10 11:40:08 PDT
Comment on attachment 141208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141208&action=review > Source/WebCore/rendering/RenderThemeMac.mm:127 > + NSLog(@"here"); Oops! I removed this.
Maciej Stachowiak
Comment 5 2012-05-10 14:48:24 PDT
Comment on attachment 141208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141208&action=review r=me, but consider the style comments above and don't forget to take out the NSLog. > Source/WebCore/rendering/RenderThemeMac.mm:124 > + // FIXME: This is a post-Lion workaround for <rdar://problem/11385461>. When that bug is resolved, we should remove > + // this code. In the meantime, this allows up to opt into a version of [NSTextField drawWithFrame:] that will only > + // draw the frame of the text field; without this, it will draw the frame and the background, which creates a number of > + // problems with styled text fields and text fields in HiDPI. There is a less comprehesive workaround for Lion and > + // SnowLeopard in place in RenderThemeMac::textField(). This comment is a bit long, any way to get the point across more succinctly? I think the key point is that this is a temporary workaround and in principle should not be required to turn off background drawing. > Source/WebCore/rendering/RenderThemeMac.mm:748 > + // See comment in RenderThemeMac::textField() for a complete explanation of this. In short, > + // on Lion and SnowLeopard, we only want to use the new style gradient for completely unstyled > + // text fields in HiDPI. isControledStyle(), however, treats all text fields that do not have > + // custom borders as "unstyled" to avoid using the CSS border in that case, so we have to sniff > + // around for other types of styling. This also seems a bit verbose, and I'm still not clear just from this comment why we special case based on scale factor. I would suggest just pointing to the other comment. > Source/WebCore/rendering/RenderThemeMac.mm:2183 > +#if !defined(BUILDING_ON_LION) && !defined(BUILDING_ON_SNOW_LEOPARD) I would suggest reversing the polarity of the #if to match the others in this patch. > Source/WebCore/rendering/RenderThemeMac.mm:2199 > + // This is a workaround for <rdar://problem/11385461> on Lion and SnowLeopard. Newer versions of the > + // OS can always use the newer version of the text field with the workaround above in > + // _coreUIDrawOptionsWithFrame. With this workaround for older OS's, when the deviceScaleFactor is 1, > + // we have an old-school gradient bezel in text fields whether they are styled or not. This is fine and > + // matches shipping Safari. When the deviceScaleFactor is greater than 1, text fields will have newer, > + // AppKit-matching gradients that look much more appropriate at the higher resolutions. However, if the > + // text field is styled in any way, we'll revert to the old-school bezel, which doesn't look great in > + // HiDPI, but it looks better than the CSS border, which is the only alternative until 11385461 is resolved. I would suggest summarizing a bit more here. Maybe more of the explanation can be in the ChangeLog, since the full context isn't clear anyway in the three separate large-ish comments.
Beth Dakin
Comment 6 2012-05-10 15:58:01 PDT
Thanks Maciej! I addressed all of your comments. http://trac.webkit.org/changeset/116697
Note You need to log in before you can comment on or make changes to this bug.