Bug 53480 – REGRESSION (r76674): SVG + BiDi text broken (text-intro-05-t.svg)

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Between r76727 and r76640, LayoutTests/svg/W3C-SVG-1.1/text-intro-05-t.svg broke.
Half of the text is not visible anymore. I suspect a problem in either FontComplexMac.mm or ComplexTextController.cpp.

Some things have changed recently, and that probably broke it.
Mitz can you have a look?

With my build of r77063, run-webkit-tests --pixel svg/W3C-SVG-1.1/text-intro-05-t.svg reports success. I don’t see missing text nor do I see a difference between what the test looks like in Safari with that build and what it looks like in an older build. That is on Snow Leopard, but perhaps you are running the test on Leopard (the combination PC / Mac OS X 10.5 makes no sense).

 Can you attach screenshots showing the failure?

[Nikolas Zimmermann]

Created attachment 80726 [details]
Screenshot

[Nikolas Zimmermann]

(In reply to comment #1)
> With my build of r77063, run-webkit-tests --pixel svg/W3C-SVG-1.1/text-intro-05-t.svg reports success. I don’t see missing text nor do I see a difference between what the test looks like in Safari with that build and what it looks like in an older build. That is on Snow Leopard, but perhaps you are running the test on Leopard (the combination PC / Mac OS X 10.5 makes no sense).
> 
>  Can you attach screenshots showing the failure?

Yes I'm testing on Leopard. I think 'startX' has changed in Font::drawComplexText, lemme trace a bit further.

[Nikolas Zimmermann]

Reduction:

<?xml version="1.0" encoding="UTF-8"?>
<svg xmlns="http://www.w3.org/2000/svg">
    <text x="10" y="60" font-size="50" font-family="Andalus">لماذا لا يتكلمون اللّغة العربية فحسب؟</text>
</svg>

Font::drawComplexText() used to produce a 'startX' value of 10.

    <rect x="10" y="14" width="571" height="59" fill-opacity="0.3" fill="navy"/>


88	    float startX = point.x() + getGlyphsAndAdvancesForComplexText(run, from, to, glyphBuffer);
Value returned is $1 = 10

point.x() is 10.

94	    // Draw the glyph buffer now at the starting point returned in startX.
95	    FloatPoint startPoint(startX, point.y());
96	    drawGlyphBuffer(context, glyphBuffer, startPoint);
(gdb) p startPoint
$5 = {
  m_x = 267.5, 
  m_y = 60
}

It used to report just 10 here, IIRC.
The correct bounds of this text are: x=10 y=14 w=571 h=59.
startX is 267.5 now, this is the regression.

Possibly fallout from the ATSUI side of http://trac.webkit.org/changeset/76674

[Nikolas Zimmermann]

Hm, just noticed that the Chromium started skipping this test in r76678.

Please stop setting the platform to PC and the priority to P2.

[Nikolas Zimmermann]

(In reply to comment #7)
> Please stop setting the platform to PC and the priority to P2.

I'm not modifying the platform and priority? PC is the default for me, when reporting bugs - as I've just noticed. (see https://bugs.webkit.org/enter_bug.cgi?product=WebKit using a Leopard machine)

https://bugs.webkit.org/show_activity.cgi?id=53480 shows you changing it back to PC twice after I’ve changed it to Macintosh (and also resetting priority and removing keywords). Perhaps you were submitting a stale form, in which case Bugzilla should have warned you. If it didn’t, then it’s a Bugzilla bug.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1

Total errors found: 0 in 0 files


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

<rdar://problem/8946023>

This doesn’t look like it was caused by r76674.

[Yair Yogev]

is this related to the regression described here:
http://code.google.com/p/chromium/issues/detail?id=71131

?

i was just about to post about it in BugZilla, i think that one regressed since http://trac.webkit.org/changeset/68976/

Created attachment 81544 [details]
Use an offset relative to m_characters for m_indexEnd

[Oliver Hunt]

Comment on attachment 81544 [details]
Use an offset relative to m_characters for m_indexEnd

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

r=me, but remove the non-ascii character from the changelog

> Source/WebCore/ChangeLog:11
> +        offset into m_characters, not into the runâs characters.

interesting non-ascii character?

Fixed in r77858.
<http://trac.webkit.org/changeset/77858>