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

VERIFIED FIXED in mozilla6



6 years ago
5 years ago


(Reporter: heycam, Assigned: Robert Longson)


(Blocks: 1 bug)

Mac OS X
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)




(2 attachments, 5 obsolete attachments)



6 years ago
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.


6 years ago


6 years ago
Assignee: nobody → longsonr

Comment 1

6 years ago
Created attachment 500673 [details] [diff] [review]
Attachment #500673 - Flags: review?(roc)

Comment 2

6 years ago
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.


6 years ago
Blocks: 503075

Comment 3

6 years ago
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?

Comment 5

6 years ago
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

6 years ago
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.)

Comment 8

6 years ago
Created attachment 502937 [details] [diff] [review]
address review comments

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?

Comment 10

6 years ago
+  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?

Comment 12

6 years ago
Created attachment 504000 [details] [diff] [review]
address review comments

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)


6 years ago
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?

Comment 14

6 years ago
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".

Comment 16

6 years ago
Created attachment 522083 [details] [diff] [review]
simplify code
Attachment #504000 - Attachment is obsolete: true
Attachment #522083 - Flags: review?(roc)
Attachment #504000 - Flags: review?(roc)


6 years ago
Attachment #522083 - Flags: review?(roc)
SetWhitespaceHandling still sets both 'leading' and 'trailing' in this patch.

Comment 18

6 years ago
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

6 years ago
Created attachment 523859 [details] [diff] [review]
address more review comments
Attachment #522083 - Attachment is obsolete: true
Attachment #523859 - Flags: review?(roc)

Comment 20

6 years ago
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+

Comment 22

6 years ago
Created attachment 529648 [details] [diff] [review]
hg changeset patch unbitrotted
Attachment #523859 - Attachment is obsolete: true

Comment 23

6 years ago
passed try modulo the usual random orange suspects.
Keywords: checkin-needed
Pushed http://hg.mozilla.org/projects/cedar/rev/8e3ce1fe0a1c
Keywords: checkin-needed
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla6
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Depends on: 655025

Comment 26

6 years ago
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:

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
No longer blocks: 655489

Comment 28

6 years ago
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.
Depends on: 729996
You need to log in before you can comment on or make changes to this bug.