Bug 82131 – [Mac] REGRESSION (r110480): Text field that specifies background-color (or is auto-filled) gets un-themed border

• Component: Layout and Rendering
• Status: RESOLVED FIXED

After <http://trac.webkit.org/r110480> (the fix for bug 80888), text fields lose their Mac look if they specify a background color.

<rdar://problem/11115221>

The change made us consider the text field to be “styled”.

[Beth Dakin]

Created attachment 141208 [details]
Patch

[Beth Dakin]

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 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]

Thanks Maciej! I addressed all of your comments. http://trac.webkit.org/changeset/116697