Bug 53480 - REGRESSION (r76674): SVG + BiDi text broken (text-intro-05-t.svg)
Summary: REGRESSION (r76674): SVG + BiDi text broken (text-intro-05-t.svg)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords: InRadar, LayoutTestFailure, PlatformOnly, Regression
Depends on:
Blocks: 53980
  Show dependency treegraph
 
Reported: 2011-02-01 00:33 PST by Nikolas Zimmermann
Modified: 2011-02-08 02:54 PST (History)
6 users (show)

See Also:


Attachments
Screenshot (31.03 KB, text/plain)
2011-02-01 00:52 PST, Nikolas Zimmermann
no flags Details
Use an offset relative to m_characters for m_indexEnd (2.17 KB, patch)
2011-02-07 15:51 PST, mitz
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 2011-02-01 00:33:57 PST
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?
Comment 1 mitz 2011-02-01 00:40:16 PST
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?
Comment 2 Nikolas Zimmermann 2011-02-01 00:52:48 PST
Created attachment 80726 [details]
Screenshot
Comment 3 Nikolas Zimmermann 2011-02-01 00:53:26 PST
(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.
Comment 4 Nikolas Zimmermann 2011-02-01 00:57:55 PST
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.
Comment 5 mitz 2011-02-01 01:01:24 PST
Possibly fallout from the ATSUI side of http://trac.webkit.org/changeset/76674
Comment 6 Nikolas Zimmermann 2011-02-01 01:08:27 PST
Hm, just noticed that the Chromium started skipping this test in r76678.
Comment 7 mitz 2011-02-01 08:44:31 PST
Please stop setting the platform to PC and the priority to P2.
Comment 8 Nikolas Zimmermann 2011-02-01 09:46:12 PST
(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)
Comment 9 mitz 2011-02-01 10:02:51 PST
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.
Comment 10 WebKit Review Bot 2011-02-01 11:37:36 PST
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.
Comment 11 mitz 2011-02-01 16:00:39 PST
<rdar://problem/8946023>
Comment 12 mitz 2011-02-01 16:59:54 PST
This doesn’t look like it was caused by r76674.
Comment 13 Yair Yogev 2011-02-02 11:34:36 PST
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/
Comment 14 mitz 2011-02-07 15:51:46 PST
Created attachment 81544 [details]
Use an offset relative to m_characters for m_indexEnd
Comment 15 Oliver Hunt 2011-02-07 15:59:47 PST
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?
Comment 16 mitz 2011-02-07 16:08:56 PST
Fixed in r77858.
<http://trac.webkit.org/changeset/77858>