Bug 53980

Created attachment 81594 [details] test case

Created attachment 81595 [details] safari screenshot

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

<rdar://problem/8970686>

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

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.

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?

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

Created attachment 85583 [details] Patch v2 Fix Dirks commentrs.

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.

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

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

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

Created attachment 85588 [details] Patch v4 Fix merge error, should now build again.

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

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

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.

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

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.

(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 :-)

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

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

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

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

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

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

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

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

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.

Created attachment 87321 [details] Fix regression - patch

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?

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

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

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.

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

Created attachment 87344 [details] Fix regression - patch v2 Adressed Erics comments.

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

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

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

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

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

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

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

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

I suspect new results are needed.

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