WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
mitz
Comment 1
2012-03-24 12:03:49 PDT
<
rdar://problem/11115221
>
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
Created
attachment 141208
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug