Bug 67415 - Text drawn via -webkit-background-clip:text is blurry at device scale factors >1.0
Summary: Text drawn via -webkit-background-clip:text is blurry at device scale factors...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.7
: P2 Normal
Assignee: Beth Dakin
URL: data:text/html,%3Cspan%20style=%22bac...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-09-01 10:03 PDT by Adam Roben (:aroben)
Modified: 2011-09-22 11:02 PDT (History)
6 users (show)

See Also:


Attachments
screenshot (13.48 KB, image/png)
2011-09-01 10:04 PDT, Adam Roben (:aroben)
no flags Details
Patch (2.58 KB, patch)
2011-09-21 16:51 PDT, Beth Dakin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-09-01 10:03:51 PDT
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!
Comment 1 Radar WebKit Bug Importer 2011-09-01 10:04:11 PDT
<rdar://problem/10060379>
Comment 2 Adam Roben (:aroben) 2011-09-01 10:04:45 PDT
Created attachment 105984 [details]
screenshot
Comment 3 Dave Hyatt 2011-09-02 08:53:17 PDT
This may be a problem everywhere we use ImageBuffers.
Comment 4 Beth Dakin 2011-09-21 16:51:05 PDT
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.
Comment 5 Simon Fraser (smfr) 2011-09-21 16:59:56 PDT
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.
Comment 6 Darin Adler 2011-09-21 17:01:13 PDT
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.
Comment 7 Beth Dakin 2011-09-21 17:06:23 PDT
(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.
Comment 8 Beth Dakin 2011-09-21 17:51:52 PDT
> 
> 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.
Comment 9 Beth Dakin 2011-09-21 20:29:29 PDT
Committed change with Darin's suggested changes. Revision 95697.
Comment 10 Adam Roben (:aroben) 2011-09-22 04:40:46 PDT
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.
Comment 11 Darin Adler 2011-09-22 10:21:42 PDT
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.
Comment 12 Simon Fraser (smfr) 2011-09-22 10:44:16 PDT
(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).
Comment 13 Beth Dakin 2011-09-22 11:02:56 PDT
Good thoughts all around. Here's the follow-up bug: https://bugs.webkit.org/show_bug.cgi?id=68641