Bug 92327 – -webkit-background-clip:text is blurry in WebKit 1 apps when deviceScaleFactor > 1

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Beth Dakin]

-webkit-background-clip:text is blurry in WebKit 1 when deviceScaleFactor > 1

<rdar://problem/11683788>

Patch forthcoming.

[Beth Dakin]

Created attachment 154509 [details]
Patch

[WebKit Review Bot]

Attachment 154509 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/CSSGradientValue.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 154510 [details]
Patch for style bot

[Early Warning System Bot]

Comment on attachment 154510 [details]
Patch for style bot

Attachment 154510 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13340465

[Beth Dakin]

Created attachment 154524 [details]
Patch that should build everywhere

[Beth Dakin]

There may be a simpler way to do this…

[Simon Fraser (smfr)]

Comment on attachment 154524 [details]
Patch that should build everywhere

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

> Source/WebCore/css/CSSGradientValue.cpp:78
> +    if (FrameView* frameView = renderer->view()->frameView()) {
> +        if (HostWindow* hostWindow = frameView->hostWindow()) {
> +            if (!hostWindow->ctmIncludesDeviceScaleFactor()) {
> +                if (Page* page = renderer->document()->page())

Hmm, that's pretty icky. Could we just export a method on Page, perhaps, that would make this cleaner?

> Source/WebCore/page/Chrome.h:90
> +        virtual bool ctmIncludesDeviceScaleFactor();

This feels a bit too general. It doesn't say _which_ ctm. I could imagine that we could easily have bugs where we use code to draw both into the tiles, and into an ImageBuffer, and we need to be able to get both of them right.

I think 'device scale is baked into CTM' is something you have to ask the GraphicsContext.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:836
> +        if (FrameView* frameView = view()->frameView()) {
> +            if (HostWindow* hostWindow = frameView->hostWindow()) {
> +                if (!hostWindow->ctmIncludesDeviceScaleFactor()) {
> +                    if (Page* page = document()->page())
> +                        scaleFactorNotIncludedInCCTM = page->deviceScaleFactor();

Similar ickitude.

[Beth Dakin]

(In reply to comment #7)
> (From update of attachment 154524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154524&action=review
> 
> > Source/WebCore/css/CSSGradientValue.cpp:78
> > +    if (FrameView* frameView = renderer->view()->frameView()) {
> > +        if (HostWindow* hostWindow = frameView->hostWindow()) {
> > +            if (!hostWindow->ctmIncludesDeviceScaleFactor()) {
> > +                if (Page* page = renderer->document()->page())
> 
> Hmm, that's pretty icky. Could we just export a method on Page, perhaps, that would make this cleaner?
> 

Agreed. A method on Page is a really good idea. I will go in that direction.

> > Source/WebCore/page/Chrome.h:90
> > +        virtual bool ctmIncludesDeviceScaleFactor();
> 
> This feels a bit too general. It doesn't say _which_ ctm. I could imagine that we could easily have bugs where we use code to draw both into the tiles, and into an ImageBuffer, and we need to be able to get both of them right.
> 
> I think 'device scale is baked into CTM' is something you have to ask the GraphicsContext.
> 

I'm a little confused here. Are you saying that you think that this function SHOULD move to GraphicsContext? Or are you just saying that it named poorly because it is named AS IF it's a GraphicsContext function?

I definitely agree that it is poorly named. But if you are saying that you think is SHOULD move to GraphicsContext, well I just don't know how we would be able to answer that question at that level. I tired a number of approaches there since that would simplify this code greatly, but nothing has panned out.

[Beth Dakin]

Created attachment 154751 [details]
Patch

Okay, here's a patch with better names, and with new function on Page to avoid the ickiness of the old patch in RenderBoxModelObject and GeneratorGeneratedImage.

[Beth Dakin]

Comment on attachment 154751 [details]
Patch

Oh, I have a way simpler approach. New patch coming soon.

[Beth Dakin]

Created attachment 154843 [details]
Much simpler patch

[Simon Fraser (smfr)]

Comment on attachment 154843 [details]
Much simpler patch

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

Does this do the right thing with a gradient on a <div> which has an ancestor with a scale transform? That might be a good testcase to have.

> Source/WebCore/platform/graphics/GraphicsContext.h:562
> +        // getCompatibleCTM() is guaranteed to include the deviceScaleFactor

Period at end of sentence.

[Darin Adler]

Comment on attachment 154843 [details]
Much simpler patch

I love this solution, but I do not understand the meaning of the word “compatible” in this function name. The matrix is compatible with what? The createCompatibleBuffer function uses the word compatible to mean “compatible with the device to be drawn on”, but I don’t see how the matrix would be compatible. Maybe deviceSpaceCTM would be a good name?

[Darin Adler]

Comment on attachment 154843 [details]
Much simpler patch

Or deviceTransform?

[Beth Dakin]

(In reply to comment #13)
> (From update of attachment 154843 [details])
> I love this solution, but I do not understand the meaning of the word “compatible” in this function name. The matrix is compatible with what? The createCompatibleBuffer function uses the word compatible to mean “compatible with the device to be drawn on”, but I don’t see how the matrix would be compatible. Maybe deviceSpaceCTM would be a good name?

I agree that the name is weak. I like your suggestions! I need to do a bit more testing before I commit because there may actually be a bug still with the case Simon mentioned (gradients with a scaled ancestor). Anyway, I will consider one of these re-names before I commit.

[Beth Dakin]

Created attachment 155011 [details]
Patch that works for gradients with scaled ancestors

[Eric Seidel (no email)]

Comment on attachment 154843 [details]
Much simpler patch

Cleared Simon Fraser's review+ from obsolete attachment 154843 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Simon Fraser (smfr)]

Comment on attachment 155011 [details]
Patch that works for gradients with scaled ancestors

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

> Source/WebCore/platform/graphics/GraphicsContext.h:419
> +        // deviceTransform() is similar to getCTM(), but it is guaranteed to include the deviceScaleFactor.
> +        AffineTransform deviceTransform() const;

I don't think deviceTransform is a good name; it implies only 1x or 2x. Maybe ctmIncludingDeviceScale()?

[Beth Dakin]

Created attachment 155036 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 155036 [details]
Patch

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

> Source/WebCore/ChangeLog:37
> +        Actually use the new parameter in the CG implementation. Use CG API to get a
> +
> +        matrix that definitely includes the device scale when that is required. 

Blank line here.

> Source/WebCore/platform/graphics/GraphicsContext.h:417
> +        enum IncludeDeviceScale { DefinitelyIncludeDeviceScale, PossiblyIncludeDeviceScale};

Space before the }

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1425
>      return AffineTransform(CGContextGetCTM(platformContext()));

It seems like this explicit construction of a AffineTransform is unnecessary.

> LayoutTests/fast/hidpi/gradient-with-scaled-ancestor.html:2
> +<div style="-webkit-transform:scale(2); -webkit-transform-origin:top left;">
> +<div style="height:300px;width:300px;background-image:-webkit-repeating-linear-gradient(top left, red 10%, blue 30%);border:1px solid">

You should fix this test to not show a scrollbar in the image result.

[Beth Dakin]

Fixed! And committed http://trac.webkit.org/changeset/123912

*** Bug 93298 has been marked as a duplicate of this bug. ***