Bug 50365 – Glyphs in vertical text tests are rotated 90 degrees clockwise on Chromium Linux

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Koan-Sin Tan]

Created attachment 75343 [details]
A patch to make Chromium Linux render vertical text correctly

As discussed in https://bugs.webkit.org/show_bug.cgi?id=48459, If you run fast/blockflow/japanese-*-text.html on Windows you'll see that the glyphs are rotated 90 degrees clockwise from how they appear on WebKit Mac and Chromium Mac. The reason is that vertical text is implemented on any platform except Mac OS X (WebCore/platform/graphics/{mac, cocoa} ).

[Kent Tamura]

Comment on attachment 75343 [details]
A patch to make Chromium Linux render vertical text correctly

View in context: https://bugs.webkit.org/attachment.cgi?id=75343&action=review

I'm not familiar with this area.  So I make some comments on patch style.

* Please add a ChangeLog entry. WebCore/ChangeLog.
* Please make a patch at the top level directory.  You made the patch at WebKit/WebCore, should make at WebKit/.

You can use WebKitTools/Scripts/webkit-patch to make an acceptable patch.

> platform/graphics/chromium/FontLinux.cpp:106
> +            pos2[i].set(x+SkFloatToScalar(adv[i].width()), y);
> +            pos3[i].set(x+SkFloatToScalar(adv[i].width()), y-SkFloatToScalar(adv[i].width())); 

Add spaces around operators such as + -

> platform/graphics/chromium/FontLinux.cpp:132
> +                canvas->drawTextOnPath(glyphs+i, 2, path, 0, paint);

Add spaces around +

> platform/graphics/chromium/FontLinux.cpp:136
> +        } else {
> +            canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint);
> +        }

Remove { and }

> platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:72
> +            for (int i=0; i < bufferLength; i++) {
> +                hb_buffer_add_glyph(buffer, glyphs[i], 0, i);
> +            }

Add spaces around =.
Remove { and }

> platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:80
> +            HB_GSUB_Select_Script(hb_face->gsub, 
> +                        HB_MAKE_TAG('D', 'F', 'L', 'T'), &script_index);
> +            HB_GSUB_Select_Feature(hb_face->gsub,
> +                        HB_MAKE_TAG('v', 'e', 'r', 't'), script_index,
> +                         0xffff, &feature_index);

You don't need wrap these lines. WebKit style allow looooong lines.

> platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:90
> +                for (int i=0; i < bufferLength; i++) {
> +                    glyphs[i] = (Glyph) buffer->out_string[i].gindex;
> +                }

Remove { and }.
We prefer C++-style cast.

[Koan-Sin Tan]

Created attachment 75433 [details]
A patch to make Chromium Linux render vertical text correctly

add an entry to WebCore/ChangeLog and change a bit to conform WebKit coding style

[WebKit Review Bot]

Attachment 75433 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/graphics/chromium/FontCacheLinux.cpp', u'WebCore/platform/graphics/chromium/FontLinux.cpp', u'WebCore/platform/graphics/chromium/FontPlatformDataLinux.cpp', u'WebCore/platform/graphics/chromium/FontPlatformDataLinux.h', u'WebCore/platform/graphics/skia/FontCustomPlatformData.cpp', u'WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp']" exit_code: 1
WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Koan-Sin Tan]

Created attachment 75437 [details]
A patch to make Chromium Linux render vertical text correctly

change the place of #include <HarfbuzzSkia.h> to make the bot happy :-)

[Koan-Sin Tan]

Created attachment 75444 [details]
A patch to make Chromium Linux render vertical text correctly

Change C cast to C++ cast. Hope I get all thing right this time

[Eric Seidel (no email)]

Comment on attachment 75437 [details]
A patch to make Chromium Linux render vertical text correctly

View in context: https://bugs.webkit.org/attachment.cgi?id=75437&action=review

You need AGL's or evmar's review here.  But testing is the major failing of this patch.  Please clarify which tests cover this and ideally include the updated results.

> WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This will cause the cq to fail.  You should mention which tests this affects.

> WebCore/platform/graphics/chromium/FontLinux.cpp:106
> +            pos2[i].set(x + SkFloatToScalar(adv[i].width()), y);
> +            pos3[i].set(x + SkFloatToScalar(adv[i].width()), y-SkFloatToScalar(adv[i].width())); 

Seems we could use a local here to save about 50 chars of typing. :)

> WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:68
> +        if (hbFace->gsub) {
> +            HB_Buffer buffer;

Either early return or using a helper function for this big clause would make this clearer.

> WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:81
> +                // FIXME : how/what to do 

This isn't a very helpful fixme (nor is it a complete sentence with capital and period).

[WebKit Review Bot]

Attachment 75437 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6732021

[WebKit Review Bot]

Attachment 75444 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6791016

[Koan-Sin Tan]

Created attachment 75471 [details]
A patch to make Chromium Linux render vertical text correctly

Changed according to Eric Seidel's comments.

[Koan-Sin Tan]

Created attachment 76016 [details]
An initial patch to make Chromium Linux render vertical text correctly

Make vertical position variables more descriptive

[Kent Tamura]

Comment on attachment 76016 [details]
An initial patch to make Chromium Linux render vertical text correctly

View in context: https://bugs.webkit.org/attachment.cgi?id=76016&action=review

style comments.

> WebCore/platform/graphics/chromium/FontPlatformDataLinux.cpp:83
> +#if 0

Unused code should be removed.

> WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:65
> +        if (!error) 

Should add { } because the block has two physical lines.

> WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:70
> +    } else
> +        // if there is no GSUB table, treat it as not covered
> +        error = 0Xffff;

ditto.

[Koan-Sin Tan]

Created attachment 76018 [details]
an initial patch to make Chromium Linux render vertical text correctly

same as 76016, removed unused code and add {}s

[Xiaomei Ji]

Comment on attachment 76018 [details]
an initial patch to make Chromium Linux render vertical text correctly

View in context: https://bugs.webkit.org/attachment.cgi?id=76018&action=review

> WebCore/platform/graphics/chromium/FontLinux.cpp:163
> +            canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint);

will it be better to have another function instead of duplicating the above code?

[Kent Tamura]

Can someone try this patch locally and see if LayoutTests/fast/blockflow/Kusa-Makura-background-canvas.html is rendered correctly?

[Koan-Sin Tan]

(In reply to comment #14)
> Can someone try this patch locally and see if LayoutTests/fast/blockflow/Kusa-Makura-background-canvas.html is rendered correctly?

work for me:-)  
See http://www.flickr.com/photos/koansin/5248686548/, scrollbar works too

[Koan-Sin Tan]

(In reply to comment #13)
> (From update of attachment 76018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76018&action=review
> 
> > WebCore/platform/graphics/chromium/FontLinux.cpp:163
> > +            canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint);
> 
> will it be better to have another function instead of duplicating the above code?

Yes, definitely I can do that. But that will pass some parameters to the function and create some overhead. Is it the supposed to be the right way?

[Yuta Kitamura]

Created attachment 76170 [details]
Ruby text is sometimes missing

I built chromium-linux with this patch and tried it out.

Basically it works pretty well (good job!). But I've found a few rough edges:

* When you scroll the document (horizontally), ruby text is sometimes incomplete or missing. See the attached screenshot.

* If you move your mouse over the text, many console messages like the following will appear:
>[24529:24529:2205928082:ERROR:webkit/glue/webcursor_gtk.cc(132)] Not implemented reached in int WebCursor::GetCursorType() const

 (I don't know this message is related to this patch though)

[Kent Tamura]

(In reply to comment #15)
> (In reply to comment #14)
> > Can someone try this patch locally and see if LayoutTests/fast/blockflow/Kusa-Makura-background-canvas.html is rendered correctly?
> 
> work for me:-)  
> See http://www.flickr.com/photos/koansin/5248686548/, scrollbar works too

Cool!

However, the positions of "、" and "。" look incorrect.  They should be aligned to the right side of other characters.  See http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/blockflow/Kusa-Makura-background-canvas-expected.png
Is it expected?

[Koan-Sin Tan]

(In reply to comment #18)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Can someone try this patch locally and see if LayoutTests/fast/blockflow/Kusa-Makura-background-canvas.html is rendered correctly?
> > 
> > work for me:-)  
> > See http://www.flickr.com/photos/koansin/5248686548/, scrollbar works too
> 
> Cool!
> 
> However, the positions of "、" and "。" look incorrect.  They should be aligned to the right side of other characters.  See http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/blockflow/Kusa-Makura-background-canvas-expected.png
> Is it expected?

Nope, that's because my fallback font don't have the GSUB/vert table, I think.
Those punctuations are supposed be replaced by vertical presentation glyph
by looking up the GSUB/vert table

[Koan-Sin Tan]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > Can someone try this patch locally and see if LayoutTests/fast/blockflow/Kusa-Makura-background-canvas.html is rendered correctly?
> > > 
> > > work for me:-)  
> > > See http://www.flickr.com/photos/koansin/5248686548/, scrollbar works too
> > 
> > Cool!
> > 
> > However, the positions of "、" and "。" look incorrect.  They should be aligned to the right side of other characters.  See http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/blockflow/Kusa-Makura-background-canvas-expected.png
> > Is it expected?
> 
> Nope, that's because my fallback font don't have the GSUB/vert table, I think.
> Those punctuations are supposed be replaced by vertical presentation glyph
> by looking up the GSUB/vert table

Argh, wrong answer, what you mentioned is 100% font problem. I was talking about another problem, such as 「」『』 replacement, see  http://www.flickr.com/photos/koansin/5248695650/

[Yuta Kitamura]

Created attachment 76172 [details]
Em dash is not aligned correctly

I've noticed another issue: "em dash" character should be located at the center of the text, but actually does not align correctly.

I'd like to leave the decision when to fix any of bugs I have found up to the patch author and reviewers; It sounds okay for me to file bugs for now and fix them later.

[Koan-Sin Tan]

(In reply to comment #17)
> Created an attachment (id=76170) [details]
> Ruby text is sometimes missing
> 
> I built chromium-linux with this patch and tried it out.
> 
> Basically it works pretty well (good job!). But I've found a few rough edges:
> 
> * When you scroll the document (horizontally), ruby text is sometimes incomplete or missing. See the attached screenshot.
> 

I cannot reproduce it :-(
Current I rotate glyphs one by one because I cannot find anything in Skia similar to Cairo's font matrix.
Maybe this caused the refresh problem


> * If you move your mouse over the text, many console messages like the following will appear:
> >[24529:24529:2205928082:ERROR:webkit/glue/webcursor_gtk.cc(132)] Not implemented reached in int WebCursor::GetCursorType() const
> 
>  (I don't know this message is related to this patch though)

It's there before my patch, I think.

[Koan-Sin Tan]

Created attachment 76340 [details]
snapshot of Kusamakura with proper font

body {font-family: "Hiragino Micho Pro";} 

as expected, the locations of punctuations, including comma and em-dash, look right

[Kent Tamura]

If no one objects, I'll set r+, commit the patch, and update pixel results.

[Koan-Sin Tan]

Created attachment 76742 [details]
an initial patch to make Chromium Linux render vertical text correctly

same as 76018, avoid looking up GSUB table when all the glyphs is Font::isCJKIdeographs()

[Kent Tamura]

Comment on attachment 76742 [details]
an initial patch to make Chromium Linux render vertical text correctly

ok.  I'll commit it.

[Kent Tamura]

Landed as http://trac.webkit.org/changeset/74232 with some minor modifications.

[Kent Tamura]

Rebaseline done by http://trac.webkit.org/changeset/74233

Koan-Sin Tan, thank you for making this change!

[Koan-Sin Tan]

(In reply to comment #28)
> Rebaseline done by http://trac.webkit.org/changeset/74233
> 
> Koan-Sin Tan, thank you for making this change!

No problem. Thanks for your review. I'd like to see CSS Writing Mode works on my favoriate browser.

This is initial work. Some work remains to be done. E.g.,
1. the system fallback font: on my Ubutun, it's WenQuanYi Zen Hei, which doesn't display those test cases well. This may be trivial and easy work, but need some time to construct a table, maybe.

2. Non- CJK glyphs are not supposed to be rotated like CJK glyphs. This is not easy, because it seems there is no way to check if a glyph is a CJK glyph or not in Font::drawGlyphs() without modifying struct GlyphData.

3. Mongolian and some other scripts also use vertical writing.