Bug (CharLayout) – SVG text needs a special per-character layout mode.

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

To be able to implement mutliple x/y, dx/dy, rotate values properly, SVG text needs a special layout mode. vertical text & textPath can be built upon that - once it's done.

Attaching patch, which implements a new hot per-character layout mode.
x/y, dx/dy lists seem to work fine in all combinations.
rotate lists work fine, too.

text seletion is _DISABLED_ for per character layout mode and needs discussion - not yet sure how to properly implement that. vertical text is not yet "hooked in" - but you can switch a variable "isVerticalText" in the code to "true" to actually see vertical text :-)

Another problem is the width/height calculation of our InlineText/Flow boxes:
- rotate changes the size of the boxes - not taken into account yet.
- y lists may enlarge the size of the boxes - not taken into account yet.
 (only x lists are handled so far in the patch - see SVGInlineFlowBox)

These size calculations are easy to fix - just don't have time atm.
Please comment.

[Nikolas Zimmermann]

*** Bug 6420 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 12376 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 12377 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 13058 [details]
Initial patch

Initial - not yet complete (see bug description) - patch.

[Nikolas Zimmermann]

(In reply to comment #4)
> Created an attachment (id=13058) [edit]
> Initial patch
> 
> Initial - not yet complete (see bug description) - patch.
> 

Hm I am not sure anymore wheter we should do the actual width/height/xPos/yPos calculation in SVGInlineFlowBox for the per-character layout mode. We could easily do this in "layoutSingleObject" in SVGRootInlineBox, and don't use the placeSVGFlowHorizontally/placeSVGFlowVertically logic at all for per-character layout mode situations. Though I won't waste time in trying out this stuff before talking to Oliver/Eric/Rob... We need to discuss a masterplan on how to go on here - as it's just too important to get that right :-)

I'm blocked by exams until next week Wednesday - and well it's "stabilization spree" anyway, so we don't need to hurry at all. Have a nice week(end)!

Niko

[Nikolas Zimmermann]

Uploading a new patch soon, showing how far I got so far:

baseline-shift is implemented, x/y/dx/dy lists work perfectly (even all Batik examples for this!)
text-anchor now works "per text chunks", and fixes lots of misrenderings.
text on path layout, as well as vertical text are implemented.

Still a lot of TODO: text-anchor on text path, kerning property, letter-spacing/word-spacing has to be done manually to also work in path layouts, textLength/textAdjust etc.. etc..

Some screenshots:
http://www.flickr.com/photos/43532360@N00/409816149/ (text-on-path)
http://www.flickr.com/photos/43532360@N00/409816151/ (text-on-path)
http://www.flickr.com/photos/43532360@N00/409303358/ (vertical text)
http://www.flickr.com/photos/43532360@N00/409308387/ (vertical text)
http://www.flickr.com/photos/43532360@N00/381107759/ (per character layout, shows one misplaced character though: the "," in Cute and Fuzzy is misplaced - fixed locally since a week)

Stay tuned!

[Nikolas Zimmermann]

Created attachment 13471 [details]
Updated patch

Show some progress to the world! :-)

[Nikolas Zimmermann]

*** Bug 6425 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 6481 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 12574 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 13500 [details]
First complete patch

No ChangeLog yet, as this is probably not the last patch before the commit :-)
Added the new text engine as experimental feature, aka. can be build with build-webkit --svg-experimental...

[Oliver Hunt]

Comment on attachment 13500 [details]
First complete patch

> FIXME - svgChar.x/y are absolute positions, need to respect ctm here!

This (and similar) should have bugs filed if they still exist when this is landed

This looks good, i'm hesitant to r=me-ify it as someone will probably land it.

Finally -- I take it IDL has no enunmerated type equivalent?

[Dave Hyatt]

I think we should get this into the tree now.  I think things have stabilized sufficiently to go ahead and take it.

[Oliver Hunt]

Created attachment 14273 [details]
Updated patch to work against current ToT

[Nikolas Zimmermann]

*** Bug 11941 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 14586 [details]
Updated patch including text selection

Finally a new patch including the text selection work of the last months. I have been very busy - so finally after ~ 5 months a first comitable patch. Have fun reviewing :-)

[Nikolas Zimmermann]

Created attachment 14587 [details]
Layout test results (new directory: svg/batik)

[Oliver Hunt]

Comment on attachment 14586 [details]
Updated patch including text selection

You will hate me for this:
class and struct fields should have the m_ prefix

Do you really want to retain the debug code in applyTextAnchorToTextChunk?

why does totalAdvanceOfInlineBox take a ref paramter instead of just returning the length?  Or is this a preexisting method?

[Nikolas Zimmermann]

(In reply to comment #18)
> (From update of attachment 14586 [details] [edit])
> You will hate me for this:
Definately ;-)

> class and struct fields should have the m_ prefix
I guess I can do that before landing - aka. not upload a new patch just for this.
 
> Do you really want to retain the debug code in applyTextAnchorToTextChunk?
Yeah, it is essential for finding the corner cases for text selection.
 
> why does totalAdvanceOfInlineBox take a ref paramter instead of just returning
> the length?  Or is this a preexisting method?
totalAdvanceOfInlineBox is a recursive method? I need to pass around a ref.
Only the totalAdvanceOfInlineTextBox could be modified as you said..

Greetings,
Niko

[Maciej Stachowiak]

Maybe it would make more sense to land this on the feature branch for now.

[Nikolas Zimmermann]

(In reply to comment #20)
> Maybe it would make more sense to land this on the feature branch for now.
> 
Hi Maciej,

I've discussed this with Oliver - and we both find the option: land in trunk as experimental feature best. Because it's a) all prepared in this patch (builds flawlessly in experimental/non-experimental mode) and b) easiest way to merge in the normal build in the future and c) easy way for ppl to try out (just get trunk, and set the right build flags)

What do you think?

Greetings,
Niko

[Nikolas Zimmermann]

Created attachment 14632 [details]
Updated patch including text selection

Final version that should be comittable.

[Nikolas Zimmermann]

Landed in the new feature-branch in r21607.

[Bruce Rindahl]

I would love to see this land on the trunk soon.

[Andreas Neumann]

yes, I also think that the functionality from this patch is essential to the final release (main trunk). Please prioritize and move it to the main trunk.

I use the functionality provided by this patch in many SVG projects.

Thanks a lot,
Andreas

[Bruce Rindahl]

I hate to be annoying ;) but could the feature-branch be available as a special nightly?  You would have lots of potential testers on some of these advanced but must have features.  Please consider this ASAP.  Thanks again.  Bruce

[Eric Seidel (no email)]

Well, I have good news for you:

1.  It was available as a special nightly:
http://nightly.webkit.org/builds/overview/feature-branch

2.  BUT, even better: feature-branch just got rolled into trunk last night.  So:
http://nightly.webkit.org/ should have these changes tomorrow (if it doesn't already today).

[Oliver Hunt]

The feature-branch no longer exists, it was merged into trunk yesterday.

Feature-branch has been available as a nightly for a few weeks now :D

*Theoretically* a nightly should be available shortly which will be post merge (you're looking for a version > r26552

[Bruce Rindahl]

Thanks!
Great news
Bruce