Load data:text/html,<input style="background: yellow"> and look closely at the bounds of the text field. Observe that the yellow background bleeds out of the text field's border by 1 device pixel.
<rdar://problem/22896977>
Created attachment 262519 [details] Patch
Created attachment 262666 [details] Patch
Comment on attachment 262666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262666&action=review > Source/WebCore/rendering/RenderBox.cpp:1336 > + paintFillLayers(paintInfo, style().visitedDependentColor(CSSPropertyBackgroundColor), style().backgroundLayers(), adjustedBackgroundPaintRect(paintInfo.context(), paintRect), bleedAvoidance); Should not we make paintBackground() itself virtual and make RenderTextControlSingleLine overrides it? I think RenderTextControlSingleLine:: paintBackground() will only have to make a copy of paintRect, adjust it using shrinkRectByOneDevicePixel() and then call the base class function paintBackground(). I think this approach is clearer since the sequence of calls goes from the derived class "RenderTextControlSingleLine::paintBackground()" to the base class "RenderBox::paintBackground()". Your approach actually flows from the base class "RenderBox::paintBackground()" to the derived class "RenderTextControlSingleLine::adjustedBackgroundPaintRect" and then back to the base class again "RenderBox::paintBackground()"."RenderBox::paintBackground()".
Created attachment 262696 [details] Patch
Comment on attachment 262696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262696&action=review > Source/WebCore/rendering/RenderThemeMac.mm:860 > + adjustedPaintRect.inflateX(LayoutUnit::fromPixel(1) / transform.xScale()); > + adjustedPaintRect.inflateY(LayoutUnit::fromPixel(1) / transform.yScale()); shouldn't that be LayoutUnit::fromPixel(1/deviceScaleFactor) ?
Comment on attachment 262696 [details] Patch Can we make a regression test for this please?
Comment on attachment 262696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262696&action=review >> Source/WebCore/rendering/RenderThemeMac.mm:860 >> + adjustedPaintRect.inflateY(LayoutUnit::fromPixel(1) / transform.yScale()); > > shouldn't that be LayoutUnit::fromPixel(1/deviceScaleFactor) ? the device scale factor is accounted for in the x/yScales.
(In reply to comment #7) > Comment on attachment 262696 [details] > Patch > > Can we make a regression test for this please? Will do!
Created attachment 262702 [details] Patch
Comment on attachment 262702 [details] Patch Attachment 262702 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/258336 New failing tests: fast/forms/textfield-background-bleeding.html
Created attachment 262703 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 262702 [details] Patch Attachment 262702 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/258338 New failing tests: fast/forms/textfield-background-bleeding.html
Created attachment 262704 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 262705 [details] Patch
Comment on attachment 262705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262705&action=review > Source/WebCore/rendering/RenderThemeMac.mm:856 > + FloatRect adjustedPaintRect(r); I think this needs a brief, precise comment explaining why the code does this. The sequence starting on this line is a perfect example of code that is not self explanatory! Maybe something like the change log, but shorter. > Source/WebCore/rendering/RenderThemeMac.mm:860 > + adjustedPaintRect.inflateX(LayoutUnit::fromPixel(1) / transform.xScale()); > + adjustedPaintRect.inflateY(LayoutUnit::fromPixel(1) / transform.yScale()); Does this yield a different result than if you write: adjustedPaintRect.inflateX(1 / transform.xScale()); adjustedPaintRect.inflateY(1 / transform.xScale()); I’m not sure why it’s helpful to involve LayoutUnit since xScale and yScale are already floating point.
Comment on attachment 262705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262705&action=review >> Source/WebCore/rendering/RenderThemeMac.mm:856 >> + FloatRect adjustedPaintRect(r); > > I think this needs a brief, precise comment explaining why the code does this. The sequence starting on this line is a perfect example of code that is not self explanatory! Maybe something like the change log, but shorter. Got it. I'll write a sentence about why this adjustment is necessary, and include a link to the radar. >> Source/WebCore/rendering/RenderThemeMac.mm:860 >> + adjustedPaintRect.inflateY(LayoutUnit::fromPixel(1) / transform.yScale()); > > Does this yield a different result than if you write: > > adjustedPaintRect.inflateX(1 / transform.xScale()); > adjustedPaintRect.inflateY(1 / transform.xScale()); > > I’m not sure why it’s helpful to involve LayoutUnit since xScale and yScale are already floating point. Yes -- just verified that it yields the same result. I'll change it to just use the literal value.
Created attachment 262727 [details] Patch
Committed r190800: <http://trac.webkit.org/changeset/190800>
This change introduced a test failure on Windows. It may just need a Windows-specific baseline: fast/forms/hidpi-textfield-background-bleeding.html
Sorry about that! This test only affects Mac platforms. I'll update the windows TestExpectations to reflect this.
This test won't work on Windows: 1. Windows doesn't support 0.5px widths (waiting for hidpi support). 2. The solid black border used in the "expected" file does not match the style of native Windows edit controls (which has a bevelled border). Since this bug seems very Mac-specific, I'll skip on Windows.