Bug 10380 – tspans don't support styles

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Oliver Hunt]

tspans don't support style, or have any support for, well anything.

[Oliver Hunt]

Created attachment 10013 [details]
initial renderer for tspan support

Having finally undone the immense damage done the standard text rendering here's an initial patch to support style

This is basically a rehash of the standard <text> renderer, simply to get a base platform for styles to be applied.  Currently blats layout as i've made tspans become blocks which currently triggers a wrap.  As yet i'm not sure what the best solution to this will be.

I believe a second layout pass should be added following the initial renderblock layout method that repositions the generated elements.  Initially this will just line everything up in good, wholesome order, although i just realised that may eat whitespace... :-/

Anyhoo, once that's going paint will need to be fixed, as will the paint method for the standard svg text renderer, as the current paint methods make absolute positioning painful...

[Eric Seidel (no email)]

(In reply to comment #1)
> Created an attachment (id=10013) [edit]
> initial renderer for tspan support

Your patch does not contain the new files.

Once it does, hyatt shoult take a look at it.

[Oliver Hunt]

Created attachment 10017 [details]
With new files

I am a muppet, new files now included

[Oliver Hunt]

Just to note, currently the tspan renderer is basically a stripped down version of the svgtext renderer.  In the future (when the paint methods are tidied up) it may (in fact should) be possible to merge them into a single renderer.

[Oliver Hunt]

Created attachment 10532 [details]
Styled tspans patch attempt #4 million

Okles, as it currently stands i'm not happy with how vertical alignment is performed... may be able to just align it manually, which may prove to be best approach (and maybe faster?)

also, this patch does slightly better at interacting with inspector... but it still gets it wrong -- node lookup works better though.

finally, this current patch break filter support for text -- which is incredibly irritating :-/

[Nikolas Zimmermann]

Heya Oliver,

excellent work! Doing some kind of review now:

+#include "RenderSVGInlineText.h"
...
 RenderObject *Text::createRenderer(RenderArena *arena, RenderStyle *style)
 {
+    if(parentNode()->isSVGElement())
+        return new (arena) RenderSVGInlineText(this, str);
..

Both the include and the isSVGElement() needs to be in a SVG_SUPPORT block.

+RenderSVGInlineText::RenderSVGInlineText(Node *n, StringImpl *str) : RenderText(n, str) { }

Style issues: use Node* n, StringImpl* str and new lines for { }

+    virtual const char *renderName() const { return "RenderSVGInlineText"; }

Star misplaced.


+    SVGTextElement *text = static_cast<SVGTextElement *>(element());

Stars misplaced.

+void SVGInlineTextBox::paint(RenderObject::PaintInfo& paintInfo, int tx, int ty){

{ should go to a new line.

+    const SVGRenderStyle *svgStyle = object()->style()->svgStyle();

Star misplaced.

+    KCanvasFilter* filter = getFilterById(object()->document(), svgStyle->filter().substring(1));

Not related to your patch, but it reminds me how ugly the
referencing is atm, with this substring(1) stuff ... ewwww

+    KRenderingPaintServer *fillPaintServer = KSVGPainterFactory::fillPaintServer(object()->style(), object());
...
+    KRenderingPaintServer *strokePaintServer = KSVGPainterFactory::strokePaintServer(object()->style(), object());

Stars misplaced.

+    bool isSVG=m_object&&m_object->element()&&m_object->element()->isSVGElement();
+    placeBoxesVertically(isSVG?heightOfBlock-maxAscent:heightOfBlock, maxHeight, maxAscent, strictMode, topPosition, bottomPosition, selectionTop, selectionBottom);
 
Spaces between operators, really! ;-)
Also needs a SVG_SUPPORT block.

Style issues only, other than that it rocks! Great work!

[Eric Seidel (no email)]

Comment on attachment 10532 [details]
Styled tspans patch attempt #4 million

Yup, as WildFox noted:
+    if(parentNode()->isSVGElement())
+        return new (arena) RenderSVGInlineText(this, str);
needs SVG_SUPPORT wrappers

mat is poor name for a variable:
+    AffineTransform mat = absoluteTransform();
+    rects.append(enclosingIntRect(mat.mapRect(FloatRect(0, 0, width(), height()))));

sleep now, review later.

[Oliver Hunt]

Created attachment 10813 [details]
styled tspan patch #4 million and one

and again...

[Eric Seidel (no email)]

Comment on attachment 10813 [details]
styled tspan patch #4 million and one

marking this for review so others will take a peak.

I've looked through, there are definitely some style issues left to resolve.  I'm also not certain the placement of the filter/clip/mask code is correct.  I don't think we want to filter the individual boxes, unless there is only one box per tspan/text (since that would cause blurs to be off).

Also, SVG_SUPPORT is missing from dom/Text.cpp and possibly other core files.

Some example style issues:

+RenderSVGTSpan::RenderSVGTSpan(Node *n):RenderInline(n) {}

Node* n not Node *n
also generally : RenderInline is on the next line, indented
and in this case { } would be on separate lines after that

+    InlineFlowBox* initFlow=firstLineBox();

generally we use spaces before and after =

This could benefit from argument names:
+    virtual InlineBox* createInlineBox(bool, bool, bool isOnlyRun = false);


+void SVGInlineTextBox::paint(RenderObject::PaintInfo& paintInfo, int tx, int ty){

{ should go on a separate line.

Nice work olliej.  I can't wait to see this land.

[Oliver Hunt]

Created attachment 10859 [details]
reformatting patch

[Oliver Hunt]

Comment on attachment 10859 [details]
reformatting patch

found bug in handling of transparency

[Oliver Hunt]

My feeling is that i need to get tspans into the used render tree -- it seems the sanest way of insuring opacity, filters, etc get applied properly.  I have a small local fix for transparency, but it only fixes opacity, and none of the other problems.

[Oliver Hunt]

Created attachment 10918 [details]
Now with anchoring and positioning

There are still a few hackish places, and text-filter is still borked...

[Eric Seidel (no email)]

Comment on attachment 10918 [details]
Now with anchoring and positioning

A bunch of these "if(" should be "if (" (spacing)

This should be multiple lines:
+RenderSVGInline::RenderSVGInline(Node *n) : RenderInline(n) {}
also the * is in the "wrong" place.

Shouldn't need RenderObject:: here:
+    RenderObject::PaintInfo pi = paintInfo;

As demonstrated recently in a separate bug, this will need a c->clip(boundingBox) call:
+    if (opacity < 1.0f)
+        c->beginTransparencyLayer(opacity);

+    boundingBox=object->absoluteTransform().mapRect(boundingBox);
 need some spacing around the =

Also, I don't think you want to be using the aboluteTransform here:
+    boundingBox=object->absoluteTransform().mapRect(boundingBox);

Filters, clip, etc. all take the relativeBBox (normally).

Old comments like this can be removed:

+        // fillPaintServer->setActiveClient(this);

Our modern style does constructors slightly differently, with each m_ on its own line:

+    SVGInlineFlowBox(RenderObject *obj) : InlineFlowBox(obj), m_hasAbsx(false), 
+      m_hasAbsy(false), m_absx(0), m_absy(0), m_dx(0), m_dy(0) 
+    {
+        m_isSVGFlow = true;
+    }


The { goes on teh same line as the for():
+    for (InlineBox* curr = firstChild(); curr; curr = curr->nextOnLine()) 
+    {
again for all of the if's in that file:
+    if (positioned)
+    {

This doesn't strike me as a very helpful comment:
+    //SVG flowbox layout rules are different from html, etc
it should be clarified or removed.

Maybe "ignoreX, ignoreY" would be better here?
+        int temp1, temp2; //throwaway values

Why these constants?
+        int top = 100000, bottom = -100000, baseline = heightOfBlock;

[Oliver Hunt]

Created attachment 10939 [details]
with selection issues fixed

And again, now generates correct repaint info for selection, and supports local transforms (oops)

[Oliver Hunt]

Comment on attachment 10939 [details]
with selection issues fixed

obsoleted, and i spotted more formatting issues *sigh*

[Oliver Hunt]

Created attachment 10972 [details]
Final (hopefully) patch

Fixes clipping, masking, and filtering

[Oliver Hunt]

Created attachment 10973 [details]
removed modification to RenderText

the remaining issue would seem to be the use of a m_isSVGFlow flag instead of virtual methods for layout.  I'm hesitant to use virtual methods in the middle of the layout code as that introduces new virtual calls to the layout code for non-svg files as well.

[Oliver Hunt]

Created attachment 10988 [details]
with darins comments

[Dave Hyatt]

Patch looks good to me.  I only have one objection really.

I would rather you remove the SVG flow bit and make placesBoxesHorizontally and placeBoxesVertically virtual.  Then you can just subclass them and do your SVG work instead (and move your new place*** functions into your SVG flow box subclass).  That should keep all SVG code out of InlineFlowBox and should perform just fine.

Also, a minor nitpick that you can choose to fix or not... the name RenderSVGInlineText is pretty redundant... I'd just call it RenderSVGText (that way you have consistently named subclasses that just have the word SVG in between Render and the rest of the class name).

[Oliver Hunt]

Created attachment 10997 [details]
removed m_isSVGFLow, using virtual methods to perform flow layout

Haven't renamed RenderSVGInlineText to RenderSVGText as RenderSVGText as RenderSVGText exists, and is for a <text> element rather than #text :-/

Also as hyatt noted this needs performance testing to check the impact of the changes to Text::createRenderer

[Oliver Hunt]

Comment on attachment 10997 [details]
removed m_isSVGFLow, using virtual methods to perform flow layout

This fails a number of tests now :-/

[Oliver Hunt]

Comment on attachment 10997 [details]
removed m_isSVGFLow, using virtual methods to perform flow layout

No, i was wrong, it's fine -- still needs perf testing though

[Oliver Hunt]

Created attachment 10998 [details]
yet more formatting changes

[Dave Hyatt]

YOu don't need parentPaintMethod, since RootInlineBox doesn't do anything you care about.

So you can just do

flow->InlineFlowBox::paint

instead.

[Oliver Hunt]

Created attachment 11004 [details]
incorporating suggestions from hyatt

[Dave Hyatt]

Comment on attachment 11004 [details]
incorporating suggestions from hyatt

r=me

[Maciej Stachowiak]

Make sure to performance test this before landing, since it added virtual method calls to some potentially hot code.