Summary: | [Mac] REGRESSION (r110480): Text field that specifies background-color (or is auto-filled) gets un-themed border | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||
Component: | Layout and Rendering | Assignee: | Beth Dakin <bdakin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bdakin, eric, phiw2, webkit.review.bot | ||||
Priority: | P1 | Keywords: | InRadar, Regression | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Mac | ||||||
OS: | Unspecified | ||||||
URL: | data:text/html,%3Cinput%20style=%22background-color:%20lightyellow;%22%3E | ||||||
Attachments: |
|
Description
mitz
2012-03-24 12:03:15 PDT
The change made us consider the text field to be “styled”. Created attachment 141208 [details]
Patch
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. 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. Thanks Maciej! I addressed all of your comments. http://trac.webkit.org/changeset/116697 |