[webkit-reviews] review granted: [Bug 53980] REGRESSION (r68976): Incorrect bidi rendering in SVG text : [Attachment 85588] Patch v4

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 53980: REGRESSION (r68976): Incorrect bidi rendering in SVG text
https://bugs.webkit.org/show_bug.cgi?id=53980

Attachment 85588: Patch v4
https://bugs.webkit.org/attachment.cgi?id=85588&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
(In reply to comment #8)
> > Doesn't "With using these informations" sound better?
> information is plural already, no?
Yes, you are allowed to leave the s out ;-)

> > Should we take care about SVG Tiny 1.2 at all?
> Yes, I think we should. The SVG Tiny 1.2 behaviour matches HTML(5), where all
blocks (eg. <div>) respect direction="rtl" w/o the need to set unicode-bidi to
embed or bidi-override.
> The main reason for this is the W3C-I18N testsuite, which targets SVG Tiny
1.2. I could of course add unicode-bidi="embed" in all the test files, that say
direction="rtl", to get 1.1 compatible behaviour, but I think the Tiny
interpretation is better. I sent a mail to www-svg, w/o answer yet, about that
topic. For now, I think we should support the two interpretations, to ease to
switch to one of them, once SVG WG is convinced.
Ok.

> > 
> > > Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:112
> > > +        for (InlineTextBox* textBox = ltr ? text->firstTextBox() :
text->lastTextBox(); textBox; textBox = ltr ? textBox->nextTextBox() :
textBox->prevTextBox()) {
> > 
> > Don't know, but this looks horrible. Can't you use another kind of loop and
check the stuff in the loop?
> I could rewrite with a do/while loop, if you think that helps?
As long as the code is more readable, yes!

> 
> > 
> > > LayoutTests/ChangeLog:283
> > > +        * svg/text/bidi-text-anchor-direction.svg: Added.
> > 
> > Can you change the test, so that all texts start at the same position?
> Hm, I could but I wanted to show explicitely how text-anchor interpretation
differs, when direction is LTR or RTL (start -> end, end -> start is flipped).
IIRC the later tests in your patch take this as example. The test as is, is
maybe confusing on debuging it later, once nobody knows of your patch. 

> 
> Did you notice that svg/W3C-SVG-1.1/text-intro-02 and
svg/W3C-SVG-1.1-SE/text-intro-02 look differently now? I thought you'd shout
about that one! :-)
> 
> SVG 1.1 First Edition:
>     <g font-size="18"  font-family="'Arial Unicode MS',
'LucidaSansUnicode','MS-Gothic'">
> 84		 <text x="10" y="180" unicode-bidi="bidi-override"
direction="rtl">Text "×× ×™ יכול לאכול זכוכית וזה לא
מזיק לי" is in Hebrew</text>
> 
> SVG 1.2 Second Edition:
>  63	 <g font-size="18" font-family="'Arial Unicode MS',
'LucidaSansUnicode','MS-Gothic'">
>  64	   <text x="10" y="180" unicode-bidi="bidi-override" direction="rtl"
text-anchor="end">Text "×× ×™ יכול לאכול זכוכית וזה לא
מזיק לי" is in Hebrew</text>
>  65	 </g>
> 
> text-anchor is start by default, but if you switch to direction="rtl", you
have to use text-anchor="end" as the meaning is now flipped. SVG 1.1
First/Second edition don't say a word about that but obviously the Second
Edition reference image, demands this interpretation of the text-anchor
attribute. That breaks the SVG 1.1 First Edition testcase.... I'll hope the SVG
WG resolves this soon.
Ok, Great!

Please	try to fix the for loop and change the test. Minor changes so r=me. The
patch itself is great. Good to have the new behavior. Thanks a lot. Lets see
what the SVG WG thinks about the difference between SVG 1.1SE and SVG 1.2 Tiny.