RESOLVED FIXED Bug 23291
-webkit-box-shadow doesn't work on Qt-WebKit 
https://bugs.webkit.org/show_bug.cgi?id=23291
Summary -webkit-box-shadow doesn't work on Qt-WebKit
Bernhard Rosenkraenzer
Reported 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.
Attachments
Attempted patch (2.83 KB, patch)
2009-08-10 14:43 PDT, Carol Szabo 		no flags
DetailsFormatted DiffDiff
Improved Attempted patch (2.83 KB, patch)
2009-08-10 15:13 PDT, Carol Szabo 		ariya.hidayat: review-
DetailsFormatted DiffDiff
Improved patch that also updates LayoutTest (15.96 KB, patch)
2009-08-12 05:01 PDT, Ariya Hidayat 		no flags
DetailsFormatted DiffDiff
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-
DetailsFormatted DiffDiff
Patch proposal that addresses Eric's concerns (see comment) (15.92 KB, patch)
2009-09-01 12:59 PDT, Carol Szabo 		eric: review-
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Repeat of previous patch, but with PNG files incorporated. (100.60 KB, patch)
2009-09-04 08:43 PDT, Carol Szabo 		ariya.hidayat: review-
DetailsFormatted DiffDiff
Addressed Ariya's concerns. (99.86 KB, patch)
2009-09-08 09:52 PDT, Carol Szabo 		ariya.hidayat: review+
DetailsFormatted DiffDiff
Fixed ChangeLog typo/style issues (99.87 KB, patch)
2009-09-08 15:27 PDT, Carol Szabo 		ariya.hidayat: review+
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Kumaran Santhanam
Comment 1 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>
Carol Szabo
Comment 2 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.
Carol Szabo
Comment 3 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.
Carol Szabo
Comment 4 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.
Ariya Hidayat
Comment 5 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.
Ariya Hidayat
Comment 6 2009-08-11 02:32:36 PDT
Comment on attachment 34521 [details] Improved Attempted patch r- for now because of the missing pixel tests.
Ariya Hidayat
Comment 7 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.
Ariya Hidayat
Comment 8 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/.
Ariya Hidayat
Comment 9 2009-08-12 05:01:39 PDT
Created attachment 34657 [details] Improved patch that also updates LayoutTest
Simon Hausmann
Comment 10 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.
Ariya Hidayat
Comment 11 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.
Carol Szabo
Comment 12 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.
Eric Seidel (no email)
Comment 13 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.
Carol Szabo
Comment 14 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?
Carol Szabo
Comment 15 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]
Eric Seidel (no email)
Comment 16 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.
Carol Szabo
Comment 17 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.
Eric Seidel (no email)
Comment 18 2009-09-02 23:39:04 PDT
Comment on attachment 38927 [details] Fixed style issues Looks sane.
Eric Seidel (no email)
Comment 19 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.
Carol Szabo
Comment 20 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.
Carol Szabo
Comment 21 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.
Ariya Hidayat
Comment 22 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.
Carol Szabo
Comment 23 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.
Ariya Hidayat
Comment 24 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.
Carol Szabo
Comment 25 2009-09-08 15:27:06 PDT
Created attachment 39221 [details] Fixed ChangeLog typo/style issues
Laszlo Gombos
Comment 26 2009-09-09 13:43:30 PDT
Committed as http://trac.webkit.org/changeset/48220 - but can not change the status of the bug.
Carol Szabo
Comment 27 2009-09-09 13:50:25 PDT
Comment on attachment 39221 [details] Fixed ChangeLog typo/style issues The bug has been committed by Laszlo
Note You need to log in before you can comment on or make changes to this bug.