Text drawn via -webkit-background-clip:text is blurry at device scale factors >1.0

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

To reproduce:

1. On a display with a device scale factor >1.0, go to data:text/html,%3Cspan%20style=%22background-color:red;%20-webkit-background-clip:text;%20-webkit-text-fill-color:transparent%22%3EThis%20text%20should%20be%20nice%20and%20sharp

The text looks blurry. It should be sharp instead!

[Radar WebKit Bug Importer]

<rdar://problem/10060379>

[Adam Roben (:aroben)]

Created attachment 105984 [details]
screenshot

[Dave Hyatt]

This may be a problem everywhere we use ImageBuffers.

[Beth Dakin]

Created attachment 108254 [details]
Patch

We might want to consider baking the scale factor stuff into the ImageBuffer class. This patch doesn't do that…instead all of the scale factor logic is in RenderBoxModelObject. I still haven't fully wrapped my head around how useful this would be to ImageBuffers in general, so I kept it specific to text clipping at the time. Anyway, input is of course encouraged.

[Simon Fraser (smfr)]

Comment on attachment 108254 [details]
Patch

Yeah, i wonder if we could pass a scale factor into ImageBuffer::create(), and it would make a larger buffer, and set the context scale.

[Darin Adler]

Comment on attachment 108254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108254&action=review

r=me as is even though I have some suggestions for improvement

Is there a good way to make a test for this?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:692
> +        float deviceScaleFactor = Page::deviceScaleFactor(frame());

I’m surprised we made this helper a Page static member, even though I think I possibly suggested putting it in Page.h. I think it could instead just be a namespace-level function that takes a Frame*.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:705
> +        // To create a mask image of the appropriate resolution, we need to scale maskRect's size
> +        // by the device scale factor.
> +        float deviceScaleFactor = Page::deviceScaleFactor(frame());
> +        IntSize scaledMaskSize = maskRect.size();
> +        scaledMaskSize.scale(deviceScaleFactor);
>          
>          // Now create the mask.
> -        OwnPtr<ImageBuffer> maskImage = ImageBuffer::create(maskRect.size());
> +        OwnPtr<ImageBuffer> maskImage = ImageBuffer::create(scaledMaskSize);
>          if (!maskImage)
>              return;
>          
>          GraphicsContext* maskImageContext = maskImage->context();
> +
> +        // Scale the whole context by the device scale factor so that all of the clips set up at 
> +        // the appropriate size.
> +        maskImageContext->scale(FloatSize(deviceScaleFactor, deviceScaleFactor));

It seems to me that without adding a feature to ImageBuffer where it knows its scale factor, we could create a single helper function that creates an image buffer given a size and a scale factor. It would do all the steps here: Increase the size by the scale factor, create the buffer, scale the context.

[Beth Dakin]

(In reply to comment #6)
> (From update of attachment 108254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108254&action=review
> 
> r=me as is even though I have some suggestions for improvement
> 
> Is there a good way to make a test for this?
> 

Not at this time. I have not yet added HiDPI-testing ability to WebKitTestRunner, though I have talked about doing so extensively with Sam. 

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:692
> > +        float deviceScaleFactor = Page::deviceScaleFactor(frame());
> 
> I’m surprised we made this helper a Page static member, even though I think I possibly suggested putting it in Page.h. I think it could instead just be a namespace-level function that takes a Frame*.
> 

Okay.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:705
> > +        // To create a mask image of the appropriate resolution, we need to scale maskRect's size
> > +        // by the device scale factor.
> > +        float deviceScaleFactor = Page::deviceScaleFactor(frame());
> > +        IntSize scaledMaskSize = maskRect.size();
> > +        scaledMaskSize.scale(deviceScaleFactor);
> >          
> >          // Now create the mask.
> > -        OwnPtr<ImageBuffer> maskImage = ImageBuffer::create(maskRect.size());
> > +        OwnPtr<ImageBuffer> maskImage = ImageBuffer::create(scaledMaskSize);
> >          if (!maskImage)
> >              return;
> >          
> >          GraphicsContext* maskImageContext = maskImage->context();
> > +
> > +        // Scale the whole context by the device scale factor so that all of the clips set up at 
> > +        // the appropriate size.
> > +        maskImageContext->scale(FloatSize(deviceScaleFactor, deviceScaleFactor));
> 
> It seems to me that without adding a feature to ImageBuffer where it knows its scale factor, we could create a single helper function that creates an image buffer given a size and a scale factor. It would do all the steps here: Increase the size by the scale factor, create the buffer, scale the context.

Do you envision this helper function being a helper in RenderBoxModel object or elsewhere? Just want to clarify.

[Beth Dakin]

> 
> Do you envision this helper function being a helper in RenderBoxModel object or elsewhere? Just want to clarify.

Darin and I discussed this in person. We decided that it should just be a static function in RenderBoxModelObject for the time being, and if we find we need to functionality in a different class at a later time, we will move to to an appropriate shared location.

[Beth Dakin]

Committed change with Darin's suggested changes. Revision 95697.

[Adam Roben (:aroben)]

Comment on attachment 108254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108254&action=review

>>> Source/WebCore/rendering/RenderBoxModelObject.cpp:692
>>> +        float deviceScaleFactor = Page::deviceScaleFactor(frame());
>> 
>> I’m surprised we made this helper a Page static member, even though I think I possibly suggested putting it in Page.h. I think it could instead just be a namespace-level function that takes a Frame*.
> 
> Okay.

Do we need to take the page scale factor into account? Or maybe that's already been handled for us? Otherwise we'll get blurry text if you full-page zoom.

[Darin Adler]

From my testing it does seem that we would get improved results if page scale factor was also taken into account. I don’t think that’s covered by this bug, but it’s a simple refinement we could make.

[Simon Fraser (smfr)]

(In reply to comment #11)
> From my testing it does seem that we would get improved results if page scale factor was also taken into account. I don’t think that’s covered by this bug, but it’s a simple refinement we could make.

Not only the page scale factor, but also scale transforms on ancestors, no? Maybe the right thing to do is extract the scale component of the CTM or something (SVG does this in a number of places).

[Beth Dakin]

Good thoughts all around. Here's the follow-up bug: https://bugs.webkit.org/show_bug.cgi?id=68641