[WinCairo] Text box backgrounds do not render in partially opaque layers

[Martin Robinson]

Even after the fixes here ( https://bugs.webkit.org/show_bug.cgi?id=41111 ), text box backgrounds do not render in partially opaque layers. The reason is that the internal Windows theme drawing code is rendering the text box background with GDI. The HDC that it is rendering to is backed by an HBITMAP with an alpha layer. Since GDI is malevolently alpha unaware, rendering onto such an HDC will peg alpha values to 0, meaning everything on the context is fully transparent. Controls other than text boxes (checkboxes and radio buttons, for instance) render in an alpha-aware fashion.

The CoreGraphics port handles this issue via a special image surface type which ignores the alpha channel. There is no such thing in Cairo, as far as I can tell, so the solution is to manually adjust the alpha values on Windows surfaces not supporting alpha-blending to 255 (fully opaque).

[Martin Robinson]

Created attachment 59581 [details]
Test case demonstrating this issue

[Martin Robinson]

Created attachment 59582 [details]
Rendering of this test in WinLauncher

[Martin Robinson]

Created attachment 59583 [details]
Patch

[Brent Fulgham]

I hadn't noticed this problem before -- this is a great fix!

I only had one minor suggestion:
> +    unsigned char* bytes = reinterpret_cast<unsigned char*>(info.bmBits);
> +    if (!supportAlphaBlend)
> +        for (size_t i = 0; i < info.bmHeight * info.bmWidthBytes; i += 4)
> +            bytes[i + 3] = 255;
> +

Could the byte flipping code be represented as a static function for clarity?  For example:

static void setRGBABitmapAlpha(unsigned char* bytes, size_t length, unsigned char level)
{
    for (size_t i = 0; i < length; i += 4)
        bytes[i + 3] = level;
}

... then your other change would read:
    if (!supportAlphaBlend)
        setRGBABitmapAlpha(bytes, info.bmHeight * info.bmWidthBytes, 255);

Otherwise, looks great!

[Adam Roben (:aroben)]

Comment on attachment 59583 [details]
Patch

> +        Functionality unchanged, no new tests.

This doesn't seem correct. You are changing functionality (by fixing a bug). Can we come up with a test?

> +    // If this context does not support alpha blending, then it may have
> +    // been drawn with GDI functions which always set the alpha channel
> +    // to zero. We need to manually set the bitmap to be fully opaque.
> +    unsigned char* bytes = reinterpret_cast<unsigned char*>(info.bmBits);
> +    if (!supportAlphaBlend)
> +        for (size_t i = 0; i < info.bmHeight * info.bmWidthBytes; i += 4)
> +            bytes[i + 3] = 255;

Our style guidelines say that there should be braces around the body of the if (because it spans multiple lines, even though it's only a single statement).

But I agree with Brent that a function might be clearer.

[Martin Robinson]

(In reply to comment #5)
> This doesn't seem correct. You are changing functionality (by fixing a bug). Can we come up with a test?

Thanks for looking at this patch. I agree after reading your comments that this does deserve a test. Given that the WinCairo port doesn't yet support pixel tests, do you have a suggestion for how best to test this change?

> Our style guidelines say that there should be braces around the body of the if (because it spans multiple lines, even though it's only a single statement).
> But I agree with Brent that a function might be clearer.

Alright, I should have an updated patch tonight.

[Adam Roben (:aroben)]

(In reply to comment #6)
> (In reply to comment #5)
> > This doesn't seem correct. You are changing functionality (by fixing a bug). Can we come up with a test?
> 
> Thanks for looking at this patch. I agree after reading your comments that this does deserve a test. Given that the WinCairo port doesn't yet support pixel tests, do you have a suggestion for how best to test this change?

Hm, that is a tricky issue. But it seems like it should be possible for you to write a test that would show the bug if we did have pixel tests, given that you already have attachment 59581 [details]. I think it's probably best to just land that as a test case.

[Martin Robinson]

Created attachment 59818 [details]
Patch including manual test

[Adam Roben (:aroben)]

Comment on attachment 59818 [details]
Patch including manual test

r=me

[Martin Robinson]

Comment on attachment 59818 [details]
Patch including manual test

Clearing flags on attachment: 59818

Committed r62012: <http://trac.webkit.org/changeset/62012>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.