Bug 53980 – REGRESSION (r68976): Incorrect bidi rendering in SVG text

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Yair Yogev]

Only tested this under Windows.
verified with r77858 (after https://bugs.webkit.org/show_bug.cgi?id=53480 )

Details form the chromium bug tracker:

What steps will reproduce the problem?
1. Create an SVG with a <text> element with direction="rtl" and unicode-bidi="embed" or "bidi-override".
2. Add some text inside the <text> element.
3. Add a <tspan> inside the text element with direction="ltr" and unicode-bidi="embed".

What is the expected result?
The <tspan> element renders left-to-right, inline with the right-to-left text in the <text> element.

What happens instead?
The <tspan> element renders at position (0,0), regardless of the position of the <text> element.


The same problem occurs when using the unicode directional formatting codes instead of the direction/unicode-bidi attributes.
svg_bidi.svg reproduces the problem using both methods.
svg_bidi-batik.png shows the correct rendering produced by Batik.
svg_bidi-safari.png shows the rendering in Safari (also correct).


inChromium issue: http://code.google.com/p/chromium/issues/detail?id=71131

[Yair Yogev]

Created attachment 81594 [details]
test case

[Yair Yogev]

Created attachment 81595 [details]
safari screenshot

[Yair Yogev]

Created attachment 81596 [details]
batik / safari pre 68976 screenshot

<rdar://problem/8970686>

[Nikolas Zimmermann]

Created attachment 85312 [details]
Patch

I'm including 54 new tests, so the patch size is large, because of the load of pngs.
Finally SVG bidi support, properly :-)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/rendering/svg/SVGTextLayoutEngine.h:85:  The parameter name "box" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 183 files


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

[Dirk Schulze]

Comment on attachment 85312 [details]
Patch

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

The following comments don't justify a r-, but I'd like to hear your opinion before I accept the patch. Btw. thanks for not making the code changes to big, even if the patch size still seems amazing :-)

> Source/WebCore/ChangeLog:37
> +        characters, their position based on the DOM attributes x/y/dx/dy/rotate. Using that information it's possible to

Doesn't "With using these informations" sound better?

> Source/WebCore/ChangeLog:43
> +        rules. For each characer of the passed in InlineTextBox, it determines the x/y/dx/dy/rotate value, and the position

s/characer/character/

> Source/WebCore/ChangeLog:51
> +        before it starts processing the boxes in visual order, fed by SVGRootInlineBox. When layout out text, we can now

Don't understand this sentence. What is 'out text'?

> Source/WebCore/ChangeLog:114
> +        * css/CSSStyleSelector.cpp: Special case for SVG Tiny 1.2 support, which has slightly different definition of the "direction" attribute, see inline comments.

Should we take care about SVG Tiny 1.2 at all?

> Source/WebCore/ChangeLog:157
> +                                                            The width of a SVGTextFragment is always equal to the sum of each each glyph advance. (This was not the case for

s/each each/each/

> Source/WebCore/css/CSSStyleSelector.cpp:2051
> +        // SVG 1.1 First/Second edition and SVG 1.2 Tiny behave differently when handling the direction/unicode-bidi attributes.

See my comment above. Unsure if we should add a hack for SVG Tiny 1.2

> 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?

> 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?

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 85312 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85312&action=review
> 
> The following comments don't justify a r-, but I'd like to hear your opinion before I accept the patch. Btw. thanks for not making the code changes to big, even if the patch size still seems amazing :-)
I tried hard :-)

> > Source/WebCore/ChangeLog:37
> > +        characters, their position based on the DOM attributes x/y/dx/dy/rotate. Using that information it's possible to
> 
> Doesn't "With using these informations" sound better?
information is plural already, no?

> 
> > Source/WebCore/ChangeLog:43
> > +        rules. For each characer of the passed in InlineTextBox, it determines the x/y/dx/dy/rotate value, and the position
> 
> s/characer/character/
Fixed, thanks.

> 
> > Source/WebCore/ChangeLog:51
> > +        before it starts processing the boxes in visual order, fed by SVGRootInlineBox. When layout out text, we can now
> 
> Don't understand this sentence. What is 'out text'?
When laying out our text, we can..
Fixed.

> 
> > Source/WebCore/ChangeLog:114
> > +        * css/CSSStyleSelector.cpp: Special case for SVG Tiny 1.2 support, which has slightly different definition of the "direction" attribute, see inline comments.
> 
> 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.

> 
> > Source/WebCore/ChangeLog:157
> > +                                                            The width of a SVGTextFragment is always equal to the sum of each each glyph advance. (This was not the case for
> 
> s/each each/each/
Thanks, fixed.

> 
> > Source/WebCore/css/CSSStyleSelector.cpp:2051
> > +        // SVG 1.1 First/Second edition and SVG 1.2 Tiny behave differently when handling the direction/unicode-bidi attributes.
> 
> See my comment above. Unsure if we should add a hack for SVG Tiny 1.2
See above, I think it makes sense, I actually would want to switch to the 1.2 behaviour completly, if SVG WG agrees.

> 
> > 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?

> 
> > 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).

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.

[Nikolas Zimmermann]

Created attachment 85583 [details]
Patch v2

Fix Dirks commentrs.

[Nikolas Zimmermann]

Created attachment 85586 [details]
Patch v3

Found another issue: only check the SVG 1.2 version attribute, if we need to, aka. optimize code a bit in CSSStyleSelector.

[WebKit Review Bot]

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

[Early Warning System Bot]

Attachment 85586 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8148637

[Collabora GTK+ EWS bot]

Attachment 85586 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8149681

[Nikolas Zimmermann]

Created attachment 85588 [details]
Patch v4

Fix merge error, should now build again.

[WebKit Review Bot]

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

[WebKit Review Bot]

Attachment 85586 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8139694

[Dirk Schulze]

Comment on attachment 85588 [details]
Patch v4

(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.

[Nikolas Zimmermann]

Created attachment 85703 [details]
Patch v5

SVG WG resolved my problem, remove SVG Tiny 1.2 special cases. SVG 1.1 Second Edition now behaves mostly like Tiny 1.2

[Dirk Schulze]

Comment on attachment 85703 [details]
Patch v5

You forgot to change LayoutTests/svg/text/bidi-text-anchor-direction.svg Great that you could remove the hack in CSS. Looks like SVG 1.1 SE is really better than the old standard ;-) r=me, but change the test first.

[Nikolas Zimmermann]

(In reply to comment #19)
> (From update of attachment 85703 [details])
> You forgot to change LayoutTests/svg/text/bidi-text-anchor-direction.svg Great that you could remove the hack in CSS. Looks like SVG 1.1 SE is really better than the old standard ;-) r=me, but change the test first.

Fixing before landing.. Yeah 1.1 2nd Edition is really starting to be nice :-)

[Nikolas Zimmermann]

Committed r81168: <http://trac.webkit.org/changeset/81168>

[Nikolas Zimmermann]

*** Bug 6524 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 11662 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 17392 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 24374 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

A bunch of these tests need new results on Windows. I'll take care of that.

[Nikolas Zimmermann]

(In reply to comment #26)
> A bunch of these tests need new results on Windows. I'll take care of that.
Thanks Adam, I was about to fix them!

[Yair Yogev]

Nikolas, i just tested the "test case" uploaded above, against chromium with webkit r81221 under Windows.

The results are still deferent from the image i posted which was taken from batik (same as pre r68976 webkit)

Can you confirm that the results are now correct?
while every browser seems to view it differently (!) the standalone-above-the-line "world" is unique to webkit

[Nikolas Zimmermann]

Oops, bug confirmed! I knew I forgot to test this file again, I had it working at some point. The attached screenshots are both wrong, I'll describe in detail and fix the problem, once I have some time again, next week.

[Nikolas Zimmermann]

Created attachment 87321 [details]
Fix regression - patch

[Eric Seidel (no email)]

So I now understand how bidi works. :)  But I'm confused why SVG needs so much custom bidi code...  Can you pop onto #webkit and explain?

[Early Warning System Bot]

Attachment 87321 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8285318

[Nikolas Zimmermann]

(In reply to comment #31)
> So I now understand how bidi works. :)  But I'm confused why SVG needs so much custom bidi code...  Can you pop onto #webkit and explain?

For the record:
[16:43] WildFox: eseidel: a) the x/y/dx/dy/rotate value lists are in logical order, and need to be reordered to maintain correspondence (that doesn't exist in HTML)  b) bidi-reordering does not happen across text chunks (every character which has an absolute position adjustment assigned to it - aka. x attribute in horizontal text or y attribute in vertical text - defines a new text chunk)
[16:43] WildFox: eseidel: ignoring a) and b) allows us to match exactly the HTML bidi code

b) is done in RenderBlockLineLayout, grep for characterStartsNewTextChunk.
a) is done in SVGRootInlineBox, with this patch.

(In reply to comment #32)
> Attachment 87321 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/8285318

Missing SVGTextLayoutAttributes.h include, should fix Qt build.

[Eric Seidel (no email)]

Comment on attachment 87321 [details]
Fix regression - patch

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

> Source/WebCore/rendering/InlineFlowBox.cpp:1293
> +    while (leaf) {
> +        if (leaf->bidiLevel() > maxLevel)
> +            maxLevel = leaf->bidiLevel();
> +        if (leaf->bidiLevel() < minLevel)
> +            minLevel = leaf->bidiLevel();
> +        leafBoxesInLogicalOrder.append(leaf);
> +        leaf = leaf->nextLeafChild();
> +    }

This easier to write as a for loop using min/max.

> Source/WebCore/rendering/InlineFlowBox.cpp:1313
> +            while (it != end) {
> +                if ((*it)->bidiLevel() >= minLevel)
> +                    break;
> +                ++it;

See bug https://bugs.webkit.org/show_bug.cgi?id=57341.  It's a shame you're copying this code out of BidiResolver.

> Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:237
> +static inline void reverseInlineBoxRangeAndValueListsIfNeeded(void* userData, Vector<InlineBox*>::iterator first, Vector<InlineBox*>::iterator last)

I'm confused why we ever need to use a void*...

> Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:243
> +        if (first == last || first == --last)

So you want this to conditionally decrement last?  This seems very easy to mis-read.

> Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:291
> +                swapItems(xValuesFirst, xValuesLast, firstBoxPosition, lastBoxPosition);

Seems we should write a swapItems which takes a SVGTextLayoutAttributes* and then we don't need quite so much copy/paste code here.

[Nikolas Zimmermann]

(In reply to comment #34)
> (From update of attachment 87321 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87321&action=review
> 
> > Source/WebCore/rendering/InlineFlowBox.cpp:1293
> > +    while (leaf) {
> > +        if (leaf->bidiLevel() > maxLevel)
> > +            maxLevel = leaf->bidiLevel();
> > +        if (leaf->bidiLevel() < minLevel)
> > +            minLevel = leaf->bidiLevel();
> > +        leafBoxesInLogicalOrder.append(leaf);
> > +        leaf = leaf->nextLeafChild();
> > +    }
> 
> This easier to write as a for loop using min/max.
Okay.

> 
> > Source/WebCore/rendering/InlineFlowBox.cpp:1313
> > +            while (it != end) {
> > +                if ((*it)->bidiLevel() >= minLevel)
> > +                    break;
> > +                ++it;
> 
> See bug https://bugs.webkit.org/show_bug.cgi?id=57341.  It's a shame you're copying this code out of BidiResolver.
I wasn't aware of that, I only moved the code from editing/visible_units.cpp.

> 
> > Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:237
> > +static inline void reverseInlineBoxRangeAndValueListsIfNeeded(void* userData, Vector<InlineBox*>::iterator first, Vector<InlineBox*>::iterator last)
> 
> I'm confused why we ever need to use a void*...
I could use templates to suply a real type for the user data, but I want to avoid having to move the whole function directly into InlineFlowBox, hence the void.

> 
> > Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:243
> > +        if (first == last || first == --last)
> 
> So you want this to conditionally decrement last?  This seems very easy to mis-read.
Yeah, it's a direct copy from the STL implementation of std::reverse.

from c++/4.2.1/bits/stl_algo.h:

  template<typename _BidirectionalIterator>
    void
    __reverse(_BidirectionalIterator __first, _BidirectionalIterator __last,
          bidirectional_iterator_tag)
    {
      while (true)
    if (__first == __last || __first == --__last)
      return;
    else
      {
        std::iter_swap(__first, __last);
        ++__first;
      }
    }


> 
> > Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:291
> > +                swapItems(xValuesFirst, xValuesLast, firstBoxPosition, lastBoxPosition);
> 
> Seems we should write a swapItems which takes a SVGTextLayoutAttributes* and then we don't need quite so much copy/paste code here.
I'll check again.

[Nikolas Zimmermann]

Created attachment 87344 [details]
Fix regression - patch v2

Adressed Erics comments.

[Early Warning System Bot]

Attachment 87344 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8282383

[WebKit Review Bot]

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

[Nikolas Zimmermann]

Created attachment 87350 [details]
Fix regression - patch v3

Fix another missing include problem -- can't spot the problem on Mac as the mac port uses RenderSVGAllInOne....

[Eric Seidel (no email)]

Comment on attachment 87350 [details]
Fix regression - patch v3

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

Seems reasonable.  I'm not sure I fully folowed the entire patch.  You might want krit to take a second look.  Please consider my suggested cleanups.

Thsi seems like a ridiculous amount of code for SVG bidi.  I woudl expect we could share more with HTML.

> Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:278
> +            SVGTextLayoutAttributes* firstAttributes = 0;
> +            SVGTextLayoutAttributes* lastAttributes = 0;
> +            unsigned attributesSize = attributes.size();
> +            for (unsigned i = 0; i < attributesSize; ++i) {
> +                SVGTextLayoutAttributes& currentAttributes = attributes.at(i);
> +                if (!firstAttributes && firstContext == currentAttributes.context())
> +                    firstAttributes = &currentAttributes;
> +                if (!lastAttributes && lastContext == currentAttributes.context())
> +                    lastAttributes = &currentAttributes;
> +                if (firstAttributes && lastAttributes)
> +                    break;
> +            }

I would do this as a helper function which reutnred firstAttributes and lastAttributes as refernece params.  And then you could easily name the locals first and last and current and not worry about confusion. :)

> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:464
> +        while (!aborted) {
> +            if (!currentLogicalCharacterMetrics(logicalAttributes, logicalMetrics)) {
> +                if (!currentLogicalCharacterAttributes(logicalAttributes))

Just push this while into a function which returns a bool.  Then your if aborted logic which follows is easier. :)

[Nikolas Zimmermann]

(In reply to comment #40)
> (From update of attachment 87350 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87350&action=review
> 
> Seems reasonable.  I'm not sure I fully folowed the entire patch.  You might want krit to take a second look.  Please consider my suggested cleanups.
> 
> Thsi seems like a ridiculous amount of code for SVG bidi.  I woudl expect we could share more with HTML.
> 
> > Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:278
> > +            SVGTextLayoutAttributes* firstAttributes = 0;
> > +            SVGTextLayoutAttributes* lastAttributes = 0;
> > +            unsigned attributesSize = attributes.size();
> > +            for (unsigned i = 0; i < attributesSize; ++i) {
> > +                SVGTextLayoutAttributes& currentAttributes = attributes.at(i);
> > +                if (!firstAttributes && firstContext == currentAttributes.context())
> > +                    firstAttributes = &currentAttributes;
> > +                if (!lastAttributes && lastContext == currentAttributes.context())
> > +                    lastAttributes = &currentAttributes;
> > +                if (firstAttributes && lastAttributes)
> > +                    break;
> > +            }
> 
> I would do this as a helper function which reutnred firstAttributes and lastAttributes as refernece params.  And then you could easily name the locals first and last and current and not worry about confusion. :)
>
Fixed.
 
> > Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:464
> > +        while (!aborted) {
> > +            if (!currentLogicalCharacterMetrics(logicalAttributes, logicalMetrics)) {
> > +                if (!currentLogicalCharacterAttributes(logicalAttributes))
> 
> Just push this while into a function which returns a bool.  Then your if aborted logic which follows is easier. :)
Oh yeah, great suggestion, much more readable now...

Testing first.

[Dirk Schulze]

(In reply to comment #40)
> (From update of attachment 87350 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87350&action=review
> 
> Seems reasonable.  I'm not sure I fully folowed the entire patch.  You might want krit to take a second look.  Please consider my suggested cleanups.

I accept the patch as well.

[Nikolas Zimmermann]

Thanks. Committed r82411: <http://trac.webkit.org/changeset/82411>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82411 might have broken Windows 7 Release (Tests)
The following tests are not passing:
svg/text/bidi-reorder-value-lists.svg
svg/text/font-size-below-point-five.svg

[Eric Seidel (no email)]

I suspect new results are needed.

[Ryosuke Niwa]

Rebaselined svg/W3C-SVG-1.1-SE/types-dom-05-b.html on Windows in http://trac.webkit.org/changeset/86818.