Bug 149843 – Backgrounds bleed out of natively rendered text fields

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

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.

[Wenson Hsieh]

<rdar://problem/22896977>

[Wenson Hsieh]

Created attachment 262519 [details]
Patch

[Wenson Hsieh]

Created attachment 262666 [details]
Patch

[Said Abou-Hallawa]

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()".

[Wenson Hsieh]

Created attachment 262696 [details]
Patch

[alan]

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) ?

[Darin Adler]

Comment on attachment 262696 [details]
Patch

Can we make a regression test for this please?

[Wenson Hsieh]

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.

[Wenson Hsieh]

(In reply to comment #7)
> Comment on attachment 262696 [details]
> Patch
> 
> Can we make a regression test for this please?

Will do!

[Wenson Hsieh]

Created attachment 262702 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Wenson Hsieh]

Created attachment 262705 [details]
Patch

[Darin Adler]

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.

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 262727 [details]
Patch

[Wenson Hsieh]

Committed r190800: <http://trac.webkit.org/changeset/190800>

[Brent Fulgham]

This change introduced a test failure on Windows. It may just need a Windows-specific baseline:

fast/forms/hidpi-textfield-background-bleeding.html

[Wenson Hsieh]

Sorry about that! This test only affects Mac platforms. I'll update the windows TestExpectations to reflect this.

[Brent Fulgham]

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.