Closed Bug 234789 Opened 20 years ago Closed 20 years ago

SVG text ignores font-size if no unit specified

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: Justus-bulk, Assigned: scootermorris)

Details

Attachments

(1 file, 3 obsolete files)

Final patch -- has dbaron's sr comments included
6.23 KB, patch
Details | Diff | Splinter Review 
User-Agent:       
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040207

The font-size attribute of the <text> element has no effect. Verify e.g. using
the following SVG file, and playing with the respective attribute value:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
                     "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg xmlns="http://www.w3.org/2000/svg"
  width="300px" height="300px" viewBox="0 0 100 100">
  <text x="10" y="50" font-size="20" fill="black">Text</text>
</svg>


Reproducible: Always
Steps to Reproduce:



Expected Results:  
Compare with e.g. squiggle (Apache batik) or the Adobe SVG plugin.

This is an SVG-enabled Mozilla build that I downloaded on Feb 16 from mozilla.org.
It is not that font-size doesn't work at all in SVG, but...

SVG allows font-sizes to be given without units (in which case they should
default to px). Since we reuse the normal CSS font-size style property and CSS
doesn't appears to require a unit specifier, SVG font-size declarations
currently require unit specifiers as well.
This is quite a major interoperability issue, since a lot of SVG authoring tools
don't put units into font-size declarations.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: SVG text ignores font-size → SVG text ignores font-size if no unit specified
Correction: In SVG, a font-size without units is in user coordinates and not in
pixels. It is thus a (the?) typical case and not an omission if no unit is given.
(In reply to comment #2)
> Correction: In SVG, a font-size without units is in user coordinates and not in
> pixels. 

OK, true. But: 
"One px unit is defined to be equal to one user unit. Thus, a length of '5px' is
the same as a length of '5'" :-)

> It is thus a (the?) typical case and not an omission if no unit is given.

Exactly. For SVG that is true. I just don't know how to best make that
compatible with our style system for which omitted units are apparently an error
(probably as per the CSS specs).
Any help welcome.
dbaron: Any idea how to solve this? I have no idea where to even begin. This is
going to cause all kinds of wacky problems in cases with no SVG content (and we
can't key our behaviour off whether SVG is there or not since you don't want the
entire cascade to change if you introduce SVG dynamically!).
QA Contact: ian
Attached patch Initial shot at a fix (obsolete) — — Splinter Review 
Here is a patch which I think fixes the font-size problem.  I added an instance
variable to the parser object that mimics the QuirksMode stuff.  This SVGMode
can then be used to influence the behaviour of the parser.  Does this make
sense?
That won't work in a mixed namespace document with a PI linking to a stylesheet 
that says:

       .foo { font-size: 4em; }
       .foo { font-size: 4; }

...and then uses the class in both HTML parts and SVG parts. We'd need to parse
every sheet in SVG mode, just to ensure that if anyone starts poking SVG
elements into the DOM, everything still works. But we can't parse the second
rule above (and thus drop the earlier one during the cascade) because if we did
we would be catching millions of pages who expect unitless numbers to be pixels,
contrary to the CSS spec.

Chris: How is this supposed to work?
I am worried that a full explanation of graphical coordinate systems would be
seen as spamming this bug. So I will try to be brief; for details see
http://www.w3.org/TR/SVG11/coords.html

SVG, like all 2D graphics ststems from GKS onwards, uses the concept of several
coordinate systems. A given element may have its own, local, coordinate system.
Then there is, for the root-most svg  element a world coordinate system. You
specify a local coordinate system by a transform from that of your parent. You
do the mapping from a given local coordinate system to the world coordinate
system by doing matrix multiplication of all the local transforms between you
and the root-most; the result is called the current transformation matrix or CTM.

The third coordinate system is the viewport coordinate system. The mapping
between the world coordinate system and the viewport coordinate system is
defined in SVG by the viewBox. It gives a rectangular region of world coordinate
space that is to be mapped to the viewport. Additional attributes give control
over what happens when the aspect ratio of the viewport and the world coordinate
space region differ (symmetric or assymetric scaling, clip to fill the whole
viewport or shrink to fit entirely in the viewport with some uncovered.

The viewport, in CSS terms, is defined by the width and height of the containing
block in the usual way. 

The default case for SVG is that you specify a viewBox, to give the region of
world coordinate space that you intend to draw, and you leave off the width and
height attributes which default to 100%, percentage there being relative to
containing block (windowm, in the standalone case). So ther SVG is scalable ;-)

Its also possible to force the SVG to not be scalable but to be a fixed size, eg
by setting width="2in" which makes the region of world coordinate space map to 
2 inches wide; this is then fitted into the size of the containing block and
will likely clip, or not cover all of it.

It is, further, possible to omit the viewBox. In that case, the SVG renderer has
to figure out what range of coordinates is actually used. SVG says that in this
case, one user unit is one CSS px. So in that case, if I draw a circle with
radius "50" with no width and height and no viewBox, I am asking for a circle
with radius 50px.  

This (essentially corner case) sometimes leads people to assume that unitless
SVG lengths are pixels, or CSS px units. They are *not*, except in certain
special cases. The quote by Alex references that special case. It is not the
general case.

If a font-size with px, mm, etc is inherited into some SVG, the result is well
defined but does involve the various mappings of viewport to world coordinates
and world to local coordinates as described in the spec.

If a unitless font-size inherits from SVG into a child element that uses the CSS
box model (eg, a foreignObject element) then a mapping is also clear and well
defined, because the boundary between the SVG layout system and the CSS system
is a new viewport defined by the (required) x, y, width and height of the
foreignObject. See 
http://www.w3.org/TR/SVG11/coords.html

Yes, I need to speak to Håkon and ensure that the values and units module of
CSS3 refers to this when describing unitless lengths.
chris: The problem is not with how to interpret it in SVG. The problem is how
should the SVG changes to the CSS2 syntax interact with the non-SVG case in a
mixed-namespace processor.
I covered the cases where the value is correctly used within the profile of CSS,
and inheritance across namespace boundaries that use different profiles of CSS.
That seemed to cover the concerns of the original poster.

I agree that for correct parsing and to not throw away useful values as 'unknown
properties', a multinamespace UA has to understand the union of the CSS profiles
that it implements. So in the example that started this bug, font-size="20" is
correct and should not be thrown away as an invalid value.

This does bring up the case of people misusing the allowed values and applying
them to the wrong content. But then we are into error handling. The most useful
handling, in the case of an undeclared world to viewport mapping,  would be to
treat 1 user unit as 1px. However, care should be taken to inherit "20" not a
fixed up "20px" so that the correct thing happens once the value inherits into a
context with a defined world to viewport mapping.

And it should be clear that this is error handling and recovery.
Regarding the rule:

a {
  font-size: 20px;
  font-size: 15;
}

I can say three things:

 * The CSSOM requires that this rule store a single value for the 'font-size'
property
 * The CSS specification requires that an element to which this rule applies
have a font size of 20px, unless a declaration higher in the cascade overrides it.
 * The SVG specification requires that an element to which this rule applies
have a font size of 15 user units, unless a declaration higher in the cascade
overrides it.

The question being asked in comment 6, I think, is how the CSS parser should
handle this.  Note that the same rule may apply to both SVG and non-SVG elements.
Attachment #144724 - Flags: review?
Attachment #144724 - Flags: review? → review?(tor)
Attachment #144724 - Flags: review?(tor) → review?(bugmail)
Comment on attachment 144724 [details] [diff] [review]
Initial shot at a fix

removing reviewrequest until we know how we're supposed to handle this.
Attachment #144724 - Flags: review?(bugmail)
OK, I've just reread the SVG and CSS specs (at least the relevant portions) and
my conclusion is that the specs are fundamentally in conflict (duh).  If I use
CSS styling of the form:

a {
    font-size: 10;
}

it is a CSS error.  The CSS 2.1 spec clearly states that font sizes are lengths
and that lengths *must* have units.  On the other hand, the SVG spec allows me
to include an attribute of the form: font-size=10 and the 10 should be
interpreted in "user units".  The patch as submitted allows the attribute
specification (because it will be in SVG context) but continues to make the CSS
rule an error.  I believe that this is the best we can do until the CSS and SVG
communities clarify this (and other) areas of divergence (like color).

This seems sort of minor on the face of it, but unfortunately the W3C SVG test
suite uses the "unitless" font-size approach, which means that lots of people
who are looking for examples will follow suit.

Can I suggest we move forward on this patch (or some variant thereof) to
implement the "common case" for SVG?  I understand that it may be confusing for
page developers that are trying to style SVG content, but I can't see an
alternative without violating one spec or the other.
Note that this patch just makes attributes "work", which is not, strictly
speaking, incompatible with the CSS spec (well, unless there's a line-height
attribute, of course.. if there is, we're in deep trouble here, since it would
seem that the behavior of SVG is undefined in that case).

Making other style work would in fact be incompatible.  :(
Note how SVG rocks so much that parsing 'font' is exciting fun -- what do you
think these mean?:

   font: 900          serif;
   font: 900   medium serif;
   font: 900px medium serif;

As mentioned several years ago:
   http://lists.w3.org/Archives/Public/www-svg/1999Apr/0028.html
OK, the good news is there is no line-height property.

The further good news is that for non-shorthand properties we could maybe even
do this for general style (store two values for everything that's a size -- the
last "real" value and the last "SVG" value and do something wacky during
computation; this has some issues depending on how the unitless lengths are
inherited, of course).

The bad news is that shorthands have issues, as Ian pointed out, and line-height
in a stylesheet is just flat out ambiguous....

I find it interesting that the issue Ian raised was quite so summarily ignored
by the working group.
Don't bother trying to address this yet for CSS parsing. Just do it for
attribute parsing, where it is unambiguously SVG semantics, and where you need
to do it anyway for SVG Tiny documents. For CSS parsing, wait for the CSS and
SVG working groups to resolve this. Whatever you do don't try to do wacky-ass
special casing nonsense that cascades multiple values for the same property and
tries to decide which one to compute to based on where you are... *shudder*. :-)
> Just do it for attribute parsing

In that case, it can be done entirely within the attr-mapping code, really (if
it looks like a raw number, tack on "px" or whatever before passing to the CSS
parser).

Note, though, that doing that is in fact incorrect per comment 7, if we have any
way whatsoever of specifying a viewBox.
(In reply to comment #17)

> 
> Note, though, that doing that is in fact incorrect per comment 7, if we have any
> way whatsoever of specifying a viewBox.

I'm not sure what Chris means in comment #9. It should be perfectly ok to fix up
'20' as '20px'. The specs state categorically
(http://www.croczilla.com/~alex/reference/SVG/REC-SVG11-20030114/coords.html#Units)
that "One px unit is defined to be equal to one user unit. Thus, a length of
"5px" is the same as a length of "5"."
Assignee: alex → scootermorris
Could we move forward with this?  There has been a lot of discussion after the
initial patch posting, but nothing since then.  This patch is pretty narrow,
isn't part of the default build, and really doesn't impact much outside of the
svg element tree.  For SVG builds, however, it has a significant impact in
usability.
What we want as a fix is for attributes without units to be parsed as if they
had "px" units (although without affecting their values in the DOM) and for
stylesheets to drop all unitless lengths. Write a patch to do that and it should
be possible to take it, as I understand it.
OK, so if I understand this correctly, the situation is:

1. Unitless SVG values are just a shorthand for 'px'. [1]
2. SVG's unitless CSS values lead to parsing ambiguities against the published
grammar in 2 CSS properties: 'font' and 'line-height'.
3. CSS itself has an ambiguous grammar for 'font', see [2].
4. line-height isn't used by SVG.

So it looks like from both a technical and a user point of view the situation is
not so bad as long as we disambiguate the parsing of font and line-height.

Short of allowing unitless values for CSS generally, the options of fixing this are:

a) Allow unitless values for the relevant svg presentation attributes but
nowhere else.
b) Allow unitless values for presentation attributes and inline style.
c) Allow unitless values in presentation attributes, inline style & <svg:style>.
d) Allow u. l. values in all of the above plus external style sheets , IFF the
document that the stylesheet is being included into is an svg document (i.e. of
svg mime type).

I am in favour of c), since presentation attributes, inline style & <svg:style>
 all fall under the SVG specs, so we can do with them what we like without
impacting on CSS conformance elsewhere.

Implementing d) -while perfectly feasible- could be potentially confusing if
people included an external stylesheet in a pure svg document as well as a
mixed-namespace document, since the stylesheet parsing would need to be
different in the two cases. 

Scooter, your patch currently implements a), right?

[1]
http://www.croczilla.com/~alex/reference/SVG/REC-SVG11-20030114/coords.html#Units

[2] http://lists.w3.org/Archives/Public/www-style/2004May/0343.html

We want to do (a). CSS shouldn't change meaning or be parsed differently based
on where it comes from.
How do b) and c) change the meaning of CSS? Is there any spec that says that any
attribute named 'style' needs to implement CSS syntax? Or that any element named
<ns:style> needs to accept CSS syntax? 
I think b) and c) are entirely consistent with CSS, as long as we don't claim
that svg 'style' attribs and svg style elements take CSS syntax. I don't see how
this situation is any different from presentation attributes. If presentation
attributes implement a 'custom mapping' into style, why can't these other
elements , too?

Both HTML and XHTML have an attribute 'style' and an element STYLE.
(In reply to comment #24)
> Both HTML and XHTML have an attribute 'style' and an element STYLE.

Yeah, but there is nothing to say that attribute 'style' on an svg element needs
to behave as on an (X)HTML element. It doesn't on a 'generic' xml element.
Similarly, <svg:style> is quite different to <(x)html:style>. It even has a
different DOM interface. So why mandate that its textual child content needs to
be 'clean' css?
(In reply to comment #23)
> How do b) and c) change the meaning of CSS? Is there any spec that says that
> any attribute named 'style' needs to implement CSS syntax?

No, but there is a spec that says that SVG's style attribute does. Namely, the 
SVG spec itself.
And in any case, we definitely don't want to get into a position whereby a block 
that is marked up as:

   <style type="text/css"> ... </style>

...can't be moved to an external stylesheet and still work.
(In reply to comment #26)
> (In reply to comment #23)
> > How do b) and c) change the meaning of CSS? Is there any spec that says that
> > any attribute named 'style' needs to implement CSS syntax?
> 
> No, but there is a spec that says that SVG's style attribute does. Namely, the 
> SVG spec itself.

The SVG specs also say that this is "with the exception that SVG allows <length>
 and <angle>  values without a unit identifier". When the SVG specs talk about
CSS they mean CSS plus this little enhancement.
OK, so this might not be very nice and the WG did mess up (as did the CSS WG on
the 'font' syntax), but given that in SVG-land specifying lengths without unit
is commonplace, IMO the _pragmatic_ thing is to implement this for as many cases
as possible.
(In reply to comment #27)
> And in any case, we definitely don't want to get into a position whereby a block 
> that is marked up as:
> 
>    <style type="text/css"> ... </style>
> 
> ...can't be moved to an external stylesheet and still work.

Fair enough. Maybe we should implement (a)-(d) after all, i.e. parse all style
for svg documents in 'quirks'-mode. Multi-namespace documents are not common
anyway (we just use them a lot in Mozilla SVG currently to work around
unimplemented features such as <svg:script>). The interoperabilty gained by
(a)-(d) is probably preferable to the confusion caused by the different parsing
modes for svg and mixed docs.
> ...the WG did mess up (as did the CSS WG on the 'font' syntax)

This is already resolved internally ("/" is not a legal character at the start 
of a family-name). And was resolved within about 10 days of being raised, unlike 
the whole length thing, which I raised 5 years ago...
I'm personally pretty much opposed to doing anything but (a) myself. The problem
with (c) and (d) is that they would allow applying "svg css" to non-svg elements
where it makes very little sense (there are no "user units" for non-svg
elements).  I don't see such application as desirable at all.
(In reply to comment #29)
I'd like to emphasize that SVG style handling is tightly integrated with CSS,
including presentation attributes. From http://www.w3.org/TR/SVG11/styling.html:

"For user agents that support CSS, the presentation attributes must be
translated to corresponding CSS style rules... The presentation attributes thus
will participate in the CSS2 cascade as if they were replaced by corresponding
CSS style rules..."

Even (d) is not the final answer, as embedded SVG is affected by
externally-included CSS style sheets. A solution conforming to the current
standard would thus need to do the cascading differently for (X)HTML and SVG
contexts, as was pointed out by Ian in Comment #16 ("shudder").

I think the W3C made a mistake in allowing this deviation from CSS. They
probably did not notice that this leads to context-dependent cascading. It seems
that CSS was designed with only (X)HTML in mind, and SVG grafted itself on top
of it. Ideally, CSS would be a general style language that behaves consistently
for any number of XML languages (XHTML, SVG, MathML...) that could all be mixed
and matched within the same document.

To be practical, I suggest the following:

1. Implement (d). It's not a perfect solution, but covers probably a large
proportion of the actually occurring cases without requiring a major code
re-design (if I understand the code correctly, from the little time I've spent
looking at it).

2. Get the W3C to revise the standard. SVG 1.2 is "is rapidly approaching Last
Call". Anybody out there with some authority who would contact the SVG with the
aim of either changing the spec, or obtaining a clear and convincing explanation
why the spec is not at fault? One solution may be to allow unit-less font sizes
in CSS as well, likewise as a synonym of px.
Unless someone wants to challenge the current list of owners/peers for CSS,
we're doing (a).  End of story.
(In reply to comment #31)
> I'm personally pretty much opposed to doing anything but (a) myself. The problem
> with (c) and (d) is that they would allow applying "svg css" to non-svg elements
> where it makes very little sense (there are no "user units" for non-svg
> elements).  I don't see such application as desirable at all.

I don't understand why are you not opposed to (a) as well then. After all the
presentation attributes map into style as well and might apply to non-svg
elements (only in <foreignObject> elements of course). So there needs to be some
translation from presentation attributes into style. If you accept that, I don't
understand what's so different about allowing (b).
(In reply to comment #33)
> Unless someone wants to challenge the current list of owners/peers for CSS,
> we're doing (a).  End of story.

Thank you for that constructive and enlightening comment.
Here's another thought:

Option (e):
Since implementing (a) won't even get us compatible with the SVG conformance
suite, we might as well break with the SVG standard in a more fundamental way
and fix SVG's interaction with CSS by orthogonalizing SVG CSS properties and
other CSS properties. So 'font' becomes 'svg-font', 'stroke' becomes
'svg-stroke', etc.


The point is that SVG's attributes contain values that it defines as "CSS
values", but we don't really need to treat them as true CSS.  SVG can define its
attributes to have whatever syntax it wants, and they can be transformed into
the equivalent CSS, just like HTML's presentation attributes.  The differences
between (a) and the others relate to parsing of complete CSS declarations or
rules.  I have no problem making a value parser that's not really CSS, or even
using the CSS parser to do it, but I object to changing our parsing of CSS
declarations or rules.

The W3C didn't design SVG with compound documents in mind.  The rendering model
was described in a way incompatible with the CSS model, and they changed the
syntax of CSS.  That doesn't prevent us from implementing it in a way that makes
sense for compound documents -- by combining the rendering models as makes sense
and by following the standards that the SVG group should have followed in the
first place.  After all, if we're not focusing on compound documents, what's the
point of implementing SVG in Mozilla rather than just using Adobe's plugin?

Furthermore, pixel units are bad for the Web, and allowing unitless numbers to
be treated as pixels just encourages use of pixels, so we're not going to do
that any more than we already do.
(In reply to comment #37)
 
> Furthermore, pixel units are bad for the Web, and allowing unitless numbers to
> be treated as pixels just encourages use of pixels, so we're not going to do
> that any more than we already do.

Note that px units in the case of SVG are not necessarily device pixels, but are
relative to whatever coordinate system is effective at the point of use, so they
are really just 'user units'. There was a point in the history of SVG where
absolute units like px, cm, inches behaved differently to 'user units', but for
some reason the WG decided against that in the end. If this difference between
px and user units ever gets resurrected (or another non-scaled unit is
introduced) we're again faced with the problem of how to map this into CSS, even
if we only go for option (a). Option (e) then looks like the only scalable and
stable option in the face of evolving requirements, especially given the
importance we attach to multi-namespace docs.

If we're going to do (e), then why not start from scratch (not unreasonable, IMO)?

But the more important points in comment 37 were the first 2 paragraphs.
And also, given that (a) has no effect on conformance to SVG Tiny (which seems
to be heavily promoted these days), is it really that bad?  CSS doesn't seem all
that important to SVG -- it's a styling language for semantic markup, which SVG
isn't.
(In reply to comment #39)
> If we're going to do (e), then why not start from scratch (not unreasonable, IMO)?

Yeah, starting from scratch has some appeal to it, but is very dramatic compared
to (e). (e) could be done relatively quickly (it is mainly renaming existing
SVG-only properties & cloning a small number of shared properties). Starting
from scratch will take much longer and you loose out on published SVG documentation.
The impact of (e) to the end user is likely to be not much greater than (a). In
(e) we can at least achieve consistent syntax across pres. attribs and style. 
The only "advantage" of (a) over (e) is that there some sharing of seemingly
related properties such as "font-size". On closer examination, however, this
sharing doesn't look very helpful, not least because of the different meanings
of the units in HTML and SVG. 

> But the more important points in comment 37 were the first 2 paragraphs.

I don't really share your pov of the first paragraph, but I think it doesn't
matter. I wholeheartedly agree with your second paragraph.

(In reply to comment #40)
> And also, given that (a) has no effect on conformance to SVG Tiny (which seems
> to be heavily promoted these days), is it really that bad?  CSS doesn't seem all
> that important to SVG -- it's a styling language for semantic markup, which SVG
> isn't.

Ah, option (f):
Don't implement CSS styling for SVG. Probably controversial but it would solve
several implementation problems (e.g. related to <use> elements), and you're
right: CSS seems of limited value in an SVG setting. 

I don't see any reason to prefer (f) over (a).
(In reply to comment #43)
> I don't see any reason to prefer (f) over (a).

Either (e) or (f) would remove a lot of friction between CSS & SVG. I can
imagine that the font-size issue isn't the last problem we are going to see. It
is quite possible that SVG will want to extend some CSS properties in future.
E.g. Imagine that the SVG standard resurrects 'true' user units which scale
differently to 'px' et. al.. I think this would be a very sensible move - we
still have the code to handle this commented out in CVS [1]. If we do
(a),(b),(c) or (d), how are we going to map user units satisfactorily into CSS?
In either (e) or (f) this is not going to be a problem.

I think there are two sensible points of view here. 
(1) You could argue that interoperability with existing content, existing
SVG-exporting programs and other existing SVG clients is the primary concern. In
that case anything other than (d) is not really helping.

(2) You could argue that syntactic and semantic consistency of CSS, especially
in the face of muli-ns docs is the overriding concern. In that case (e) or (f)
look much more sensible than (a).

What do you think of (e)?

[1]
http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGLength.cpp#455

(In reply to comment #21)
> OK, so if I understand this correctly, the situation is:
> 
> 1. Unitless SVG values are just a shorthand for 'px'. [1]
> 2. SVG's unitless CSS values lead to parsing ambiguities against the published
> grammar in 2 CSS properties: 'font' and 'line-height'.
> 3. CSS itself has an ambiguous grammar for 'font', see [2].
> 4. line-height isn't used by SVG.
> 
> So it looks like from both a technical and a user point of view the situation is
> not so bad as long as we disambiguate the parsing of font and line-height.
> 
> Short of allowing unitless values for CSS generally, the options of fixing
this are:
> 
> a) Allow unitless values for the relevant svg presentation attributes but
> nowhere else.
> b) Allow unitless values for presentation attributes and inline style.
> c) Allow unitless values in presentation attributes, inline style & <svg:style>.
> d) Allow u. l. values in all of the above plus external style sheets , IFF the
> document that the stylesheet is being included into is an svg document (i.e. of
> svg mime type).
> 
The patch, as submitted does "a" based on a quick test.  

I think that there are a lot of interesting issues that are being raised by this
dialog, particularly with respect to SVG directions and whether or not SVG is
really meant to be implemented by browsers.  I would like to lobby that since we
already have a patch for "a" that we go forward with that for now, but that we
continue this dialog, to resolve the future issues and directions.
Comment on attachment 144724 [details] [diff] [review]
Initial shot at a fix

Alex, can you give this a review so that we can try for a superreview from
dbaron?
Attachment #144724 - Flags: review?(alex)
Attached patch Refreshed to current tree (obsolete) — — Splinter Review 

        Attachment #144724 -
        Attachment is obsolete: true

    
  

        Attachment #144724 -
        Flags: review?(alex)

    
  

        Attachment #154782 -
        Flags: review?(tor)

    
    

    
  
Comment on attachment 154782 [details] [diff] [review]
Refreshed to current tree

r=tor

        Attachment #154782 -
        Flags: review?(tor) → review+

    
  

        Attachment #154782 -
        Flags: superreview?(dbaron)

    
    

    
  
Comment on attachment 154782 [details] [diff] [review]
Refreshed to current tree

Two things need to be fixed:
 1) You need to initialize mSVGMode to PR_FALSE in CSSParserImpl's constructor
 2) In nsSVGElement.cpp, you should call SetSVGMode outside of the loop rather
than inside it.

        Attachment #154782 -
        Flags: superreview?(dbaron) → superreview-

    
    

    
  
Comment on attachment 154782 [details] [diff] [review]
Refreshed to current tree

 3) fix this comment to use correct CSS syntax and to wrap at less than 80
characters:

>+    // SVG and CSS differ slightly in their interpretation of some of
>+    // the attributes.  For example, SVG has a very different color parsing
>+    // model, and SVG allows "font-size=5" where CSS requires units: "font-size=5 pts"
>+    // .  Set a flag to pass information to the parser

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Incorporates dbarons comments (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #154782 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 155440 [details] [diff] [review]
Incorporates dbarons comments

Thanks for the comments!  This patch should incorporate everything you pointed
out...

        Attachment #155440 -
        Flags: superreview?(dbaron)

    
    

    
  
Comment on attachment 155440 [details] [diff] [review]
Incorporates dbarons comments

> CSSParserImpl::CSSParserImpl()
>   : mToken(),
>     mScanner(nsnull),
>     mChildLoader(nsnull),
>     mSection(eCSSSection_Charset),
>     mHavePushBack(PR_FALSE),
>     mNavQuirkMode(PR_FALSE),
>     mCaseSensitive(PR_FALSE),
>+#ifdef MOZ_SVG
>+    mSVGMode(PR_FALSE),
>+#endif
>     mParsingCompoundProperty(PR_FALSE)

Put the new lines between mNavQuirkMode and mCaseSensitive to match the class
definition; otherwise some compilers will warn.

>+  // SVG and CSS differ slightly in their interpretation of some of
>+  // the attributes.  SVG allows attributes of the form: "font-size=5" 

When I said correct XML syntax, I meant quotes around the attribute value.

With that, sr=dbaron.

        Attachment #155440 -
        Flags: superreview?(dbaron) → superreview+

    
    

    
  


  
Done -- ready for check-in.  Alex?  Tor?

    
  

        Attachment #155440 -
        Attachment is obsolete: true

    
    

    
  
Checked in.

Checking in html/style/public/nsICSSParser.h;
/cvsroot/mozilla/content/html/style/public/nsICSSParser.h,v  <--  nsICSSParser.h
new revision: 3.37; previous revision: 3.36
done
Checking in html/style/src/nsCSSParser.cpp;
/cvsroot/mozilla/content/html/style/src/nsCSSParser.cpp,v  <--  nsCSSParser.cpp
new revision: 3.270; previous revision: 3.269
done
Checking in svg/content/src/nsSVGElement.cpp;
/cvsroot/mozilla/content/svg/content/src/nsSVGElement.cpp,v  <--  nsSVGElement.cpp
new revision: 1.53; previous revision: 1.52
done
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED

    
    

    
  
This problem is back in Firefox 28.

    
    

    
  
(In reply to Magnus Norddahl from comment #57)
> This problem is back in Firefox 28.

I can't reproduce this in Firefox 28.  Can you file a new bug along with a test case that demonstrates the problem, and CC me?  Thanks.
Flags: needinfo?(magnus.norddahl)

    
    

    
  
Sorry, seems I should have done some more careful testing before reopening this ticket.

My problem is related but only surfaces itself when specifying lengths from a style sheet (i.e. font-size: 40) where the property in question is shared between HTML and SVG. I will open a new ticket with example code demonstrating it.
Flags: needinfo?(magnus.norddahl)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: