889235 - make text-shadow work on SVG text

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

We currently deliberately ignore text-shadow on SVG text.  It shouldn't be too much effort to get that to work.

Comment 2

[Robert Longson [:longsonr]]

Attachment: shadow.txt

Needs tests but this seems to work.

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8357860 [details] [diff] [review]
shadow.txt

Review of attachment 8357860 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +5515,5 @@
>      gfxPoint textBaselinePt(aFramePt.x + xOffset, aTextBaselinePt.y);
>  
>      // Determine what shadow, if any, to draw - either from textStyle
>      // or from the ::-moz-selection pseudo-class if specified there
> +    nsCSSShadowArray *shadow = textStyle->GetTextShadow();

While you're modifying this line, can you bump the "*" next to the type?

::: layout/svg/SVGTextFrame.cpp
@@ +927,1 @@
>  

I think that we should treat shadows separately from the fill, wrt aBBoxFlags.  For example, SVGTextFrame::FindCloserFrameForSelection probably shouldn't take the shadows into account, and I'm not sure about SVGTextFrame::GetFrameForPoint or SVGTextFrame::GetBBoxContribution either.  Can you add a new TextRenderedRun::eIncludeTextShadow and then use it only in Get{Run,Frame}UserSpaceRect calls where it makes sense?

Comment 4

[Robert Longson [:longsonr]]

Attachment: address review comments and add tests

Note that I haven't made text-shadow a mapped property as it isn't listed in http://www.w3.org/TR/SVG2/propidx.html 

Are you planning to get text-shadow added to the SVG 2 specification Cameron?

Comment 5

[Robert Longson [:longsonr]]

in SVGTextFrame::GetRunUserSpaceRect maybe I should only set the flag if HasStroke or HasFill is true and ignore (hitTestFlags & SVG_HIT_TEST_STROKE)?

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #4)
> Note that I haven't made text-shadow a mapped property as it isn't listed in
> http://www.w3.org/TR/SVG2/propidx.html 

OK.  I also haven't added e.g. a white-space="" presentation attribute yet.

> Are you planning to get text-shadow added to the SVG 2 specification Cameron?

Yes, we'll be enumerating the properties from css-text-3 and other CSS specs that we are having explicitly apply to SVG text.  I think we probably will be making presentation attributes for all properties we explicitly support, too (although I know roc wasn't fond of that idea).

(In reply to Robert Longson from comment #5)
> in SVGTextFrame::GetRunUserSpaceRect maybe I should only set the flag if
> HasStroke or HasFill is true and ignore (hitTestFlags & SVG_HIT_TEST_STROKE)?

Yes I think that'd be right.  Checking the hitTestFlags is only done so that our mRect is big enough to capture pointer events say on the stroke shape even if we have stroke="none".  Since I don't think we want the text shadow to influence the hit detection area, looking only at HasStroke/HasFill sounds right.  (I assume you mean ReflowSVG rather than GetRunUserSpaceRect.)

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8358356 [details] [diff] [review]
address review comments and add tests

Review of attachment 8358356 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/SVGTextFrame.cpp
@@ +914,5 @@
>    }
> +  nsRect fillInAppUnits(x, baseline - above,
> +                        width, metrics.mBoundingBox.height + above + below);
> +
> +  // Account for text-shadow

"." at the end of the comment.

Comment 8

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=1211d1c20219

Comment 9

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ce60609ad03

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ce60609ad03