Last Comment Bug 620286 - White space at beginning (end) of tspan should render when there is preceding (following) non-white space text
: White space at beginning (end) of tspan should render when there is preceding...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Robert Longson
:
Mentors:
http://dev.w3.org/SVG/profiles/1.1F2/...
Depends on: 655025 729996
Blocks: svg11tests 503075
  Show dependency treegraph
 
Reported: 2010-12-19 13:48 PST by Cameron McCormack (:heycam)
Modified: 2012-02-23 12:28 PST (History)
8 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reduced test (125 bytes, image/svg+xml)
2010-12-19 13:48 PST, Cameron McCormack (:heycam)
no flags Details
patch (22.88 KB, patch)
2011-01-02 07:30 PST, Robert Longson
no flags Details | Diff | Review
address review comments (20.16 KB, patch)
2011-01-11 14:28 PST, Robert Longson
no flags Details | Diff | Review
address review comments (20.21 KB, patch)
2011-01-14 15:11 PST, Robert Longson
no flags Details | Diff | Review
simplify code (20.19 KB, patch)
2011-03-26 03:40 PDT, Robert Longson
no flags Details | Diff | Review
address more review comments (20.14 KB, patch)
2011-04-03 05:13 PDT, Robert Longson
roc: review+
Details | Diff | Review
hg changeset patch unbitrotted (15.49 KB, patch)
2011-05-03 00:45 PDT, Robert Longson
no flags Details | Diff | Review

Description Cameron McCormack (:heycam) 2010-12-19 13:48:17 PST
Created attachment 498643 [details]
Reduced test

For cases like <text>A<tspan> B </tspan>C</text>, both spaces should be rendered.

This is kind of the opposite of bug 617737.
Comment 1 Robert Longson 2011-01-02 07:30:37 PST
Created attachment 500673 [details] [diff] [review]
patch
Comment 2 Robert Longson 2011-01-03 03:38:27 PST
The test for whether we've a text container child in SetWhitespaceCompressionModeOfChildren is wrong. Instead of checking for a nsISVGGlyphFragmentNode (which includes nsSVGGlyphFrame) we want to skip nsSVGGlyphFrame frames so the check should be

    if (kid->IsFrameOfType(nsIFrame::eSVGContainer)) {

I can either fix this on check in or post an updated patch.
Comment 3 Robert Longson 2011-01-06 10:17:40 PST
Comment on attachment 500673 [details] [diff] [review]
patch

I think roc is on holiday.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-09 18:28:49 PST
Instead of storing state in NS_STATE_SVG_COMPRESS_WHITESPACE, wouldn't it be simpler to have a function that figures out whether to compress whitespace for a given frame or not, by scanning up the frame tree?
Comment 5 Robert Longson 2011-01-10 00:25:47 PST
You'd have to potentially scan up the tree to the svg root node for each fragment, so it would be slower but yes it would be simpler. 

We could cache the flag if we figured out how to do that for text in clip paths. I was going to leave that as somthing to think about for 4.1.
Comment 6 Robert Longson 2011-01-10 01:35:28 PST
I'll change it back to the 3.x mechanism and store the compression in the glyph frame.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-10 02:19:16 PST
Our medium-term plan is to replace most of the SVG text frame code, to create an anonymous nsBlockFrame containing nsTextFrames and nsInlineFrames for the text and <tspan> content, and reuse that code for text layout. That can take care of whitespace compression, although we'll have to map xml:space into CSS 'white-space' rules or something like that. (Yes, there are a lot of other issues that will have to be worked out.)
Comment 8 Robert Longson 2011-01-11 14:28:58 PST
Created attachment 502937 [details] [diff] [review]
address review comments

This moves us closer to what we have already.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-11 14:42:41 PST
Is there a way to get rid of the code duplication in nsSVGTSpanFrame::SetWhitespaceCompression and nsSVGTextFrame::SetWhitespaceHandling?
Comment 10 Robert Longson 2011-01-11 15:12:50 PST
+  nsISVGGlyphFragmentNode* node = GetFirstGlyphFragmentChildNode();
+
+  while (node) {
+    node->SetWhitespaceCompression(compressWhitespace);
+    node = GetNextGlyphFragmentChildNode(node);
+  }

could move to nsSVGTextContainerFrame as a method.

There's not that much other duplication as the TextFrame is scanning up the frame ancestry and the tspan frame only looks at itself.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-11 15:24:29 PST
Why couldn't the tspan look up the tree instead of receiving a value in SetWhitespaceCompression?
Comment 12 Robert Longson 2011-01-14 15:11:07 PST
Created attachment 504000 [details] [diff] [review]
address review comments

This version scans to the root always as you suggested.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-24 18:47:29 PDT
+    if (!fragment->IsAllWhitespace()) {
+      lastNonWhitespaceFragment = fragment;
+      trimLeadingWhitespaceOfLastFragment = trimLeadingWhitespace;
+    }

I'm not sure why we want to trim the leading whitespace of the last fragment here. Shouldn't we be trimming its *trailing* whitespace?

Bariables named trimLeading/TrailingWhitespace are a bit misleading because we actually don't trim if the node has compression disabled. How about hasLeading/TrailingWhitespace?
Comment 14 Robert Longson 2011-03-26 02:17:19 PDT
Consider these two examples...

<text>A <tspan> B </tspan> </text>
<text>C<tspan> D </tspan> </text>

These would be separate scans but i'll describe both together which I hope won't be too confusing. 

We don't start scanning trimming trailing whitespace. So we leave A and C alone but when we get to B we know to trim its leading whitespace because A ended with whitespace similarly we know not to trim the leading whitespace of D. We remember B and D as the last non-whitespace nodes which would change if we saw further non-whitespace nodes.

The final node is whitespace in both cases and doesn't get trim leading or trailing set but as it's entirely whitespace, compressWhitespace turns it into nothing anyway. We now realise we need to go back and fix up B and D so that their trailing whitespace is trimmed. We need to remeber what the trim leading whitespace of B and D was as we set both trim values at the same time and as the examples demonstrate, these can be different.

I will remove the trimTrailingWhitespace variable from that code and just pass PR_FALSE in the loop as that's what it's set to always and that will be clearer.

hasLeading and hasTrailingWhitespace would be confusing as it's not about whether the node in question has leading/trailing whitespace but whether its previous/subsequent siblings do.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-26 02:31:57 PDT
Ah. I think the code would be clearer if SetWhitespaceHandling was split into two methods, one which sets "leading" and the other "trailing".
Comment 16 Robert Longson 2011-03-26 03:40:51 PDT
Created attachment 522083 [details] [diff] [review]
simplify code
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-26 04:15:30 PDT
SetWhitespaceHandling still sets both 'leading' and 'trailing' in this patch.
Comment 18 Robert Longson 2011-03-26 04:33:12 PDT
I was redoing is as you did your review comment ;-) I'm just recompiling and retesting something along the lines you're suggesting
Comment 19 Robert Longson 2011-04-03 05:13:23 PDT
Created attachment 523859 [details] [diff] [review]
address more review comments
Comment 20 Robert Longson 2011-05-02 06:03:36 PDT
Did you want to make any more review comments roc?
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-02 16:46:01 PDT
Comment on attachment 523859 [details] [diff] [review]
address more review comments

Review of attachment 523859 [details] [diff] [review]:

Nope, this looks good. Sorry about forgetting about it.
Comment 22 Robert Longson 2011-05-03 00:45:45 PDT
Created attachment 529648 [details] [diff] [review]
hg changeset patch unbitrotted
Comment 23 Robert Longson 2011-05-03 02:22:05 PDT
passed try modulo the usual random orange suspects.
Comment 24 Boris Zbarsky [:bz] 2011-05-03 12:27:08 PDT
Pushed http://hg.mozilla.org/projects/cedar/rev/8e3ce1fe0a1c
Comment 25 Boris Zbarsky [:bz] 2011-05-04 13:54:35 PDT
http://hg.mozilla.org/mozilla-central/rev/8e3ce1fe0a1c
Comment 26 bomfog 2011-05-05 15:30:59 PDT
I crash consistently at http://www.yougov.com/ with the hourly containing this m-c merge. The immediately preceding hourly build doesn't crash. Happens with brand-new profile and/or safe mode. There's an SVG chart on the page.

This bug, or should I file a new one?

Mozilla/5.0 (Windows NT 6.0; rv:6.0a1) Gecko/20110505 Firefox/6.0a1 ID:20110505030608

Some crash reports:
bp-90ce9e07-85a1-4ceb-8f28-4a0c72110505
bp-398e3076-e20f-4eb6-a560-9f2bd2110505
bp-4c71df08-cb5c-4a47-865a-a09b12110505
bp-89f376f9-539d-4024-9181-d2cf42110505
bp-418e0b03-804a-4ec4-bed5-0523a2110505
bp-a3ac174e-bbd5-44d3-83e2-275132110505

0 	xul.dll 	nsTextFragment::CharAt 	obj-firefox/dist/include/nsTextFragment.h:206
1 	xul.dll 	nsSVGGlyphFrame::EndsWithWhitespace 	layout/svg/base/src/nsSVGGlyphFrame.cpp:1447
2 	xul.dll 	nsSVGTextFrame::SetWhitespaceHandling 	layout/svg/base/src/nsSVGTextFrame.cpp:312
3 	xul.dll 	nsSVGTextFrame::UpdateGlyphPositioning 	layout/svg/base/src/nsSVGTextFrame.cpp:339
4 	xul.dll 	nsSVGTextFrame::InitialUpdate 	layout/svg/base/src/nsSVGTextFrame.cpp:254
5 	xul.dll 	nsSVGDisplayContainerFrame::InsertFrames 	layout/svg/base/src/nsSVGContainerFrame.cpp:136
6 	xul.dll 	nsCSSFrameConstructor::AppendFrames 	layout/base/nsCSSFrameConstructor.cpp:5764
7 	xul.dll 	nsCSSFrameConstructor::ContentAppended 	layout/base/nsCSSFrameConstructor.cpp:6702
8 	xul.dll 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsCSSFrameConstructor.cpp:6311
9 	xul.dll 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsCSSFrameConstructor.cpp:6321
10 	xul.dll 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsCSSFrameConstructor.cpp:6321
11 	xul.dll 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsCSSFrameConstructor.cpp:6321
12 	xul.dll 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsCSSFrameConstructor.cpp:6321
13 	xul.dll 	nsCSSFrameConstructor::CreateNeededFrames 	layout/base/nsCSSFrameConstructor.cpp:6321
14 	xul.dll 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4792
15 	xul.dll 	ExternalResourceTraverser 	content/base/src/nsDocument.cpp:790
16 	xul.dll 	nsDocument::FlushPendingNotifications 	content/base/src/nsDocument.cpp:6394
17 	xul.dll 	nsGenericElement::GetPrimaryFrame 	content/base/src/nsGenericElement.cpp:3771
18 	xul.dll 	nsSVGGraphicElement::GetBBox 	content/svg/content/src/nsSVGGraphicElement.cpp:95
19 	xul.dll 	XPCWrappedNative::InitTearOff 	js/src/xpconnect/src/xpcwrappednative.cpp:2150
20 	xul.dll 	xul.dll@0xbe63f 	
21 		@0x6
Comment 27 Daniel Holbert [:dholbert] 2011-05-05 15:53:44 PDT
Thanks -- that crash is a known issue - bug 655025.  It's being investigated.
Comment 28 George Carstoiu 2011-07-27 06:35:00 PDT
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on Mac OS X 10.6 using the attached testcase. Both spaces are rendered - everything seems in order.

Setting status to Verified Fixed.

Note You need to log in before you can comment on or make changes to this bug.