Bug 12698 (CharLayout) - SVG text needs a special per-character layout mode.
Summary: SVG text needs a special per-character layout mode.
Status: RESOLVED FIXED
Alias: CharLayout
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
: 6420 6425 6481 12376 12377 12574 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-02-08 09:18 PST by Nikolas Zimmermann
Modified: 2007-10-18 12:37 PDT (History)
5 users (show)

See Also:


Attachments
Initial patch (27.35 KB, patch)
2007-02-08 09:23 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (74.21 KB, patch)
2007-03-04 04:36 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
First complete patch (94.52 KB, patch)
2007-03-06 17:30 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch to work against current ToT (99.77 KB, patch)
2007-04-30 02:02 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Updated patch including text selection (121.58 KB, patch)
2007-05-16 17:08 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Layout test results (new directory: svg/batik) (473.41 KB, patch)
2007-05-16 17:18 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch including text selection (138.39 KB, patch)
2007-05-19 17:23 PDT, Nikolas Zimmermann
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2007-02-08 09:18:59 PST
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.
Comment 1 Nikolas Zimmermann 2007-02-08 09:19:56 PST
*** Bug 6420 has been marked as a duplicate of this bug. ***
Comment 2 Nikolas Zimmermann 2007-02-08 09:21:35 PST
*** Bug 12376 has been marked as a duplicate of this bug. ***
Comment 3 Nikolas Zimmermann 2007-02-08 09:22:26 PST
*** Bug 12377 has been marked as a duplicate of this bug. ***
Comment 4 Nikolas Zimmermann 2007-02-08 09:23:36 PST
Created attachment 13058 [details]
Initial patch

Initial - not yet complete (see bug description) - patch.
Comment 5 Nikolas Zimmermann 2007-02-08 14:12:03 PST
(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
Comment 6 Nikolas Zimmermann 2007-03-04 04:35:08 PST
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!
Comment 7 Nikolas Zimmermann 2007-03-04 04:36:29 PST
Created attachment 13471 [details]
Updated patch

Show some progress to the world! :-)
Comment 8 Nikolas Zimmermann 2007-03-04 04:37:23 PST
*** Bug 6425 has been marked as a duplicate of this bug. ***
Comment 9 Nikolas Zimmermann 2007-03-04 04:37:36 PST
*** Bug 6481 has been marked as a duplicate of this bug. ***
Comment 10 Nikolas Zimmermann 2007-03-04 04:39:09 PST
*** Bug 12574 has been marked as a duplicate of this bug. ***
Comment 11 Nikolas Zimmermann 2007-03-06 17:30:50 PST
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...
Comment 12 Oliver Hunt 2007-03-20 22:45:40 PDT
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?
Comment 13 Dave Hyatt 2007-04-19 02:05:59 PDT
I think we should get this into the tree now.  I think things have stabilized sufficiently to go ahead and take it.
Comment 14 Oliver Hunt 2007-04-30 02:02:58 PDT
Created attachment 14273 [details]
Updated patch to work against current ToT
Comment 15 Nikolas Zimmermann 2007-05-16 15:22:37 PDT
*** Bug 11941 has been marked as a duplicate of this bug. ***
Comment 16 Nikolas Zimmermann 2007-05-16 17:08:51 PDT
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 :-)
Comment 17 Nikolas Zimmermann 2007-05-16 17:18:00 PDT
Created attachment 14587 [details]
Layout test results (new directory: svg/batik)
Comment 18 Oliver Hunt 2007-05-16 17:35:43 PDT
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?
Comment 19 Nikolas Zimmermann 2007-05-17 04:55:59 PDT
(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
Comment 20 Maciej Stachowiak 2007-05-17 17:49:04 PDT
Maybe it would make more sense to land this on the feature branch for now.
Comment 21 Nikolas Zimmermann 2007-05-19 04:22:56 PDT
(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
Comment 22 Nikolas Zimmermann 2007-05-19 17:23:39 PDT
Created attachment 14632 [details]
Updated patch including text selection

Final version that should be comittable.
Comment 23 Nikolas Zimmermann 2007-05-19 18:45:19 PDT
Landed in the new feature-branch in r21607.
Comment 24 Bruce Rindahl 2007-07-18 08:40:07 PDT
I would love to see this land on the trunk soon.
Comment 25 Andreas Neumann 2007-07-19 03:14:11 PDT
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
Comment 26 Bruce Rindahl 2007-10-13 14:45:12 PDT
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
Comment 27 Eric Seidel (no email) 2007-10-13 16:23:59 PDT
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).
Comment 28 Oliver Hunt 2007-10-13 18:01:20 PDT
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
Comment 29 Bruce Rindahl 2007-10-18 12:37:08 PDT
Thanks!
Great news
Bruce