24687 2009-03-18 17:01:54 -0700 Gradient text in canvas not working on chromium/skia. 2009-03-20 09:39:54 -0700 1 1 1 Unclassified WebKit Text 528+ (Nightly build) PC Windows XP RESOLVED FIXED P2 Normal --- 1 senorblanco senorblanco eric krit senorblanco oldest_to_newest 114256 0 senorblanco 2009-03-18 17:01:54 -0700 Gradient text in canvas is not working on chromium/skia: the gradient is there, but is not scaled correctly (is solid colour, should be gradient). See LayoutTest/fast/canvas/canvas-text-alignment.html. 114258 1 28739 senorblanco 2009-03-18 17:09:19 -0700 Created attachment 28739 Fix for canvas text gradient problem. 114284 2 krit 2009-03-19 00:04:44 -0700 > +#if PLATFORM(CG) > if (boundingBoxMode() && !isPaintingText) { > +#else > + if (boundingBoxMode()) { > +#endif It's not just CG, that uses this code. Please just use #if !PLATFORM(SKIA) if (boundingBoxMode() && !isPaintingText) { #else if (boundingBoxMode()) { #endif if it is necessary. I think you will break some tests of batik. Have you checked this? And the special skia code, that we have in SVGPaintServer, is just needed for fillAndStrokePath. (btw. this can be fixed by closing the path after filling or stroking inf gc:fillPath() and gc::strokePath()). I'm still not sure if I get the problem. Gradients don't work for Skia in Canvas, right? And you fix SVG, because the transformation matrix is applied twice for Canvas. Wehre (beside SkiaFontWin.cpp) and why? To add more platform code to SVG is maybe not the best way and it can break some tests. Can we fix in internally? 114313 3 senorblanco 2009-03-19 08:33:00 -0700 (In reply to comment #2) > > +#if PLATFORM(CG) > > if (boundingBoxMode() && !isPaintingText) { > > +#else > > + if (boundingBoxMode()) { > > +#endif > > It's not just CG, that uses this code. Please just use > > #if !PLATFORM(SKIA) > if (boundingBoxMode() && !isPaintingText) { > #else > if (boundingBoxMode()) { > #endif > > if it is necessary. I think you will break some tests of batik. Have you > checked this? There were no regressions in the batik tests on Chromium on Win32 or Webkit on Mac. I'm pretty sure that those other platforms will need this matrix in order to do gradient text correctly. It's only CG that won't need it, since it uses the gradient later in teardown() to fill a rect. I can change it to be skia-only, if you still want, but I'm pretty sure you're going to need this matrix for Cairo as well (I'm assuming that's what you're worried about). > And the special skia code, that we have in SVGPaintServer, is just needed for > fillAndStrokePath. (btw. this can be fixed by closing the path after filling or > stroking inf gc:fillPath() and gc::strokePath()). You're saying that we could remove the skia-specific code in SVGPaintServer? That sounds good, but is orthogonal to this patch. > I'm still not sure if I get the problem. Gradients don't work for Skia in > Canvas, right? And you fix SVG, because the transformation matrix is applied > twice for Canvas. Wehre (beside SkiaFontWin.cpp) and why? Basically, the correct way to do gradient text is to scale up the gradient to the appropriate size (the bounding box, in this case) before rendering the text. Canvas was already doing that (m_p0 was 0,0 and m_p1 was 600,600 in this test), so when I added code in a previous CL to scale up the gradient in SkiaFontWin, it was getting scaled twice. So I removed my previous code to scale the gradient (in SkiaFontWin), and allowed the SVG path to set the matrix from the bounding box instead. > To add more platform code to SVG is maybe not the best way and it can break > some tests. Can we fix in internally? I don't think there's any other way to fix it, since the lower-level rendering code needs that bounding box in order to correctly scale the gradient. And the lower level doesn't know if it has been already set (as in the canvas case) or unset (as in the SVG case). The only reason CG doesn't need the matrix is that it doesn't use the gradient during text rendering; it applies it in teardown(). 114334 4 krit 2009-03-19 10:33:47 -0700 I got it. Yes the current code in SVGPaintServerGradient does only work for CG correctly. Your patch is ok. :-). eseidel? r+? 114376 5 28739 eric 2009-03-19 14:40:54 -0700 Comment on attachment 28739 Fix for canvas text gradient problem. Looks fine. 114502 6 eric 2009-03-20 09:38:05 -0700 I expect this will break qt/gtk pixel results (if those exist), but I have no easy way to update those, and from what I can tell from Dirk and Stephen this will be a progression for both sets of results. Also, Stephen, be sure to add the bug URL to the ChangeLog in the future. ;) 114503 7 eric 2009-03-20 09:39:54 -0700 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/platform/graphics/skia/SkiaFontWin.cpp M WebCore/svg/graphics/SVGPaintServerGradient.cpp Committed r41860 Thanks again for the patch! 28739 2009-03-18 17:09:19 -0700 2009-03-19 14:40:54 -0700 Fix for canvas text gradient problem. canvas-gradient-text.patch text/plain 3718 senorblanco SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MTgxOCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjIgQEAKKzIwMDktMDMtMTggIFN0ZXBoZW4gV2hpdGUgIDxzZW5vcmJsYW5jb0Bj aHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgRml4IGZvciBMYXlvdXRUZXN0cy9mYXN0L2NhbnZhcy9jYW52YXMtdGV4dC1hbGlnbm1l bnQuaHRtbAorICAgICAgICBvbiBjaHJvbWl1bS9za2lhLiAgVGhlIHByb2JsZW0gd2FzIHRoYXQg dGhlIGdyYWRpZW50IG1hdHJpeAorICAgICAgICBmb3IgdGV4dCB3YXMgYmVpbmcgYXBwbGllZCB0 d2ljZS4gIEZpeGVkIGJ5IHJldmVydGluZyBzb21lIG9mCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMzk1Nywgc28gdGhhdCBza2lhRHJhd1RleHQKKyAg ICAgICAgaXMgbm8gbG9uZ2VyIHJlc3BvbnNpYmxlIGZvciBtZWFzdXJpbmcgdGhlIHRleHQgYW5k IHNjYWxpbmcgdXAKKyAgICAgICAgdGhlIGdyYWRpZW50IG1hdHJpeC4gIEluc3RlYWQsIHRoZSB0 ZXh0IGJvdW5kaW5nIGJveCBpcyBwYXNzZWQKKyAgICAgICAgaW4gZnJvbSBTVkdQYWludFNlcnZl ckdyYWRpZW50LiAgSSBkaWRuJ3QgbWFrZSB0aGlzIGNoYW5nZSBmb3IgQ0csCisgICAgICAgIHNp bmNlIGl0IHVzZXMgYSBkaWZmZXJlbnQgbWV0aG9kICh0aGUgZ3JhZGllbnQgaXMgZHJhd24gdXNp bmcgdGhlCisgICAgICAgIHRleHQgYXMgYSBwcmUtcmVuZGVyZWQgbWFzaykuCisKKyAgICAgICAg KiBwbGF0Zm9ybS9ncmFwaGljcy9za2lhL1NraWFGb250V2luLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OnNraWFEcmF3VGV4dCk6CisgICAgICAgICogc3ZnL2dyYXBoaWNzL1NWR1BhaW50U2VydmVy R3JhZGllbnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U1ZHUGFpbnRTZXJ2ZXJHcmFkaWVudDo6 c2V0dXApOgorCiAyMDA5LTAzLTE4ICBFcmljIENhcmxzb24gIDxlcmljLmNhcmxzb25AYXBwbGUu Y29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IFNpbW9uIEZyYXNlci4KSW5kZXg6IFdlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9Ta2lhRm9udFdpbi5jcHAKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2Vi Q29yZS9wbGF0Zm9ybS9ncmFwaGljcy9za2lhL1NraWFGb250V2luLmNwcAkocmV2aXNpb24gNDE4 MTgpCisrKyBXZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3NraWEvU2tpYUZvbnRXaW4uY3BwCSh3 b3JraW5nIGNvcHkpCkBAIC0yNjYsMjMgKzI2Niw5IEBAIHN0YXRpYyBib29sIHNraWFEcmF3VGV4 dChIRk9OVCBoZm9udCwKICAgICAgICAgICAgICAgICAgICAgICAgICBpbnQgbnVtR2x5cGhzKQog ewogICAgIFNrU2hhZGVyKiBzaGFkZXIgPSBOVUxMOwotICAgIFNrTWF0cml4IG9sZFNoYWRlck1h dHJpeDsKLSAgICBpZiAoZ3JhZGllbnQpIHsKKyAgICBpZiAoZ3JhZGllbnQpCiAgICAgICAgIHNo YWRlciA9IGdyYWRpZW50LT5wbGF0Zm9ybUdyYWRpZW50KCk7Ci0gICAgICAgIC8vIEdldCB0aGUg bGVuZ3RoIG9mIHRoZSBzdHJpbmcgaW4gcGl4ZWxzLgotICAgICAgICBpbnQgd2lkdGggPSAwOwot ICAgICAgICBmb3IgKGludCBpID0gMDsgaSA8IG51bUdseXBoczsgaSsrKQotICAgICAgICAgICAg d2lkdGggKz0gYWR2YW5jZXNbaV07Ci0KLSAgICAgICAgLy8gU2F2ZSB0aGUgY3VycmVudCBzaGFk ZXIgbWF0cml4LgotICAgICAgICBzaGFkZXItPmdldExvY2FsTWF0cml4KCZvbGRTaGFkZXJNYXRy aXgpOwotCi0gICAgICAgIC8vIFNjYWxlIHVwIHRoZSBncmFkaWVudCBtYXRyaXggYnkgdGhlIHdp ZHRoIG9mIHRoZSB0ZXh0IHN0cmluZy4KLSAgICAgICAgU2tNYXRyaXggc2hhZGVyTWF0cml4KG9s ZFNoYWRlck1hdHJpeCk7Ci0gICAgICAgIHNoYWRlck1hdHJpeC5wb3N0U2NhbGUoc3RhdGljX2Nh c3Q8ZmxvYXQ+KHdpZHRoKSwgMS4wZik7Ci0gICAgICAgIHNoYWRlck1hdHJpeC5wb3N0VHJhbnNs YXRlKHBvaW50LmZYLCBwb2ludC5mWSk7Ci0gICAgICAgIHNoYWRlci0+c2V0TG9jYWxNYXRyaXgo c2hhZGVyTWF0cml4KTsKLSAgICB9IGVsc2UgaWYgKHBhdHRlcm4pCisgICAgZWxzZSBpZiAocGF0 dGVybikKICAgICAgICAgc2hhZGVyID0gcGF0dGVybi0+Y3JlYXRlUGxhdGZvcm1QYXR0ZXJuKHRy YW5zZm9ybWF0aW9uTWF0cml4KTsKIAogICAgIHBhaW50LT5zZXRTaGFkZXIoc2hhZGVyKTsKQEAg LTMwNiwxMCArMjkyLDYgQEAgc3RhdGljIGJvb2wgc2tpYURyYXdUZXh0KEhGT05UIGhmb250LAog ICAgICAgICB4ICs9IGFkdmFuY2VzW2ldOwogICAgIH0KIAotICAgIC8vIFJlc3RvcmUgdGhlIHBy ZXZpb3VzIHNoYWRlciBtYXRyaXguCi0gICAgaWYgKGdyYWRpZW50KQotICAgICAgICBzaGFkZXIt PnNldExvY2FsTWF0cml4KG9sZFNoYWRlck1hdHJpeCk7Ci0KICAgICByZXR1cm4gdHJ1ZTsKIH0K IApJbmRleDogV2ViQ29yZS9zdmcvZ3JhcGhpY3MvU1ZHUGFpbnRTZXJ2ZXJHcmFkaWVudC5jcHAK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gV2ViQ29yZS9zdmcvZ3JhcGhpY3MvU1ZHUGFpbnRTZXJ2ZXJHcmFkaWVu dC5jcHAJKHJldmlzaW9uIDQxODE4KQorKysgV2ViQ29yZS9zdmcvZ3JhcGhpY3MvU1ZHUGFpbnRT ZXJ2ZXJHcmFkaWVudC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIxOCw5ICsyMTgsMTcgQEAgYm9v bCBTVkdQYWludFNlcnZlckdyYWRpZW50OjpzZXR1cChHcmFwaAogICAgIH0KIAogICAgIFRyYW5z Zm9ybWF0aW9uTWF0cml4IG1hdHJpeDsKKyAgICAvLyBDRyBwbGF0Zm9ybXMgd2lsbCBoYW5kbGUg dGhlIGdyYWRpZW50IHNwYWNlIHRyYW5zZm9ybSBmb3IgdGV4dCBpbgorICAgIC8vIHRlYXJkb3du LCBzbyB3ZSBkb24ndCBhcHBseSBpdCBoZXJlLiAgRm9yIG5vbi1DRyBwbGF0Zm9ybXMsIHdlCisg ICAgLy8gd2FudCB0aGUgdGV4dCBib3VuZGluZyBib3ggYXBwbGllZCB0byB0aGUgZ3JhZGllbnQg c3BhY2UgdHJhbnNmb3JtIG5vdywKKyAgICAvLyBzbyB0aGUgZ3JhZGllbnQgc2hhZGVyIGNhbiB1 c2UgaXQuCisjaWYgUExBVEZPUk0oQ0cpCiAgICAgaWYgKGJvdW5kaW5nQm94TW9kZSgpICYmICFp c1BhaW50aW5nVGV4dCkgeworI2Vsc2UKKyAgICBpZiAoYm91bmRpbmdCb3hNb2RlKCkpIHsKKyNl bmRpZgogICAgICAgICBGbG9hdFJlY3QgYmJveCA9IG9iamVjdC0+cmVsYXRpdmVCQm94KGZhbHNl KTsKLSAgICAgICAgLy8gRG9uJ3QgdXNlIGdyYWRpZW50ZXMgZm9yIDFkIG9iamVjdHMgbGlrZSBo b3Jpem9udGFsL3ZlcnRpY2FsIAorICAgICAgICAvLyBEb24ndCB1c2UgZ3JhZGllbnRzIGZvciAx ZCBvYmplY3RzIGxpa2UgaG9yaXpvbnRhbC92ZXJ0aWNhbCAKICAgICAgICAgLy8gbGluZXMgb3Ig cmVjdGFuZ2xlcyB3aXRob3V0IHdpZHRoIG9yIGhlaWdodC4KICAgICAgICAgaWYgKGJib3gud2lk dGgoKSA9PSAwIHx8IGJib3guaGVpZ2h0KCkgPT0gMCkgewogICAgICAgICAgICAgQ29sb3IgY29s b3IoMCwgMCwgMCk7Cg==