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?

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/librsvg/issues/55.