Bug 36840 – SVG @font-face breaks text-overflow: ellipsis

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

[Michel Jansen]

Created attachment 52064 [details]
Screenshot + reduced testcase

First of all: this is my first bug report here, and I am not sure if I have selected the right component etc. Please let me know if additional information is needed.

The bug is quite easy to reproduce: define a custom @font-face using an SVG font and apply it to a field that also has the CSS text-overflow property set to "ellipsis". If overflow indeed happens, and the ellipsis are drawn, they seem to replace the text rather than append to it.

For desktop browsers based on webkit, a workaround is to use TTF fonts in the @font-face definition, but this issue quite severely affects the iPhone where SVG is the only way to supply web fonts.

I've attached a ZIP file containing a minimal test case and a screenshot.

[Dirk Schulze]

Seems to crash on trunk. I take a look to the crash.

[Dirk Schulze]

(In reply to comment #1)
> Seems to crash on trunk. I take a look to the crash.
Sorry, wrong Alert. My tree was not clean.

[Nikolas Zimmermann]

Changed component to SVG, so it shows up in my all-svg-bugs search.

[Emil Schutte]

Created attachment 80464 [details]
Patch, with layout test

Note, the bulk of this patch is the included layout test

[WebKit Review Bot]

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

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

Source/WebCore/svg/SVGFont.cpp:658:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 7 files


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

[Eric Seidel (no email)]

I feel like Justin filed a bug related to this the other day, relating to how text selection of SVG text worked on the iphone.  Then again, that may have been about SVG text and not SVG font-styled HTML text.

[Eric Seidel (no email)]

Comment on attachment 80464 [details]
Patch, with layout test

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

> Source/WebCore/svg/SVGFont.cpp:625
> +    if (const SVGFontData* fontData = svgFontAndFontFaceElementForFontData(primaryFont(), fontFaceElement, fontElement)) {

Early return is preferred over long if clauses.

[Emil Schutte]

Created attachment 80535 [details]
Patch (revised), with layout test

Thanks for the review.  I just got back to a computer and am attaching a revised patch.

[Dirk Schulze]

Comment on attachment 80535 [details]
Patch (revised), with layout test

Added 'patch' flag for the bots

[Dirk Schulze]

Comment on attachment 80535 [details]
Patch (revised), with layout test

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

Great that you're working on that. Just some notes. Is it possiblw to add a selection test? IIRC we have such a general selection test in svg/W3C... maybe it is possible to adapt this test to simulate the selection with such a test. Doesn't the event sender support mouse control? So you may can simulate it with the event sender.

> Source/WebCore/ChangeLog:6
> +        Implement offsetForPositionForTextUsingSVGFont for SVG fonts, enabling per-character SVG text selection
> +        and fixing text-overflow: ellipsis.

You should add a more detailed instruction what you changed, how it works.

> Source/WebCore/svg/SVGFont.cpp:579
> +    int boundedFrom = std::max(0, std::min(len, from));
> +    int boundedTo = std::max(0, std::min(len, to));

Add using std; at the  beginning please.

> Source/WebCore/svg/SVGFont.cpp:644
> +    data.at = from;
> +    data.from = from;

from is still 0 here. I'd initialize the int before the first real use.

> Source/WebCore/svg/SVGFont.cpp:648
> +    data.scale = convertEmUnitToPixel(this->size(), fontFaceElement->unitsPerEm(), 1.0f);

s/1.0f/1/

> Source/WebCore/svg/SVGFont.cpp:649
> +    data.length = 0.0f;

Just 0

> LayoutTests/fast/css/resources/CartoGothicStd-Bold-webfont.svg:3
> +<?xml version="1.0" standalone="no"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" >
> +<svg xmlns="http://www.w3.org/2000/svg">

We have multiple SVGFonts on different places, also see LayoutTests/svg/. Do we need this font? Can't we just use one of the fonts of svg?

[Emil Schutte]

Created attachment 80598 [details]
Patch (2nd rev)

Thanks for the comments.  I made the suggested code changes and added a mouse selection test.  Note that I had to add an ellipsis glyph to ABCFont.svg.

[Dirk Schulze]

Comment on attachment 80598 [details]
Patch (2nd rev)

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

Nearly done:

> Source/WebCore/ChangeLog:18
> +        Implement offsetForPositionForTextUsingSVGFont for SVG fonts, fixing
> +        rendering of SVG font-styled text with text-overflow: ellipsis and
> +        per-character selection of SVG font-styled text.  Uses existing
> +        SVGTextRunWalker mechanism to walk the text run, measuring total pixel
> +        length up to the specified limit, then returning the number of characters
> +        walked.
> +
> +        SVG @font-face breaks text-overflow: ellipsis
> +        https://bugs.webkit.org/show_bug.cgi?id=36840
> +        REGRESSION: SVG Font selection problems
> +        https://bugs.webkit.org/show_bug.cgi?id=41934
> +
> +        Tests: editing/selection/select-text-svgfont.html
> +               fast/css/text-overflow-ellipsis-svgfont.html

Please use this style:

bug name
bug link

bug name
bug link

comment

Tests:

> Source/WebCore/svg/SVGFont.cpp:609
> +    return (data.at < data.to && data.length <= data.maxLength);

No braces here

> Source/WebCore/svg/SVGFont.cpp:640
> +    int len = run.length();

name it length

> Source/WebCore/svg/SVGFont.cpp:658
> +    if (!(includePartialGlyphs && (data.maxLength > data.length - (data.lastGlyphLength / 2))))

if (!includePartialGlyphs || !(data.maxLength > data.length - (data.lastGlyphLength / 2)))

two braces less

> LayoutTests/fast/css/text-overflow-ellipsis-svgfont.html:1
> +<META HTTP-EQUIV="CONTENT-TYPE" CONTENT="TEXT/HTML; CHARSET=utf8">

<!DOCTYPE html>
<html>

[Emil Schutte]

Created attachment 81562 [details]
Patch (3rd rev)

Updated with requested changes.

[Dirk Schulze]

Comment on attachment 81562 [details]
Patch (3rd rev)

LGTM. r=me

[WebKit Commit Bot]

Comment on attachment 81562 [details]
Patch (3rd rev)

Rejecting attachment 81562 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:

editing/selection ..................................................................................................................................................................................................................................................
editing/selection/select-text-svgfont.html -> failed

Exiting early after 1 failures. 5383 tests run.
114.25s total testing time

5382 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7797012

[Emil Schutte]

Created attachment 81660 [details]
Patch (4th rev--fixed test output)

Fixed test output.

[Dirk Schulze]

Comment on attachment 81660 [details]
Patch (4th rev--fixed test output)

2nd try

[WebKit Commit Bot]

Comment on attachment 81660 [details]
Patch (4th rev--fixed test output)

Rejecting attachment 81660 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-..." exit_code: 2

Last 500 characters of output:

editing/selection ..................................................................................................................................................................................................................................................
editing/selection/select-text-svgfont.html -> failed

Exiting early after 1 failures. 5383 tests run.
114.56s total testing time

5382 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7819191

[Emil Schutte]

Created attachment 83084 [details]
Patch (5th rev--changed test for platform independence)

Changed test select-text-svgfont to use dumpAsText() for platform independence.

[WebKit Commit Bot]

Comment on attachment 83084 [details]
Patch (5th rev--changed test for platform independence)

Rejecting attachment 83084 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2

Last 500 characters of output:
websocket/tests/workers ......
http/tests/workers ......
http/tests/xhtmlmp .
http/tests/xmlhttprequest ...............................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers .........
693.24s total testing time

22715 test cases (99%) succeeded
1 test case (<1%) was new
14 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7940346

[Nikolas Zimmermann]

Comment on attachment 83084 [details]
Patch (5th rev--changed test for platform independence)

Oops, this has not been landed yet! Emil, can you look at it again?

[Emil Schutte]

Yes, looking at it now and will work on it tomorrow.

I can't make sense of the previous build output at http://queues.webkit.org/results/7940346 so if anyone can give me pointers about what's breaking on the Mac build I'd appreciate it (I'm on GTK).  Will ask on #webkit tomorrow.

[Dirk Schulze]

Fir(In reply to comment #22)
> Yes, looking at it now and will work on it tomorrow.
> 
> I can't make sense of the previous build output at http://queues.webkit.org/results/7940346 so if anyone can give me pointers about what's breaking on the Mac build I'd appreciate it (I'm on GTK).  Will ask on #webkit tomorrow.

Not sure about the std errors, but I did not see, that the results are in the wrong folder. If they are really platform independent, you should move the DRT output to LayoutTests/fast/css/ and upload a new patch.

[Joseph Pecoraro]

Clicking on the link in the "Attachment Details" section both mac bots say:

  "Error: mac-ews cannot process patches from non-committers :("

Maybe that is the issue?

[Eric Seidel (no email)]

Our Mac bots are only jailed (wrt network) and not in a VM (as well as being jailed) like the rest of our bots. So we only trust committers to run code on them.

[Eric Seidel (no email)]

Comment on attachment 81562 [details]
Patch (3rd rev)

Cleared Dirk Schulze's review+ from obsolete attachment 81562 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 81660 [details]
Patch (4th rev--fixed test output)

Cleared Dirk Schulze's review+ from obsolete attachment 81660 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Emil Schutte]

Joseph, thanks, you're right.  My question should have been, why are the tests failing in commit-queue.

Dirk, only the editing test is cross-platform, not the css test.

Are the tests with stderr output the reason the tests are failing, and if so, is there a way for me to see the output?

Thanks!

[Eric Seidel (no email)]

The commit-queue does not currently upload failure results but should be taught to. :(  I just haven't gotten around to it yet.

[Eric Seidel (no email)]

Comment on attachment 83084 [details]
Patch (5th rev--changed test for platform independence)

The commit-queue now knows how to upload failures.  I'm not sure this still applies.  However if it does, you'll learn what the failrues are.  If it doesn't we need to r- this patch.

[WebKit Review Bot]

Comment on attachment 83084 [details]
Patch (5th rev--changed test for platform independence)

Rejecting attachment 83084 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:
tTests/editing/selection/select-text-svgfont.html
patching file LayoutTests/fast/css/text-overflow-ellipsis-svgfont.html
patching file LayoutTests/platform/gtk/fast/css/text-overflow-ellipsis-svgfont-expected.checksum
patching file LayoutTests/platform/gtk/fast/css/text-overflow-ellipsis-svgfont-expected.txt
patching file LayoutTests/svg/custom/resources/ABCFont.svg

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Dirk Schulze', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/8914004

[Eric Seidel (no email)]

Comment on attachment 83084 [details]
Patch (5th rev--changed test for platform independence)

This needs an updated patch.

[Emil Schutte]

I just looked and this logic is now included in rendering/svg/SVGTextRunRenderingContext.cpp, so this patch is obsolete.

[Nikolas Zimmermann]

(In reply to comment #33)
> I just looked and this logic is now included in rendering/svg/SVGTextRunRenderingContext.cpp, so this patch is obsolete.

Hi Emil,

your testcase is included in the patch at bug 59085. It has got r+ and will land today. This will make SVG Fonts a first class citizen. It's now possible to mix WOFF/SVG/platform fonts using segmented fonts, and apply all kind of CSS properties (letter/word-spacing/ttext-overflow/...) to SVG Fonts.

Your patch is not necessary anymore, as when bug 59085 lands, we're reusing the simple code path completly for SVG Fonts, so the need for SVGTextRunWalker & friends is gone, so I'm removing them alltogether.

Cheers,
Niko

[Nikolas Zimmermann]

Fixed in r89233.