Closed Bug 842142 Opened 9 years ago Closed 9 years ago

Text incorrectly wraps with svg.text.css-frames.enabled


(Core :: SVG, defect)

Windows Vista
Not set





(Reporter: longsonr, Assigned: heycam)





(3 files, 3 obsolete files)

banana should follow "You are not a" on the same line but wraps instead.

Same with "But you" on right hand side.
Blocks: svgtext
Attached image test
The wrapping seems to be OK on Mac.

I do however get wrapping where I shouldn't with this attached case if I full page zoom it in one step.
Attached image screen shot
I did the get wrapping in that test once.  It seems quite intermittent.
There is some timing dependence on this bug manifesting.  On my machine, and with my network characteristics, I can reliably reproduce with this reduced test:

The SVG is being loaded inside an <object>.  If I change this to an <iframe>, the bug is gone.  It looks like the <object> is sized initially so that the root SVG element has mViewport{Width,Height} = 5.  Then later it gets resized to 480x360.  The small viewport results in a large mFontSizeScaleFactor, which in turn is going to result in slightly different text layout from when it is at 480x360.  I think we're not reflowing the text once the <object> gets its final size and the viewport changes.
Attached patch patch (obsolete) — Splinter Review
This patch fixes the problem, but at the expense of reflowing the text whenever the transform changes.  It would be better if we could avoid reflowing if it's not a change in the scale.  Unfortunately in NotifySVGChange when (aFlags & TRANSFORM_CHANGED), we don't have access to the old transform.

Ultimately the reason why we get these word wraps is that we reflow the anonymous block frame at the exact width it reports as being its preferred width.  When the metrics change slightly without reflowing, it can end up wrapping.

The change in AttributeChanged is so that we actually invalidate if our own transform changes.  NotifySVGChange(TRANSFORM_CHANGED) won't invalidate us, since it handles the case when an ancestor's transform changed, and we would be included in their invalidation area already.
Assignee: nobody → cam
Attachment #715302 - Flags: review?(longsonr)
Attached patch patch (obsolete) — Splinter Review
This is the right patch.
Attachment #715302 - Attachment is obsolete: true
Attachment #715302 - Flags: review?(longsonr)
Attachment #715304 - Flags: review?(longsonr)
Comment on attachment 715304 [details] [diff] [review]

With this patch applied, if I view and full page zoom it, the page flashes each time I zoom, as it is briefly not rendering the text.  We'll need a different fix.
Attachment #715304 - Flags: review?(longsonr)
From the comments in bug 839865, it looks like calling InvalidateBounds() from AttributeChanged is wrong anyway.
Surely it's better to either 

a) make the outer svg element default to whitespace: nowrap, or
b) fix nsTextFrameThebes such that anywhere it calls WhiteSpaceCanWrap() or WordCanWrap() we also check for IsSVGText() and return false.
I like option (b).
Attached patch patch v2 (obsolete) — Splinter Review
This makes WhiteSpaceCanWrap and WordCanWrap sensitive to whether the frame is for SVG text, and returns false for both.  This solves the unexpected text wrapping problem, although there is a related problem where the SVG text inside the <object> is not selectable initially.  So we still probably need to reflow the text due to the <object> resizing, but I'll look at that in a separate bug.
Attachment #715304 - Attachment is obsolete: true
Attachment #728811 - Flags: review?(roc)
Duplicate of this bug: 842428
Attached patch patch v2Splinter Review
The actual patch.
Attachment #728811 - Attachment is obsolete: true
Attachment #728811 - Flags: review?(roc)
Attachment #728817 - Flags: review?(roc)
(In reply to Cameron McCormack (:heycam) from comment #10)
> So we still probably
> need to reflow the text due to the <object> resizing, but I'll look at that
> in a separate bug.

Bug 854286.
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.