Created attachment 201897 [details] [review]
Patch rv. 1.0

Basic support of vertical writing.
This patch implements writing-mode: tb|tb-rl;
# (no glyph-orientation-vertical and glyph-orientation-horizontal)

This patch also improves rendering of x, y, dx, dy attribute of text/tspan in both vertical and horizontal texts.

Created attachment 201898 [details]
Rendering result with patch

+    if (!PANGO_GRAVITY_IS_VERTICAL(gravity)) {
[...]
+    } else {

I think it would be more natural to just invert the order here, so:
if (PANGO_GRAVITY_IS_VERTICAL(...)) {...} else {...}. (Same in all places this occurs.)

+        cairo_rotate (render->cr, -rotation);

Maybe do this conditionally: if (rotation != 0.) ?

     if (state->fill) {
         cairo_move_to (render->cr, x, y);
         rsvg_bbox_insert (&render->bbox, &bbox);
         _set_source_rsvg_paint_server (ctx,
                                        state->current_color,
                                        state->fill,
                                        state->fill_opacity,
                                        bbox, rsvg_current_state (ctx)->current_color);
 
+        cairo_rotate (render->cr, -rotation);
         pango_cairo_show_layout (render->cr, layout);
     }
 
     if (state->stroke) {
         cairo_move_to (render->cr, x, y);
         rsvg_bbox_insert (&render->bbox, &bbox);
+        cairo_save (render->cr);
+        cairo_rotate (render->cr, -rotation);
         pango_cairo_layout_path (render->cr, layout);
+        cairo_restore (render->cr);

This doesn't look right... if it's fill & stroke, won't this rotate the stroke twice? I think both the fill and the stroke case need to be surrounded by cairo_save/restore. (And do that as first/last thing in the block.)

I think the code in rsvg_parse_style_pair() should handle "lr" value explicitly instead of just in the final catch-all else {...}.

Also, can you please attach patches in git format-patch format? That makes applying them easier. Thanks!

Created attachment 202135 [details] [review]
[PATCH 1/3] Set correct value when unicode-bidi: inherit;

Thank you for your review!

In fact, this patch is not required to support vertical writing.
However I noticed that misusage.

Created attachment 202136 [details] [review]
[PATCH 2/3] Remove unused member 'orientation'

RsvgTextLayout's 'orientation' is never used.

Created attachment 202141 [details] [review]
[PATCH 3/3] Support basic vertical writing (Gnome Bug #664533)

This is main patch.

(In reply to comment #3)
> +    if (!PANGO_GRAVITY_IS_VERTICAL(gravity)) {
> [...]
> +    } else {
> 
> I think it would be more natural to just invert the order here, so:
> if (PANGO_GRAVITY_IS_VERTICAL(...)) {...} else {...}. (Same in all places this
> occurs.)

Addressed.

 
> +        cairo_rotate (render->cr, -rotation);
> 
> Maybe do this conditionally: if (rotation != 0.) ?

Addressed.


>      if (state->fill) {
>          cairo_move_to (render->cr, x, y);
>          rsvg_bbox_insert (&render->bbox, &bbox);
>          _set_source_rsvg_paint_server (ctx,
>                                         state->current_color,
>                                         state->fill,
>                                         state->fill_opacity,
>                                         bbox, rsvg_current_state
> (ctx)->current_color);
> 
> +        cairo_rotate (render->cr, -rotation);
>          pango_cairo_show_layout (render->cr, layout);
>      }
> 
>      if (state->stroke) {
>          cairo_move_to (render->cr, x, y);
>          rsvg_bbox_insert (&render->bbox, &bbox);
> +        cairo_save (render->cr);
> +        cairo_rotate (render->cr, -rotation);
>          pango_cairo_layout_path (render->cr, layout);
> +        cairo_restore (render->cr);
> 
> This doesn't look right... if it's fill & stroke, won't this rotate the stroke
> twice? I think both the fill and the stroke case need to be surrounded by
> cairo_save/restore. (And do that as first/last thing in the block.)
> 

Yes. this caused a misrendering and is address.


> I think the code in rsvg_parse_style_pair() should handle "lr" value explicitly
> instead of just in the final catch-all else {...}.
> 

Parsing writing-mode is simplified because lr, rl, and tb are shorthand of lr-tb, rl-tb,and tb-rl in SVG 1.1.
# SVG Tiny 1.2 doesn't support writing-mode (and vertical writing).
Additionally, PANGO_DIRECTION_TTB_XXX are deprecated.
http://developer.gnome.org/pango/stable/pango-Bidirectional-Text.html#PangoDirection

Patch 1/3 and 2/3 look fine; I've pushed them. (Do you have gnome git commit rights, btw?)

Using patch 3/3, the rendering of the attached testcase (attachment 201896 [details]) doesn't match with the rendering in attachment 201898 [details] however...

(In reply to comment #7)
> Patch 1 [details]/3 and 2/3 look fine; I've pushed them. (Do you have gnome git commit
> rights, btw?)

Thanks. And I don't have the commit rights.

> Using patch 3 [details]/3, the rendering of the attached testcase (attachment 201896 [details])
> doesn't match with the rendering in attachment 201898 [details] however...

Hmm, it works fine for me. Could you attach the rendering result?
My environment is following:
* Ubuntu 11.10
* cairo 1.10.2
* pango 1.29.3
* gtk 3.2.0/gtk 2.24.6

It could be pango fails rendering vertical texts if no suitable fonts are installed (I'm not confident though).
In my environment,  several Japanese fonts including
IPA font (http://ossipedia.ipa.go.jp/ipafont/index.html) and
M+ font (http://mplus-fonts.sourceforge.jp/mplus-outline-fonts/index-en.html)
are installed.

Created attachment 202194 [details]
test rendering

The final dots are placed differently than in attachment 201898 [details], and I wasn't sure which rendering is right.

Also the rendering of http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/text-intro-03-b.html isn't right.

Created attachment 202271 [details]
Modified version of text-intro-03-b

(In reply to comment #9)
> Created an attachment (id=202194) [details]
> test rendering
> 
> The final dots are placed differently than in attachment 201898 [details], and I wasn't
> sure which rendering is right.

Some characters (including dots) have two glyphs.
One is for horizontal writing. And the other is for vertical.
http://en.wikipedia.org/wiki/File:Tateyoko.png

However some fonts don't have glyphs for vertical writing.
Thus dots placement can vary.


(In reply to comment #10)
> Also the rendering of
> http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/text-intro-03-b.html
> isn't right.

The test case doesn't include language information (i.e. xml:lang attributes).
So font substitution and rendering can fail if pango fails to detect languages.  
I attached modified version of test case which has xml:lang attributes for each text elements.
This would not produce mis-rendering.

The right two sentences "Se: 私は"/"Japanese: 私は..." use glyph-orientation-vertical property.
Since the patch doesn't support that property, these are rendered likely
center two sentences "Se: 私は"/"Japanese: 私は...".

Created attachment 202272 [details]
Modified version of text-intro-03-b

Oops! I attached wrong file. This is correct.

Created attachment 202273 [details]
Rendering result of modified text-intro-03-b

The rendering result of attachment 202272 [details] with patch.

Thanks for the explanations! I've pushed the patch to master.

(In reply to comment #11)
> Created an attachment (id=202271) [details]
> Modified version of text-intro-03-b
> 
> (In reply to comment #9)
> > Created an attachment (id=202194) [details] [details]
> > test rendering
> > 
> > The final dots are placed differently than in attachment 201898 [details] [details], and I wasn't
> > sure which rendering is right.
> 
> Some characters (including dots) have two glyphs.
> One is for horizontal writing. And the other is for vertical.
> http://en.wikipedia.org/wiki/File:Tateyoko.png
> 
> However some fonts don't have glyphs for vertical writing.
> Thus dots placement can vary.

I guess the same thing results in the middle text column exceeding the image height? The text runs off the bottom at the ま and the せん。are invisible, completely off the bottom.

> (In reply to comment #10)
> > Also the rendering of
> > http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/text-intro-03-b.html
> > isn't right.
> 
> The test case doesn't include language information (i.e. xml:lang attributes).
> So font substitution and rendering can fail if pango fails to detect languages. 
> I attached modified version of test case which has xml:lang attributes for each
> text elements.
> This would not produce mis-rendering.


I guess I just don't have the fonts, since the chinese text at the left still is all printed all glyphs on top of each other.
 
> The right two sentences "Se: 私は"/"Japanese: 私は..." use
> glyph-orientation-vertical property.
> Since the patch doesn't support that property, these are rendered likely
> center two sentences "Se: 私は"/"Japanese: 私は...".

Is that easy to implement? If so I'll leave this bug open for that, otherwise let's file that as a new one.

Created attachment 202348 [details] [review]
Partial support of glyph-orientation-{vertical, horizontal}

This patch adds partial support of glyph-orientation-{vertical, horizontal}.

Correctly rendered:
glyph-orientation-vertical: auto, 0, and 90;
glyph-orientation-horizontal: 0, and 270;


Not correctly rendered:
glyph-orientation-vertical: 180 and 270;
glyph-orientation-horizontal: 90 and 180;

Created attachment 202349 [details]
Testcase for glyph-orientation-{vertical, horizontal}

Created attachment 202350 [details]
Rendering result with patch

Rendering result of attachment 202349 [details] with patch

Created attachment 202351 [details]
Rendering result from Chromium browser (correct)

Chromium browser's rendering of attachment 202349 [details].
This is expected result.

Comparing this, librsvg with patch rotates texts instead of glyphs
if glyph-orientation-vertical is 180 or 270 and glyph-orientation-horizontal is 90 and 180.

Thanks for the patch!

Did you attach the testcase incorrectly? Attachment 202349 [details] is a duplicate of the 'rendering result' attachment 202350 [details].

+            double degree = rsvg_css_parse_angle (value);
+            degree = fmod(degree, 360.);
+            degree = degree < 0 ? 360. + degree : degree;

I think these two extra lines should go into rsvg_css_parse_angle().

+typedef enum {
+  GLYPH_ORIENTATION_VERTICAL_AUTO = PANGO_GRAVITY_AUTO,
+  GLYPH_ORIENTATION_VERTICAL_0 = PANGO_GRAVITY_EAST,
+  GLYPH_ORIENTATION_VERTICAL_90 = PANGO_GRAVITY_SOUTH,
+  /* XXX Below two values dopn't match SVG 1.1 Spec. */
+  GLYPH_ORIENTATION_VERTICAL_180 = PANGO_GRAVITY_WEST,
+  GLYPH_ORIENTATION_VERTICAL_270 = PANGO_GRAVITY_NORTH
+} GlyphOrientationVertical;
+
+typedef enum {
+  GLYPH_ORIENTATION_HORIZONTAL_0 = PANGO_GRAVITY_SOUTH,
+  /* XXX Below two values don't match SVG 1.1 Spec. */
+  GLYPH_ORIENTATION_HORIZONTAL_90 = PANGO_GRAVITY_WEST,
+  GLYPH_ORIENTATION_HORIZONTAL_180 = PANGO_GRAVITY_NORTH,
+  GLYPH_ORIENTATION_HORIZONTAL_270 = PANGO_GRAVITY_EAST
+} GlyphOrientationHorizontal;

Instead of assigning the new enums to the PangoGravity values, I think the enum should just have auto-values and the PangoGravity and PangoGravityHint be retrieved from a function get_[horiz|vert]_gravity_and_hint (GlyphOrientation[Horizontal|Vertical] orient, PangoGravity *grav, PangoGravityHint *hint). That makes it easier to understand, IMHO.

-    iter = pango_layout_get_iter (layout);
+    /* Align characters to the natural baseline */
+    tmp_layout = pango_layout_copy(layout);
+    pango_context_set_base_gravity(context, state->text_gravity);
+    pango_context_set_gravity_hint(context, PANGO_GRAVITY_HINT_NATURAL);
+    pango_layout_context_changed (tmp_layout);
+    iter = pango_layout_get_iter (tmp_layout);

Could you explaing why you need to make a copy here and change the gravity?

Also, why the pango_layout_context_changed() ­— from its docs I don't see why it would be necessary here?

Ping, any updates?

Ping again?

Sorry for long delay.
I'll be look at in next Saturday and Sunday.

Created attachment 223854 [details] [review]
Updated patch for glyph-orientation-{vertical, horizontal}

Addressed review comments.
Changed to use default value's gravity/gravity_hint for unsupported values.

(In reply to Kurosawa Takeshi from comment #23)
> Created attachment 223854 [details] [review] [review]
> Updated patch for glyph-orientation-{vertical, horizontal}
> 
> Addressed review comments.
> Changed to use default value's gravity/gravity_hint for unsupported values.

Wondering if this could get a review?