Bug 44403

Summary: [Qt] -webkit-text-stroke is broken due to Qt::TextBypassShaping
Product: WebKit Reporter: Andreas Kling <kling>
Component: Layout and RenderingAssignee: QtWebKit Unassigned <webkit-qt-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ariya.hidayat, hausmann, jturcotte, kenneth
Priority: P2 Keywords: Qt, QtTriaged, Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Screenshot showing this bug
none
Proposed patch
kling: review-
Proposed patch v2
none
Proposed patch v3 kenneth: review+

Description Andreas Kling 2010-08-22 21:13:36 PDT
Created attachment 65071 [details]
Screenshot showing this bug

Since there is no way to pass the Qt::TextBypassShaping flag to QPainterPath::addText(), what we stroke may differ from what we fill (and what we paint as a shadow.)

See the attached image for an example of this bug.
Comment 1 Andreas Kling 2010-08-22 21:16:54 PDT
Created attachment 65072 [details]
Proposed patch
Comment 2 Ariya Hidayat 2010-08-22 21:50:25 PDT
Even with the patch, does that mean if I fill the text first and then stroke it, they will not be the same?
Hopefully drawTextCommon is never called twice with such order.
Comment 3 Andreas Kling 2010-08-22 22:05:14 PDT
(In reply to comment #2)
> Even with the patch, does that mean if I fill the text first and then stroke it, they will not be the same?
> Hopefully drawTextCommon is never called twice with such order.

Fudge! You're right. The bug can still be triggered by separate calls to strokeText() and fillText() on a CanvasRenderingContext2D.
Comment 4 Andreas Kling 2010-08-22 22:05:53 PDT
Comment on attachment 65072 [details]
Proposed patch

Self r-, need to fix this for CRC2D strokeText()/fillText(), too.
Comment 5 Andreas Kling 2010-08-23 02:48:13 PDT
Created attachment 65091 [details]
Proposed patch v2

To avoid the problem when using the Canvas API, I've added a "shouldForceComplexShaping" property to Font (Qt-only.)
Comment 6 Jocelyn Turcotte 2010-08-23 04:18:12 PDT
I think that using forceComplexShaping would be shorter and have the same meaning as shouldForceComplexShaping, though it doesn't matter much.

Still looks good to me.
Comment 7 Andreas Kling 2010-08-23 04:30:39 PDT
Created attachment 65101 [details]
Proposed patch v3

Talked to Simon (IRL) about this and he suggested that I use Font::setCodePath() instead, which is much cleaner than adding a platform-specific member to Font.
Comment 8 Kenneth Rohde Christiansen 2010-08-23 04:32:08 PDT
Comment on attachment 65101 [details]
Proposed patch v3

A lot cleaner indeed!
Comment 9 Kenneth Rohde Christiansen 2010-08-23 04:33:17 PDT
(In reply to comment #8)
> (From update of attachment 65101 [details])
> A lot cleaner indeed!

Still maybe some comments in the ifdef's would be nice!
Comment 10 Andreas Kling 2010-08-23 04:34:37 PDT
(In reply to comment #9)
> Still maybe some comments in the ifdef's would be nice!

Will land with comments. Thanks for the review <3
Comment 11 Andreas Kling 2010-08-23 04:44:59 PDT
Committed r65801: <http://trac.webkit.org/changeset/65801>
Comment 12 Simon Hausmann 2010-08-24 03:09:33 PDT
Revision r65801 cherry-picked into qtwebkit-2.1 with commit 1a184f3f6e088a4d5e4afd5ed71d3cb8d07a5a53