Summary: | -webkit-background-clip:text is blurry in WebKit 1 apps when deviceScaleFactor > 1 | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||||||||||||
Component: | CSS | Assignee: | Beth Dakin <bdakin> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bdakin, cmarcelo, eric, gyuyoung.kim, japhet, jochen, macpherson, markbrown4, menard, mifenton, noam, rakuco, webkit.review.bot | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Beth Dakin
2012-07-25 18:18:22 PDT
Created attachment 154509 [details]
Patch
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.
Created attachment 154510 [details]
Patch for style 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 Created attachment 154524 [details]
Patch that should build everywhere
There may be a simpler way to do this… 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. (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. 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.
Comment on attachment 154751 [details]
Patch
Oh, I have a way simpler approach. New patch coming soon.
Created attachment 154843 [details]
Much simpler patch
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. 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?
Comment on attachment 154843 [details]
Much simpler patch
Or deviceTransform?
(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. Created attachment 155011 [details]
Patch that works for gradients with scaled ancestors
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. 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()? Created attachment 155036 [details]
Patch
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. Fixed! And committed http://trac.webkit.org/changeset/123912 *** Bug 93298 has been marked as a duplicate of this bug. *** |