458010 - textPath should use nsReferencedElement instead of nsSVGUtils::GetReferencedFrame

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

Then it can track changes to the ID-element-map, and it can also work with external document URLs (when bz's external-documents work lands). And we can get rid of nsSVGUtils::GetReferencedFrame, which is something that no-one should use.

Although we might want to use nsSVGRenderingObserver instead of a plain nsReferencedElement, I'm not sure.

Comment 1

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

We should do this for 1.9.1 since it's really required for incremental XML loading.

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Needs a follow-up to blow away nsSVGUtils::GetReferenced frame (or alternatively bug 399363 could be morphed into doing that).

Comment 3

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

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

Wow that was easy!

Would be nice to have a test.

Comment 4

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

Hmm, why did you remove the review request? Did the patch not work?

Comment 5

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

Hmm, doesn't a change to the path require an update of the covered region?

Comment 6

[Robert Longson [:longsonr]]

Attachment: updated patch

Almost the same thought at exactly the same time.

In fact its worse than the covered region getting out of date. All the text of which the text path is part of needs to be recalculated in case the path has got longer/shorter and can therefore accommodate more/fewer glyphs. Text after the text path may also need to move about.

I've also tidied things up by removing the remains of the old style href code since its in the same function.

Comment 7

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

Comment on attachment 341510 [details] [diff] [review]
updated patch

+  if (mFrame->IsFrameOfType(nsIFrame::eSVG)) {

I think you should just assert this. There's no way for a non-SVG frame act as a text-path.

Comment 8

[Robert Longson [:longsonr]]

Attachment: update to tip and address review comment

Comment 9

[Robert Longson [:longsonr]]

checked in fc63a8402196

Comment 10

[Robert Longson [:longsonr]]

Attachment: reftest

Comment 11

[Robert Longson [:longsonr]]

reftest checked in 86089606be35