Closed Bug 456286 Opened 16 years ago Closed 14 years ago

should altGlyph elements fall back to behaving like tspan?

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: longsonr)

Details

Attachments

(2 files, 5 obsolete files)

Attached image 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!
Attached patch WIP (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #438093 - Attachment is obsolete: true
Attachment #438152 - Flags: review?(roc)
Attachment #438152 - Attachment is obsolete: true
Attachment #438152 - Flags: review?(roc)
Attached patch patch (obsolete) — Splinter Review
If the frame acts like a tspan it might as well be a tspan at least for the moment.
Attachment #438242 - Flags: review?(roc)
Would it make sense to make nsSVGAltGlyphElement inherit from nsSVGTSpanElement?
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.
Should we make nsIDOMSVGTSpanElement not inherit from nsIDOMSVGTextPositioningElement and nsIDOMSVGAltGlyphElement not inherit from nsIDOMSVGTextPositioningElement?
We're trying to match the spec here: http://www.w3.org/TR/SVG/idl.html
I don't think that really matters. We don't implement the multiple-inheritance stuff in the spec.
Attached patch address review comments (obsolete) — Splinter Review
I have to admit that this is better.
Attachment #438242 - Attachment is obsolete: true
Attachment #438386 - Flags: review?(roc)
Attachment #438242 - Flags: review?(roc)
     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?
(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.
Attached patch how about this one then? (obsolete) — Splinter Review
This fixes the duplication by creating an nsSVGTextPositioningElement that everything can inherit from thereby avoiding having nsSVGAltGlyphElement implement nsIDOMSVGTSpanElement
Attachment #438386 - Attachment is obsolete: true
Attachment #438448 - Flags: review?(roc)
Attachment #438386 - Flags: review?(roc)
Attachment #438448 - Attachment is obsolete: true
Attachment #438448 - Flags: review?(roc)
Attachment #438462 - Flags: review?(roc)
pushed http://hg.mozilla.org/mozilla-central/rev/caa4eb12ad20
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: