Bug 10380

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...

(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.

Created attachment 10017 [details] With new files I am a muppet, new files now included

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.

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 :-/

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!

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.

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

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.

Created attachment 10859 [details] reformatting patch

Comment on attachment 10859 [details] reformatting patch found bug in handling of transparency

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.

Created attachment 10918 [details] Now with anchoring and positioning There are still a few hackish places, and text-filter is still borked...

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;

Created attachment 10939 [details] with selection issues fixed And again, now generates correct repaint info for selection, and supports local transforms (oops)

Comment on attachment 10939 [details] with selection issues fixed obsoleted, and i spotted more formatting issues *sigh*

Created attachment 10972 [details] Final (hopefully) patch Fixes clipping, masking, and filtering

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.

Created attachment 10988 [details] with darins comments

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).

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

Comment on attachment 10997 [details] removed m_isSVGFLow, using virtual methods to perform flow layout This fails a number of tests now :-/

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

Created attachment 10998 [details] yet more formatting changes

YOu don't need parentPaintMethod, since RootInlineBox doesn't do anything you care about. So you can just do flow->InlineFlowBox::paint instead.

Created attachment 11004 [details] incorporating suggestions from hyatt

Comment on attachment 11004 [details] incorporating suggestions from hyatt r=me

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