Bug 18039 – dx causes non-BMP characters to fail to render

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

[Eric Seidel (no email)]

dx causes surrogate pairs to fail to render

See test case.

[Eric Seidel (no email)]

Created attachment 20000 [details]
test case

[Alexey Proskuryakov]

It's an internal implementation detail that we break this character into surrogate pairs in memory, so I've adjusted the title a little. Not because of me being mean, but because this did cause some confusion recently.

[Tim Horton]

<rdar://problem/10422142>

[Tim Horton]

Created attachment 115901 [details]
patch

I'm not sure this is *quite* right (ligatures of non-BMP characters won't work, I think), but it's a start.

This fixes both my test case and Eric's, without breaking any tests.

[Tim Horton]

Created attachment 115904 [details]
patch

I thought that patch seemed awfully small...

[WebKit Review Bot]

Attachment 115904 [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/SVGTextLayoutAttributesBuilder.cpp:220:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 5 files


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

[Simon Fraser (smfr)]

Comment on attachment 115904 [details]
patch

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

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:210
> +                if (currentCharacter >= 0xD800 && currentCharacter <= 0xDBFF) {

I'd love to see an isSurrogatePair() function we can use, rather than magic hex values.

[Tim Horton]

(In reply to comment #7)
> (From update of attachment 115904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115904&action=review
> 
> > Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:210
> > +                if (currentCharacter >= 0xD800 && currentCharacter <= 0xDBFF) {
> 
> I'd love to see an isSurrogatePair() function we can use, rather than magic hex values.

Dan just showed me SurrogatePairAwareTextIterator, so I'll make another patch to use that (or, failing that, the U16_IS_SURROGATE/U16_IS_SURROGATE_LEAD macros that already exist (which Font.h doesn't use?)) in a bit.

[Tim Horton]

Created attachment 115919 [details]
patch, fixing style and no more constants

[Tim Horton]

Switching to SurrogatePairAwareTextIterator caused more issues than I expected, so here's a copy using the macros, for now.

[WebKit Review Bot]

Comment on attachment 115919 [details]
patch, fixing style and no more constants

Attachment 115919 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10495333

New failing tests:
svg/text/non-bmp-positioning-lists.svg

[Darin Adler]

Comment on attachment 115919 [details]
patch, fixing style and no more constants

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

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:210
> +                if (U16_IS_SURROGATE(currentCharacter) && U16_IS_SURROGATE_LEAD(currentCharacter)) {

Should be just:

    if (U16_IS_LEAD(currentCharacter))

[Darin Adler]

Comment on attachment 115919 [details]
patch, fixing style and no more constants

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

> Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp:211
> +                    // Handle surrogate pairs.

I think this assumes a correctly-formed surrogate pair. It’s not so great to do that.

[Tim Horton]

Created attachment 117001 [details]
patch, fixing both of Darin's notes

[WebKit Review Bot]

Comment on attachment 117001 [details]
patch, fixing both of Darin's notes

Attachment 117001 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10709099

New failing tests:
svg/text/non-bmp-positioning-lists.svg
svg/transforms/text-with-pattern-with-svg-transform.svg

[Tim Horton]

(In reply to comment #15)
> (From update of attachment 117001 [details])
> Attachment 117001 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10709099
> 
> New failing tests:
> svg/text/non-bmp-positioning-lists.svg
> svg/transforms/text-with-pattern-with-svg-transform.svg

I cannot reproduce a failure on that second test, plus it doesn't have any positioning lists. Pretty sure it's a lie, so I'm going to land this and keep an eye on it.

[Tim Horton]

Landed in r101537.

[Eric Seidel (no email)]

Looks to be failing on lion:
http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r101543%20(3091)/svg/text/non-bmp-positioning-lists-pretty-diff.html

[Tim Horton]

(In reply to comment #18)
> Looks to be failing on lion:
> http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r101543%20(3091)/svg/text/non-bmp-positioning-lists-pretty-diff.html

I'll update my Lion machine and commit new baselines if needed momentarily.