330059 - Text Inside Child <a> Elements Does Not Render

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Doug Schepers]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

Text inside an anchor element which is a child of a text element does not render. This also applies to anchors nested inside <tspan> elements.

For example:
<text x='100' y='100'>This renders, <a xlink='mylink.html'>but this does not</a>.</text>

Reproducible: Always

Steps to Reproduce:
1. Try to put text inside an anchor element, as above.
2. Display file.
3. Sigh.

Actual Results:  
Text inside anchor does not render.

Expected Results:  
Text inside anchor should render as a link.

Comment 1

[Martijn Wargers (dead)]

Works for me with current trunk build. Please test again with current trunk build.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: Doug's testcase

Confirming. Martijn, are you sure? Do you see the word "link" in the third and fourth lines of text?

Comment 3

[Martijn Wargers (dead)]

(In reply to comment #2)
> Confirming. Martijn, are you sure? Do you see the word "link" in the third and
> fourth lines of text?
No, apparently I was confused when I made that remark. 

Comment 4

[Robert Longson [:longsonr]]

Attachment: patch

Comment 5

[Jonathan Watt [:jwatt]]

Thanks Robert. It would be nice to have a little summary covering why this isn't currently working and why we need a special frame since you've already figured that out. :-)

Comment 6

[Robert Longson [:longsonr]]

(In reply to comment #5)
> Thanks Robert. It would be nice to have a little summary covering why this
> isn't currently working and why we need a special frame since you've already
> figured that out. :-)
> 

You need a frame for several reasons.

a) For transforms (although you could just reuse a g frame for that as Tor did for  the switch element.

b) To get a nsSVGGlyphFrame to be created for the text (this needs a parent frame that supports nsISVGTextContentMetrics, although you could change the creation logic in nsCSSFrameConstructor instead. This is the main reason the testcase doesn't work.

c) To make GetElementById("xx").getTextLength() and other text DOM functions work on <text id="xx"><a>hh</a></text>. They need to recurse into frames that support nsISVGGlyphFragmentNode.

Comment 7

[Robert Longson [:longsonr]]

(In reply to comment #5)
> Thanks Robert. It would be nice to have a little summary covering why this
> isn't currently working and why we need a special frame since you've already
> figured that out. :-)
> 

In case you were wondering nsSVGAFrame is basically most of a nsSVGTspanFrame with transform support from nsSVGTextFrame and without the x, y, dx, dy attribute handling stuff.

Comment 8

[Jonathan Watt [:jwatt]]

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

Great summary, thanks!

> NS_NewSVGForeignObjectFrame(nsIPresShell* aPresShell, nsIContent* aContent, nsStyleContext* aContext);
> #endif
> nsIFrame*
>+NS_NewSVGAFrame(nsIPresShell* aPresShell, nsIContent* aContent, nsStyleContext* aContext);
>+nsIFrame*

...

> #ifdef MOZ_SVG_FOREIGNOBJECT
>   else if (aTag == nsGkAtoms::foreignObject) {
>     newFrame = NS_NewSVGForeignObjectFrame(mPresShell, aContent, aStyleContext);
>   }
> #endif
>+  else if (aTag == nsGkAtoms::a) {
>+    newFrame = NS_NewSVGAFrame(mPresShell, aContent, aStyleContext);
>+  }

Maybe alphabetical order? *shrug*


>+#include "nsSVGTextContainerFrame.h"
>+#include "nsISVGGlyphFragmentNode.h"
>+#include "nsSVGGraphicElement.h"
>+#include "nsSVGMatrix.h"
>+#include "nsIDOMSVGAElement.h"

Can you add a comment here noting that <a> isn't necessarily used in a text context, but we need it to inherit from nsSVGTextContainerFrame so that when it is things (feel free to elaborate on "things") don't work.


>+typedef nsSVGTextContainerFrame nsSVGAFrameBase;
>+
>+class nsSVGAFrame : public nsSVGAFrameBase,
>+                    public nsISVGGlyphFragmentNode

...

>+//----------------------------------------------------------------------
>+// nsISVGGlyphFragmentNode methods:

I'm wondering why some of the methods after this point even exist since they just defer to the base class. For the methods that do some work, I'm wondering if it's possible to share the code with tspan somehow (and any other relevant classes) since it looks like more duplicate code that could diverge. Didn't look very closely though. :-)

Other than that, it looks great.

Comment 9

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #8)
> 
> > NS_NewSVGForeignObjectFrame(nsIPresShell* aPresShell, nsIContent* aContent, nsStyleContext* aContext);
> > #endif
> > nsIFrame*
> >+NS_NewSVGAFrame(nsIPresShell* aPresShell, nsIContent* aContent, nsStyleContext* aContext);
> >+nsIFrame*
> 
> ...
> 
> > #ifdef MOZ_SVG_FOREIGNOBJECT
> >   else if (aTag == nsGkAtoms::foreignObject) {
> >     newFrame = NS_NewSVGForeignObjectFrame(mPresShell, aContent, aStyleContext);
> >   }
> > #endif
> >+  else if (aTag == nsGkAtoms::a) {
> >+    newFrame = NS_NewSVGAFrame(mPresShell, aContent, aStyleContext);
> >+  }
> 
> Maybe alphabetical order? *shrug*

Didn't do this. None of it is alpha currently. Could create a follow-up bug although the if clause processing makes that tricky. The prototypes are a good candidate though.

> 
> 
> >+#include "nsSVGTextContainerFrame.h"
> >+#include "nsISVGGlyphFragmentNode.h"
> >+#include "nsSVGGraphicElement.h"
> >+#include "nsSVGMatrix.h"
> >+#include "nsIDOMSVGAElement.h"
> 
> Can you add a comment here noting that <a> isn't necessarily used in a text
> context, but we need it to inherit from nsSVGTextContainerFrame so that when it
> is things (feel free to elaborate on "things") don't work.
> 

Done.

> 
> >+typedef nsSVGTextContainerFrame nsSVGAFrameBase;
> >+
> >+class nsSVGAFrame : public nsSVGAFrameBase,
> >+                    public nsISVGGlyphFragmentNode
> 
> ...
> 
> >+//----------------------------------------------------------------------
> >+// nsISVGGlyphFragmentNode methods:
> 
> I'm wondering why some of the methods after this point even exist since they
> just defer to the base class. For the methods that do some work, I'm wondering
> if it's possible to share the code with tspan somehow (and any other relevant
> classes) since it looks like more duplicate code that could diverge. Didn't
> look very closely though. :-)

Changed to derive from nsSVGTSpanFrame to get rid of the duplication.

> 
> Other than that, it looks great.
> 

Also added missing AttributeChanged method to track transform changes.

Comment 10

[Jonathan Watt [:jwatt]]

Hmm. I don't really like the abuse of existing classes. Especially when they contain functionality that isn't applicable here. It should not be possible to QI nsSVGAFrame to nsSVGTSpanFrame. I'd much rather we have an nsSVGTextContainerBase (or whatever) and use it as the base for both these classes.

Comment 11

[Robert Longson [:longsonr]]

(In reply to comment #10)
> Hmm. I don't really like the abuse of existing classes. Especially when they
> contain functionality that isn't applicable here. It should not be possible to
> QI nsSVGAFrame to nsSVGTSpanFrame. I'd much rather we have an
> nsSVGTextContainerBase (or whatever) and use it as the base for both these
> classes.
> 

Tor didn't like that last time I tried it. Bug 341638 comment 6. I suppose I could implement his alternative suggestion of a sSVGGlyphContainerFrame which would be used directly for tspans and then derive from it for textPath and a. All I would be doing really is renaming nsSVGTspanFrame to nsSVGGlyphContainerFrame, you'd still have be able to QI from a to tspan with that solution. I gave up on that idea as it seemed like a lot of effort for no real reward.

Note that nsSVGTextPathFrame already inherits from nsSVGTSpanFrame.

Or I could go back to the previous implementation, but then I must implement the nsISVGGlyphFragmentNode interface.

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 254796 [details] [diff] [review]
address review comments

Yuck. I'm going to come back to this, but right now I don't have time, r=me.

Comment 13

[tor]

Implementing it this way, it seems like the following markup will render a circle:  <text>foo<a><circle r="50"/></a></text>.  Looking at the spec, that appears to be valid, though not addressed specifically.

Comment 14

[Robert Longson [:longsonr]]

(In reply to comment #13)
> Implementing it this way, it seems like the following markup will render a
> circle:  <text>foo<a><circle r="50"/></a></text>.  Looking at the spec, that
> appears to be valid, though not addressed specifically.
> 

The DTD seems to allow you to use <a> to break out of "text world" and even back into it e.g. replace <circle> by <text> in your example, yet directly nested text elements would not be allowed. I would have liked to have had something beyond the DTD to go on really.

Comment 15

[tor]

Asked www-svg about this:

  http://lists.w3.org/Archives/Public/www-svg/2007Feb/0063.html

Comment 16

[tor]

Comment on attachment 254796 [details] [diff] [review]
address review comments

The WG seems to think the behavior with this patch is per spec, though they're trying to change that for 1.2.  sr=tor, but I would like a follow up bug regarding reusing the transform code, possibly by lofting into nsSVGDisplayContainer.

Comment 17

[Robert Longson [:longsonr]]

checked in.

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: reftest

Comment 20

[Jonathan Watt [:jwatt]]

Checked in reftest:
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/text-in-link-01.svg