Bug 36840 - SVG @font-face breaks text-overflow: ellipsis
Summary: SVG @font-face breaks text-overflow: ellipsis
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 59085
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-30 12:17 PDT by Michel Jansen
Modified: 2011-06-20 01:29 PDT (History)
10 users (show)

See Also:


Attachments
Screenshot + reduced testcase (49.49 KB, application/octet-stream)
2010-03-30 12:17 PDT, Michel Jansen
no flags Details
Patch, with layout test (85.03 KB, patch)
2011-01-28 10:33 PST, Emil Schutte
no flags Details | Formatted Diff | Diff
Patch (revised), with layout test (84.91 KB, patch)
2011-01-28 18:38 PST, Emil Schutte
krit: review-
Details | Formatted Diff | Diff
Patch (2nd rev) (38.37 KB, patch)
2011-01-30 12:12 PST, Emil Schutte
krit: review-
Details | Formatted Diff | Diff
Patch (3rd rev) (38.44 KB, patch)
2011-02-07 18:22 PST, Emil Schutte
no flags Details | Formatted Diff | Diff
Patch (4th rev--fixed test output) (38.42 KB, patch)
2011-02-08 10:45 PST, Emil Schutte
no flags Details | Formatted Diff | Diff
Patch (5th rev--changed test for platform independence) (37.37 KB, patch)
2011-02-19 15:31 PST, Emil Schutte
eric: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michel Jansen 2010-03-30 12:17:10 PDT
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.
Comment 1 Dirk Schulze 2010-03-31 03:46:49 PDT
Seems to crash on trunk. I take a look to the crash.
Comment 2 Dirk Schulze 2010-03-31 04:02:51 PDT
(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.
Comment 3 Nikolas Zimmermann 2010-07-09 07:22:43 PDT
Changed component to SVG, so it shows up in my all-svg-bugs search.
Comment 4 Emil Schutte 2011-01-28 10:33:35 PST
Created attachment 80464 [details]
Patch, with layout test

Note, the bulk of this patch is the included layout test
Comment 5 WebKit Review Bot 2011-01-28 10:34:47 PST
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.
Comment 6 Eric Seidel (no email) 2011-01-28 12:08:05 PST
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.
Comment 7 Eric Seidel (no email) 2011-01-28 12:09:35 PST
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.
Comment 8 Emil Schutte 2011-01-28 18:38:56 PST
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.
Comment 9 Dirk Schulze 2011-01-28 22:31:07 PST
Comment on attachment 80535 [details]
Patch (revised), with layout test

Added 'patch' flag for the bots
Comment 10 Dirk Schulze 2011-01-28 22:47:22 PST
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?
Comment 11 Emil Schutte 2011-01-30 12:12:26 PST
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.
Comment 12 Dirk Schulze 2011-02-07 11:27:30 PST
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>
Comment 13 Emil Schutte 2011-02-07 18:22:53 PST
Created attachment 81562 [details]
Patch (3rd rev)

Updated with requested changes.
Comment 14 Dirk Schulze 2011-02-08 00:59:06 PST
Comment on attachment 81562 [details]
Patch (3rd rev)

LGTM. r=me
Comment 15 WebKit Commit Bot 2011-02-08 01:25:34 PST
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
Comment 16 Emil Schutte 2011-02-08 10:45:52 PST
Created attachment 81660 [details]
Patch (4th rev--fixed test output)

Fixed test output.
Comment 17 Dirk Schulze 2011-02-08 11:42:36 PST
Comment on attachment 81660 [details]
Patch (4th rev--fixed test output)

2nd try
Comment 18 WebKit Commit Bot 2011-02-08 13:15:52 PST
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
Comment 19 Emil Schutte 2011-02-19 15:31:42 PST
Created attachment 83084 [details]
Patch (5th rev--changed test for platform independence)

Changed test select-text-svgfont to use dumpAsText() for platform independence.
Comment 20 WebKit Commit Bot 2011-02-19 23:19:29 PST
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
Comment 21 Nikolas Zimmermann 2011-04-05 05:15:08 PDT
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?
Comment 22 Emil Schutte 2011-04-05 23:05:47 PDT
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.
Comment 23 Dirk Schulze 2011-04-05 23:52:16 PDT
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.
Comment 24 Joseph Pecoraro 2011-04-06 10:23:08 PDT
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?
Comment 25 Eric Seidel (no email) 2011-04-06 10:31:11 PDT
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.
Comment 26 Eric Seidel (no email) 2011-04-06 10:45:24 PDT
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.
Comment 27 Eric Seidel (no email) 2011-04-06 10:45:28 PDT
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.
Comment 28 Emil Schutte 2011-04-06 16:16:55 PDT
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!
Comment 29 Eric Seidel (no email) 2011-04-06 16:18:26 PDT
The commit-queue does not currently upload failure results but should be taught to. :(  I just haven't gotten around to it yet.
Comment 30 Eric Seidel (no email) 2011-06-18 19:54:30 PDT
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.
Comment 31 WebKit Review Bot 2011-06-18 19:57:44 PDT
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
Comment 32 Eric Seidel (no email) 2011-06-18 19:58:54 PDT
Comment on attachment 83084 [details]
Patch (5th rev--changed test for platform independence)

This needs an updated patch.
Comment 33 Emil Schutte 2011-06-18 22:01:18 PDT
I just looked and this logic is now included in rendering/svg/SVGTextRunRenderingContext.cpp, so this patch is obsolete.
Comment 34 Nikolas Zimmermann 2011-06-19 03:18:31 PDT
(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
Comment 35 Nikolas Zimmermann 2011-06-20 01:29:29 PDT
Fixed in r89233.