make text-shadow work on SVG text

RESOLVED FIXED in mozilla29

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: heycam, Assigned: longsonr)

Tracking

Trunk
mozilla29
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Updated

5 years ago
Duplicate of this bug: 529867
(Assignee)

Updated

4 years ago
Assignee: nobody → longsonr
(Assignee)

Comment 2

4 years ago
Created attachment 8357860 [details] [diff] [review]
shadow.txt

Needs tests but this seems to work.
Attachment #8357860 - Flags: review?(cam)
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?
Attachment #8357860 - Flags: review?(cam)
(Assignee)

Comment 4

4 years ago
Created attachment 8358356 [details] [diff] [review]
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?
Attachment #8357860 - Attachment is obsolete: true
Attachment #8358356 - Flags: review?(cam)
(Assignee)

Comment 5

4 years ago
in SVGTextFrame::GetRunUserSpaceRect maybe I should only set the flag if HasStroke or HasFill is true and ignore (hitTestFlags & SVG_HIT_TEST_STROKE)?
(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 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.
Attachment #8358356 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/1ce60609ad03
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.