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

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: 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 [:longsonr]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

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 [:longsonr]]

Comment on attachment 500673 [details] [diff] [review]
patch

I think roc is on holiday.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 [:longsonr]]

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 [:longsonr]]

I'll change it back to the 3.x mechanism and store the compression in the glyph frame.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 [:longsonr]]

Attachment: address review comments

This moves us closer to what we have already.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is there a way to get rid of the code duplication in nsSVGTSpanFrame::SetWhitespaceCompression and nsSVGTextFrame::SetWhitespaceHandling?

Comment 10

[Robert Longson [:longsonr]]

+  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) (email my personal email if necessary)]

Why couldn't the tspan look up the tree instead of receiving a value in SetWhitespaceCompression?

Comment 12

[Robert Longson [:longsonr]]

Attachment: address review comments

This version scans to the root always as you suggested.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    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 [:longsonr]]

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) (email my personal email if necessary)]

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 [:longsonr]]

Attachment: simplify code

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

SetWhitespaceHandling still sets both 'leading' and 'trailing' in this patch.

Comment 18

[Robert Longson [:longsonr]]

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 [:longsonr]]

Attachment: address more review comments

Comment 20

[Robert Longson [:longsonr]]

Did you want to make any more review comments roc?

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 [:longsonr]]

Attachment: hg changeset patch unbitrotted

Comment 23

[Robert Longson [:longsonr]]

passed try modulo the usual random orange suspects.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/projects/cedar/rev/8e3ce1fe0a1c

Comment 25

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/rev/8e3ce1fe0a1c

Comment 26

[bomfog]

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]]

Thanks -- that crash is a known issue - bug 655025.  It's being investigated.

Comment 28

[George Carstoiu]

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.