Last Comment Bug 456286 - should altGlyph elements fall back to behaving like tspan?
: should altGlyph elements fall back to behaving like tspan?
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: ---
Assigned To: Robert Longson
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-21 12:11 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2012-04-04 03:31 PDT (History)
10 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (229 bytes, image/svg+xml)
2008-09-21 12:11 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
WIP (32.74 KB, patch)
2010-04-09 08:18 PDT, Robert Longson
no flags Details | Diff | Splinter Review
patch (34.50 KB, patch)
2010-04-09 13:01 PDT, Robert Longson
no flags Details | Diff | Splinter Review
patch (26.37 KB, patch)
2010-04-10 01:25 PDT, Robert Longson
no flags Details | Diff | Splinter Review
address review comments (27.76 KB, patch)
2010-04-11 14:26 PDT, Robert Longson
no flags Details | Diff | Splinter Review
how about this one then? (46.36 KB, patch)
2010-04-12 03:17 PDT, Robert Longson
no flags Details | Diff | Splinter Review
just comment changes (46.47 KB, patch)
2010-04-12 05:45 PDT, Robert Longson
roc: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-21 12:11:05 PDT
Created attachment 339680 [details]
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 2010-04-09 08:18:30 PDT
Created attachment 438093 [details] [diff] [review]
WIP
Comment 2 Robert Longson 2010-04-09 13:01:14 PDT
Created attachment 438152 [details] [diff] [review]
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 2010-04-10 01:25:07 PDT
Created attachment 438242 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2010-04-10 23:31:20 PDT
Would it make sense to make nsSVGAltGlyphElement inherit from nsSVGTSpanElement?
Comment 5 Robert Longson 2010-04-11 00:19:48 PDT
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) (Exited; email my personal email if necessary) 2010-04-11 00:22:54 PDT
Should we make nsIDOMSVGTSpanElement not inherit from nsIDOMSVGTextPositioningElement and nsIDOMSVGAltGlyphElement not inherit from nsIDOMSVGTextPositioningElement?
Comment 7 Robert Longson 2010-04-11 00:33:35 PDT
We're trying to match the spec here: http://www.w3.org/TR/SVG/idl.html
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-04-11 01:06:46 PDT
I don't think that really matters. We don't implement the multiple-inheritance stuff in the spec.
Comment 9 Robert Longson 2010-04-11 14:26:14 PDT
Created attachment 438386 [details] [diff] [review]
address review comments


I have to admit that this is better.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-04-11 15:38:07 PDT
     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 2010-04-12 00:14:51 PDT
(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 2010-04-12 03:17:30 PDT
Created attachment 438448 [details] [diff] [review]
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 2010-04-12 05:45:46 PDT
Created attachment 438462 [details] [diff] [review]
just comment changes
Comment 14 Robert Longson 2010-04-13 02:45:25 PDT
pushed http://hg.mozilla.org/mozilla-central/rev/caa4eb12ad20

Note You need to log in before you can comment on or make changes to this bug.