456286 - should altGlyph elements fall back to behaving like tspan?

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase

altGlyph elements, defined in http://www.w3.org/TR/2003/REC-SVG11-20030114/text.html#AltGlyphElement , seem to be a lot like tspan elements.  In particular, the spec says:
  # If the references to alternate glyphs do not result in successful
  # identification of alternate glyphs to use, then the character(s) that
  # are inside of the 'altGlyph' element are rendered as if the 'altGlyph'
  # element were a 'tspan' element instead.

Given that we don't currently support altGlyph, I wonder if we should at least treat altGlyph like tspan so that content that uses it doesn't disappear?  (I noticed this while poking into Acid3, test 79, which has an altGlyph element, which is why we fail as early in that test as we do -- since we don't count the contents of the altGlyph towards the length.)

Steps to reproduce: load attached testcase
Actual results:   Hello, word!
Expected results: Hello, world!

Comment 1

[Robert Longson [:longsonr]]

Attachment: WIP

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Doesn't attempt to do any more than the bug title i.e. altGlyph works like tspan now.

We count the right number of characters in the acid 3 test with this patch but fail at the next step.

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

If the frame acts like a tspan it might as well be a tspan at least for the moment.

Comment 4

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

Would it make sense to make nsSVGAltGlyphElement inherit from nsSVGTSpanElement?

Comment 5

[Robert Longson [:longsonr]]

nsSVGAltGlyphElement must implement nsIDOMSVGAltGlyphElement which is derived from nsIDOMSVGTextPositioningElement

nsSVGTSpanElement must implement nsIDOMSVGTSpanElement which is also derived from nsIDOMSVGTextPositioningElement

If I derive nsSVGAltGlyphElement from nsSVGTSpanElement I'll end up with multiple inheritance of nsIDOMSVGTextPositioningElement which I don't think is desirable.

Comment 6

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

Should we make nsIDOMSVGTSpanElement not inherit from nsIDOMSVGTextPositioningElement and nsIDOMSVGAltGlyphElement not inherit from nsIDOMSVGTextPositioningElement?

Comment 7

[Robert Longson [:longsonr]]

We're trying to match the spec here: http://www.w3.org/TR/SVG/idl.html

Comment 8

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

I don't think that really matters. We don't implement the multiple-inheritance stuff in the spec.

Comment 9

[Robert Longson [:longsonr]]

Attachment: address review comments

I have to admit that this is better.

Comment 10

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

     nsSVGTextContainerFrame *metrics = do_QueryFrame(ancestorFrame);
     NS_ASSERTION(metrics,
                  "trying to construct an SVGTSpanFrame for an invalid "
                  "container");
 
     nsCOMPtr<nsIDOMSVGTSpanElement> tspan = do_QueryInterface(aContent);
-    NS_ASSERTION(tspan, "Content is not an SVG tspan");
+    nsCOMPtr<nsIDOMSVGAltGlyphElement> altGlyph = do_QueryInterface(aContent);
+    NS_ASSERTION(tspan || altGlyph, "Content is not an SVG tspan or altGlyph");

This should all be #ifdef DEBUG I guess? You don't need to fix it here if you don't want to.

This makes nsDOMSVGAltGlyphElement implement nsIDOMSVGTSpanElement. But since that's an empty interface, I'm hoping it doesn't matter to script. Does JS instanceof return true for nsIDOMSVGTSpanElement, though?

Comment 11

[Robert Longson [:longsonr]]

(In reply to comment #10)
>      nsSVGTextContainerFrame *metrics = do_QueryFrame(ancestorFrame);
>      NS_ASSERTION(metrics,
>                   "trying to construct an SVGTSpanFrame for an invalid "
>                   "container");
> 
>      nsCOMPtr<nsIDOMSVGTSpanElement> tspan = do_QueryInterface(aContent);
> -    NS_ASSERTION(tspan, "Content is not an SVG tspan");
> +    nsCOMPtr<nsIDOMSVGAltGlyphElement> altGlyph = do_QueryInterface(aContent);
> +    NS_ASSERTION(tspan || altGlyph, "Content is not an SVG tspan or
> altGlyph");
> 
> This should all be #ifdef DEBUG I guess? You don't need to fix it here if you
> don't want to.

The entire method is within #ifdef DEBUG. I guess there's just not enough context to show it.

> 
> This makes nsDOMSVGAltGlyphElement implement nsIDOMSVGTSpanElement. But since
> that's an empty interface, I'm hoping it doesn't matter to script. Does JS
> instanceof return true for nsIDOMSVGTSpanElement, though?

I'll check that out. Another alternative might be to introduce a new concrete nsSVGTextPositioningElement and have tspan and altGlyph both derive from that.

Comment 12

[Robert Longson [:longsonr]]

Attachment: how about this one then?

This fixes the duplication by creating an nsSVGTextPositioningElement that everything can inherit from thereby avoiding having nsSVGAltGlyphElement implement nsIDOMSVGTSpanElement

Comment 13

[Robert Longson [:longsonr]]

Attachment: just comment changes

Comment 14

[Robert Longson [:longsonr]]

pushed http://hg.mozilla.org/mozilla-central/rev/caa4eb12ad20