Bug 82131 - [Mac] REGRESSION (r110480): Text field that specifies background-color (or is auto-filled) gets un-themed border
Summary: [Mac] REGRESSION (r110480): Text field that specifies background-color (or is...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P1 Normal
Assignee: Beth Dakin
URL: data:text/html,%3Cinput%20style=%22ba...
Keywords: InRadar, Regression
Depends on:
Reported: 2012-03-24 12:03 PDT by mitz
Modified: 2012-05-10 15:58 PDT (History)
4 users (show)

See Also:

Patch (8.05 KB, patch)
2012-05-10 11:32 PDT, Beth Dakin
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 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.
Comment 1 mitz 2012-03-24 12:03:49 PDT
Comment 2 mitz 2012-03-24 12:06:12 PDT
The change made us consider the text field to be “styled”.
Comment 3 Beth Dakin 2012-05-10 11:32:31 PDT
Created attachment 141208 [details]
Comment 4 Beth Dakin 2012-05-10 11:40:08 PDT
Comment on attachment 141208 [details]

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 5 Maciej Stachowiak 2012-05-10 14:48:24 PDT
Comment on attachment 141208 [details]

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.
Comment 6 Beth Dakin 2012-05-10 15:58:01 PDT
Thanks Maciej! I addressed all of your comments. http://trac.webkit.org/changeset/116697