Closed Bug 620286 Opened 9 years ago Closed 9 years ago

White space at beginning (end) of tspan should render when there is preceding (following) non-white space text


(Core :: SVG, defect)

Not set





(Reporter: heycam, Assigned: longsonr)


(Depends on 1 open bug, )



(2 files, 5 obsolete files)

Attached image 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.
Assignee: nobody → longsonr
Attached patch patch (obsolete) — Splinter Review
Attachment #500673 - Flags: review?(roc)
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.
Blocks: 503075
Comment on attachment 500673 [details] [diff] [review]

I think roc is on holiday.
Attachment #500673 - Flags: review?(jwatt)
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?
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.
I'll change it back to the 3.x mechanism and store the compression in the glyph frame.
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.)
Attached patch address review comments (obsolete) — Splinter Review
This moves us closer to what we have already.
Attachment #500673 - Attachment is obsolete: true
Attachment #502937 - Flags: review?(roc)
Attachment #500673 - Flags: review?(roc)
Attachment #500673 - Flags: review?(jwatt)
Is there a way to get rid of the code duplication in nsSVGTSpanFrame::SetWhitespaceCompression and nsSVGTextFrame::SetWhitespaceHandling?
+  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.
Why couldn't the tspan look up the tree instead of receiving a value in SetWhitespaceCompression?
Attached patch address review comments (obsolete) — Splinter Review
This version scans to the root always as you suggested.
Attachment #502937 - Attachment is obsolete: true
Attachment #504000 - Flags: review?(roc)
Attachment #502937 - Flags: review?(roc)
Attachment #502937 - Attachment is patch: true
+    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?
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.
Ah. I think the code would be clearer if SetWhitespaceHandling was split into two methods, one which sets "leading" and the other "trailing".
Attached patch simplify code (obsolete) — Splinter Review
Attachment #504000 - Attachment is obsolete: true
Attachment #522083 - Flags: review?(roc)
Attachment #504000 - Flags: review?(roc)
Attachment #522083 - Flags: review?(roc)
SetWhitespaceHandling still sets both 'leading' and 'trailing' in this patch.
I was redoing is as you did your review comment ;-) I'm just recompiling and retesting something along the lines you're suggesting
Attached patch address more review comments (obsolete) — Splinter Review
Attachment #522083 - Attachment is obsolete: true
Attachment #523859 - Flags: review?(roc)
Did you want to make any more review comments roc?
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.
Attachment #523859 - Flags: review?(roc) → review+
Attachment #523859 - Attachment is obsolete: true
passed try modulo the usual random orange suspects.
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla6
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
I crash consistently at 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:

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
Thanks -- that crash is a known issue - bug 655025.  It's being investigated.
Blocks: 655489
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.
You need to log in before you can comment on or make changes to this bug.