Bug 23291 - -webkit-box-shadow doesn't work on Qt-WebKit
Summary: -webkit-box-shadow doesn't work on Qt-WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ariya Hidayat
URL: http://webkit.org/blog/86/box-shadow/
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-01-13 05:23 PST by Bernhard Rosenkraenzer
Modified: 2009-09-09 13:53 PDT (History)
4 users (show)

See Also:


Attachments
Attempted patch (2.83 KB, patch)
2009-08-10 14:43 PDT, Carol Szabo
no flags Details | Formatted Diff | Diff
Improved Attempted patch (2.83 KB, patch)
2009-08-10 15:13 PDT, Carol Szabo
ariya.hidayat: review-
Details | Formatted Diff | Diff
Improved patch that also updates LayoutTest (15.96 KB, patch)
2009-08-12 05:01 PDT, Ariya Hidayat
no flags Details | Formatted Diff | Diff
This is the second phase of the implementation of shadows on QtWebKit (110.94 KB, patch)
2009-08-27 21:24 PDT, Carol Szabo
eric: review-
Details | Formatted Diff | Diff
Patch proposal that addresses Eric's concerns (see comment) (15.92 KB, patch)
2009-09-01 12:59 PDT, Carol Szabo
eric: review-
Details | Formatted Diff | Diff
PNG files for the new and modified pixel test results. Should be considered with attachment 38878 (49.29 KB, application/x-gzip)
2009-09-01 13:00 PDT, Carol Szabo
no flags Details
Fixed style issues (15.91 KB, patch)
2009-09-02 09:01 PDT, Carol Szabo
no flags Details | Formatted Diff | Diff
Repeat of previous patch, but with PNG files incorporated. (100.60 KB, patch)
2009-09-04 08:43 PDT, Carol Szabo
ariya.hidayat: review-
Details | Formatted Diff | Diff
Addressed Ariya's concerns. (99.86 KB, patch)
2009-09-08 09:52 PDT, Carol Szabo
ariya.hidayat: review+
Details | Formatted Diff | Diff
Fixed ChangeLog typo/style issues (99.87 KB, patch)
2009-09-08 15:27 PDT, Carol Szabo
ariya.hidayat: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bernhard Rosenkraenzer 2009-01-13 05:23:48 PST
The -webkit-box-shadow attribute, as used e.g. on the announcement that WebKit supposedly supports it (http://webkit.org/blog/86/box-shadow/), is ignored on Qt-WebKit.

Verified in svn rev. 39779, Qt 4.5.0-snapshot 20090110, on Linux/X11.
Comment 1 Kumaran Santhanam 2009-07-20 22:58:09 PDT
1) Save the following HTML snippet to a file.
2) Browse the file with Firefox 3.5 or Safari 3.2.
3) Browse the file with the Qt 4.5.1 demo browser.

Qt WebKit does not render the drop shadow.
This was observed on both Linux and Windows.

The other browsers produce the expected behavior.

The WebKit source included with Qt has references
to this property, so there may be some other
mechanism that is disabling the functionality.


--- cut here ---
<html><body>

<div style="border: 1px solid #808080;
            -moz-box-shadow: 5px 5px 5px rgba(0, 0, 0, 0.5);
            -webkit-box-shadow: 5px 5px 5px rgba(0, 0, 0, 0.5);
            width: 300px; height: 200px;">
<br><center>This box should have a shadow.</center>
</div>

</body></html>
Comment 2 Carol Szabo 2009-08-10 13:46:56 PDT
I have confirmed this bug on Qt for Linux.
The root of the problem is that QtGui does not support shadow drawing and thus all shadows need to be manually drawn in Qt.
I have noticed that there is some support for drawing line shadows in GraphicsContextQt, but this existing support does not handle the blur parameter.
Comment 3 Carol Szabo 2009-08-10 14:43:19 PDT
Created attachment 34518 [details]
Attempted patch

This patch partially solves the problem reported in this bug. The patch causes shadows to appear in QtWebKit when the -webkit-box-shadow style is used, but the blur parameter is ignored. All shadows are totally sharp.
I did not find a LayoutTest to deal with the -webkit-box-shadow style and at this moment I do not know how to capture the difference visible on screen because of my fix.
Comment 4 Carol Szabo 2009-08-10 15:13:55 PDT
Created attachment 34521 [details]
Improved Attempted patch

Simplified code and possibly corrected a 1 pixel error in shadow size.
Comment 5 Ariya Hidayat 2009-08-11 01:49:16 PDT
I just added pixel tests support some time ago, so it is possible to add a test case to this patch. Check e.g. LayoutTests/fast/canvas/shadow-offset-* tests.
Comment 6 Ariya Hidayat 2009-08-11 02:32:36 PDT
Comment on attachment 34521 [details]
Improved Attempted patch

r- for now because of the missing pixel tests.
Comment 7 Ariya Hidayat 2009-08-11 04:19:32 PDT
Sorry, I pasted the wrong path. The pixel tests for this should be in  LayoutTests/platform/mac/fast/box-shadow/ instead.
Comment 8 Ariya Hidayat 2009-08-11 07:09:50 PDT
But of course, for the Qt port, we need LayoutTests/platform/qt/fast/box-shadow for the pixel test result. The actual test is in LayoutTests/fast/box-shadow/.
Comment 9 Ariya Hidayat 2009-08-12 05:01:39 PDT
Created attachment 34657 [details]
Improved patch that also updates LayoutTest
Comment 10 Simon Hausmann 2009-08-12 05:09:12 PDT
Comment on attachment 34657 [details]
Improved patch that also updates LayoutTest



> +    QPainter *p = m_data->p();

r=me, but please fix the coding style above when landing and also remove the test from the Qt skiplist.
Comment 11 Ariya Hidayat 2009-08-12 05:20:01 PDT
Comment on attachment 34657 [details]
Improved patch that also updates LayoutTest

Clearing the review flag.
Landed the patch:
http://trac.webkit.org/changeset/47103

I keep the bug open because the shadow support is not 100% completed yet.
Comment 12 Carol Szabo 2009-08-27 21:24:23 PDT
Created attachment 38714 [details]
This is the second phase of the implementation of shadows on QtWebKit

Supports all elements supported on the mac platform except for the blur.
Comment 13 Eric Seidel (no email) 2009-09-01 00:49:47 PDT
Comment on attachment 38714 [details]
This is the second phase of the implementation of shadows on QtWebKit

+++ LayoutTests/platform/qt/fast/box-shadow/basic-shadows-expected.png	(working copy)
is not properly marked as binary, making the diff nasty.

I'm surprised the (int) is needed here:
 424         shadowRect.inflate((int)p->pen().widthF());
also, normally we use static_cast<int>() for c++

I'm surprised explicit construction is needed here:
 566         pen.setColor(QColor(shadowColor));

I'm a bit surprised these don't have a bool conversion operator:
 598         if (p->brush().style() != Qt::NoBrush)
 601         if (pen.style() != Qt::NoPen) {

Again, explicit construction of the color at least is likely not needed:
 599             p->setBrush(QBrush(QColor(shadowColor)));
Color has a QColor operator, no?

I don't understand why this gets pulled into a local first:
 636             QTransform t(p->worldTransform());
pen's don't have a save/retore?  Or I guess that would be more expensive?

Seems strange that platformGradient() doesn't return a gradient including the transform?
 652             QBrush brush(*m_common->state.fillGradient->platformGradient());
 653             brush.setTransform(m_common->state.fillGradient->gradientSpaceTransform());

Really?
             TransformationMatrix affine;
 733             p->fillRect(rect, QBrush(m_common->state.fillPattern->createPlatformPattern(affine)));
In other platforms at least the passed in affine transform is supposed to be the ctm()

Locally defined functions sure would make this easier, as we could pass them to some generic "draw shadow" routine.  A shame that gcc disabled them.

Seems there is at least one instance of copy/paste fillPath() w/ shadow code which we could easily share implementations of.

r- for the confused patch and for the questions above.  Would be great to share more code here.
Comment 14 Carol Szabo 2009-09-01 12:59:06 PDT
Created attachment 38878 [details]
Patch proposal that addresses Eric's concerns (see comment)

(In reply to comment #13)
> (From update of attachment 38714 [details])
> +++ LayoutTests/platform/qt/fast/box-shadow/basic-shadows-expected.png   
> (working copy)
> is not properly marked as binary, making the diff nasty.

I do not know how to "properly" mark the file as binary. I marked it as svn:mime-type=image/png, which it is and as a result the patch showed the content of the file as base64 or something similar.
To work around this issue after consulting with ariya I have removed the new and modified png files for the pixel tests from the patch and I shall submit them as a separate attachment.

> 
> I'm surprised the (int) is needed here:
>  424         shadowRect.inflate((int)p->pen().widthF());
> also, normally we use static_cast<int>() for c++
> 

This is necessary to avoid compiler "possible overflow" warning as "qreal" can hold larger numbers than "int" can.


> I'm surprised explicit construction is needed here:
>  566         pen.setColor(QColor(shadowColor));
> 

The explicit construction is not needed, but since it was used elsewhere in the file I used it for consistency in the original patch.
Now it is removed.

> I'm a bit surprised these don't have a bool conversion operator:
>  598         if (p->brush().style() != Qt::NoBrush)
>  601         if (pen.style() != Qt::NoPen) {
> 

There is neither QPen nor QBrush to bool conversion (as far as I can tell). PenStyle and BrushStyle to bool conversions are implemented by the compiler (enum to bool) but the code readability will suffer if we use them: if(pen.style()) ??? it is odd and not immediately obvious, while if(pen) looks more acceptable but creates some confusion with checking for a NULL QPen pointer.
> Again, explicit construction of the color at least is likely not needed:
>  599             p->setBrush(QBrush(QColor(shadowColor)));
> Color has a QColor operator, no?
> 

Removed

> I don't understand why this gets pulled into a local first:
>  636             QTransform t(p->worldTransform());
> pen's don't have a save/retore?  Or I guess that would be more expensive?
> 

I am not 100% sure I know what you mean here, but I see no reference to any pen in the neighborhood of this code. Since it is a fill, it would use a brush.
I considered that for code readability, size and possibly speed also, it is better to save the worldTransform (a 9 qreal matrix), change it and restore it than to offset all points in the path twice although for short paths (4 points or less definitely and maybe even for a little more points, changing the matrix may be more costly).

> Seems strange that platformGradient() doesn't return a gradient including the
> transform?
>  652             QBrush
> brush(*m_common->state.fillGradient->platformGradient());
>  653            
> brush.setTransform(m_common->state.fillGradient->gradientSpaceTransform());
> 
> Really?
>              TransformationMatrix affine;
>  733             p->fillRect(rect,
> QBrush(m_common->state.fillPattern->createPlatformPattern(affine)));
> In other platforms at least the passed in affine transform is supposed to be
> the ctm()
> 

Please do not blame me for the code commented on above, which I do not fully understand. All I did was to indent it properly. I did not want to change its behavior, as the current behavior is supposedly tested and running until a bug is written against it and then fixing this code will be part of that bug fix not of shadow implementation.

> Locally defined functions sure would make this easier, as we could pass them to
> some generic "draw shadow" routine.  A shame that gcc disabled them.
> 
> Seems there is at least one instance of copy/paste fillPath() w/ shadow code
> which we could easily share implementations of.
> 

I have created shared implementation for fillPath and fillRect. I am not particularly happy since the code is used in only 2 places in each case and shadows are rarely, if ever, used, while the overhead of calculating parameters and calling the function will be encountered always.
I could have written some tricky macros to take care of the repetition such as:
#define BEGIN_SHADOW ... if(getSahdow(...)) {
#define END_SHADOW }

but I felt they bring little value and make the code more cryptic than it needs to be.

> r- for the confused patch and for the questions above.  Would be great to share
> more code here.

I am not sure I understand what Eric means here. More context lines maybe?
Comment 15 Carol Szabo 2009-09-01 13:00:21 PDT
Created attachment 38879 [details]
PNG files for the new and modified pixel test results. Should be considered with attachment 38878 [details]
Comment 16 Eric Seidel (no email) 2009-09-02 01:23:53 PDT
Comment on attachment 38878 [details]
Patch proposal that addresses Eric's concerns (see comment)

The ChangeLog is now out of date (it doesn't mention the new functions)

 621 static void inline DrawShadowPathFilled(GraphicsContext* self, QPainter* p, const QPainterPath *path)

self is a reserved word for Obj-C so using it here might be confusing.  I would have called that "context" instead.

Also, DrawShadowPathFilled does not match WebKit naming style:
http://webkit.org/coding/coding-style.html

I would expect the check-webkit-style script to catch the naming issue, but maybe not?

Also the indent is wrong.  4spaces, not 2.

This definitely looks better.  I mean to fully share the shadow code we'd have to come up with some object level abstraction with virtual functions for the "draw" operation, and storage for the various parameters the draw() would need.   This improved version is definitely less code duplication than before.  Really it would be best if QPainter just knew how to do this.

r- for the style issues.
Comment 17 Carol Szabo 2009-09-02 09:01:24 PDT
Created attachment 38927 [details]
Fixed style issues

Unfortunately, after fixing indenting, the svn-create-patch tool created a counterintuitive patch that makes it look like much more is changed than it actually has around the fillRect function. Carefull examination will show that essentially what has been done here is that the shadow drawing has been extracted from the function into a separate function placed before it in the code.
Comment 18 Eric Seidel (no email) 2009-09-02 23:39:04 PDT
Comment on attachment 38927 [details]
Fixed style issues

Looks sane.
Comment 19 Eric Seidel (no email) 2009-09-02 23:50:09 PDT
Comment on attachment 38927 [details]
Fixed style issues

I think it sucks that we have to copy/paste so much code, but the only alternatives I can think of is to write little classes like this:

struct ArcDrawer {
  FloatRect rect; // whatever this is?
  float startAngle;
 float startSpan;
 
 void draw(QPainter* p) {
    p->drawArc(rect, startAngle, angleSpan)
 }
}

And then have some sort of:

template<T>
drawWithPossibleShadow(T drawer, QPainter* p)
{
        IntSize shadowSize;
     int shadowBlur;
     Color shadowColor;
     if (getShadow(shadowSize, shadowBlur, shadowColor)) {
         p->save();
         p->translate(shadowSize.width(), shadowSize.height());
         QPen pen(p->pen());
         pen.setColor(shadowColor);
         p->setPen(pen);
         drawer->draw(p);
         p->restore();
     }
     drawer->draw(p);
}

or another way could be to use a C++ stack object:

ShadowSetup {
   bool hasShadow;
   QPainter* m_p;

  ShadowSetup(QPainter* p) {
     m_p = p;
     IntSize shadowSize;
     int shadowBlur;
     Color shadowColor;
     hasShadow = getShadow(shadowSize, shadowBlur, shadowColor);
     if (hasShadow) {
         p->save();
         p->translate(shadowSize.width(), shadowSize.height());
         QPen pen(p->pen());
         pen.setColor(shadowColor);
         p->setPen(pen);
    }
  }
  ~ShadowSetup() {
     if (hasShadow)
        p->restore();
  }

The final way would of course be to use virtual functions and a shared baseclass with a solution similar to the drawer one described first.

If you think any of those would be cleaner and you're up to implementing such, you certainly should feel encoruaged to.

I've removed commit-queue+ for now, pending your comment.
Comment 20 Carol Szabo 2009-09-03 05:54:23 PDT
(In reply to comment #19)
Eric,
While your solution is more elegant, it is not as fast as the current one and in the case of rendering I believe that performance trumps elegance (even if in the current case the difference is not much in either).
Here's why I prefer the current solution (and I would have preferred not to have the 2 inline functions either but my argument is weaker there):
1. Your solution involves constructing the drawer object even for shapes with no shadows (which is probably 99.99% of all shapes).
2. Your solution precludes some optimizations,  for example one of the most frequent shadowed elements (in my opinion) a filled in rectangular area has its drawing optimized using no pen and no changes to the worldTrasformation, thus I believe it will draw faster in the current version.
3. Your solution makes the code more difficult to read and debug than it needs to be.
4. All this is done for little advantage. Saving a few lines of code in less than 10 functions.

I prefer to your class based solution a macro based one where there is a BEGIN_SHADOW and an END_SHADOW macro that would cover the decision about the existence of the shadow and the setup of variables for it. The code for actually drawing the shadows remains inline and can be adapted to every shape.
This way the code stays pretty much the same in terms of performance, it is a little more compact, but I did not take this route for ease of reading and debugging. I find some value in the code being easily and naturally readable even by people who are unfamiliar with it.

I believe that the code should be committed as is. If Qt will support native shadows, than the code can be easily removed.
Comment 21 Carol Szabo 2009-09-04 08:43:37 PDT
Created attachment 39060 [details]
Repeat of previous patch, but with PNG files incorporated.

This is essentially the same patch that I have submitted before and has been reviewed positively by Eric Seidel. I have just added the expected PNG files so that it can be easily landed.
Comment 22 Ariya Hidayat 2009-09-08 02:54:49 PDT
Comment on attachment 39060 [details]
Repeat of previous patch, but with PNG files incorporated.


> +        Need a short description and bug URL (OOPS!)
> +        https://bugs.webkit.org/show_bug.cgi?id=23291

Don't forget to write down something descriptive here, for both WebCore/ChangeLog and LayoutTests/ChangeLog.

> +        QTransform t(p->worldTransform());
> +        p->translate(shadowSize.width(), shadowSize.height());
> +        p->fillPath(*path, QBrush(shadowColor));
> +        p->setWorldTransform(t);

I suggest translating back (negative offset) the painter after filling the path. That is much faster than stashing and restoring the transformation matrix.

> -    if (m_common->state.shadowsIgnoreTransforms) {
> -        // Meaning that this graphics context is associated with a CanvasRenderingContext
> -        // We flip the height since CG and HTML5 Canvas have opposite Y axis
> -        m_common->state.shadowSize = IntSize(size.width(), -size.height());
> -    }

I added this block because it is necessary to make the shadows working properly inside the Canvas.
Why is this removed? Do we now take Canvas into account somewhere else?

In principle, the patch LGTM except the minor issues.

r- until the issues are solved.
Comment 23 Carol Szabo 2009-09-08 09:52:27 PDT
Created attachment 39193 [details]
Addressed Ariya's concerns.

I have removed changes to setPlatformShadow as they were unintended, and they do not affect in any way any of the test cases I ran through.
Comment 24 Ariya Hidayat 2009-09-08 14:57:12 PDT
Comment on attachment 39193 [details]
Addressed Ariya's concerns.

Before landing it, beware that "platofrm" should be "platform". Also prefix the log with [Qt], it's preferable.
Comment 25 Carol Szabo 2009-09-08 15:27:06 PDT
Created attachment 39221 [details]
Fixed ChangeLog typo/style issues
Comment 26 Laszlo Gombos 2009-09-09 13:43:30 PDT
Committed as http://trac.webkit.org/changeset/48220 - but can not change the status of the bug.
Comment 27 Carol Szabo 2009-09-09 13:50:25 PDT
Comment on attachment 39221 [details]
Fixed ChangeLog typo/style issues

The bug has been committed by Laszlo