Closed Bug 305859 Opened 19 years ago Closed 17 years ago

Em and ex units not implemented for SVGLength (tspan often rendered incorrectly)

Categories

(Core :: SVG, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: j.prevost, Assigned: takenspc)

References

()

Details

Attachments

(1 file, 9 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+

I encountered this problem when attempting to set up an image that would scale
with Ctrl-+ Ctrl-- scaling.  In short, even though according to the SVG 1.1 spec
(and the example above), em and ex units should be usable anywhere that
coordinates or lengths are required, these units are not actually recognized.

Instead, whenever an em or ex unit appears in an SVG attribute, the value is
interpreted as zero.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.w3.org/TR/SVG11/coords.html
2. Scroll down to the figure entitled "Example Units"
3. Compare the figure with Mozilla's rendering

Actual Results:  
The two images do not match--the pre-rendered example includes three boxes in
the center column, but the Firefox-rendered version shows only one box.

Expected Results:  
The two images should have matched.
Note that the em unit *is* recognized and used in the font-size property of a
text element.  However, in that context, the default font size (and hence the
size of the em unit) does not change as the font size of the web page changes. 
That may or may not be desirable--but percentage units combined with container
elements can work around that.

Regardless, this is the one place I can find that the em unit is recognized to
have any meaning at all.
I found the test case that covers this:

http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-coords-units-03-b.html

Notice the two missing horizontal lines in this test case--since they have
length zero, they do not appear.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: Em and ex units are not recognized in width and height attributes → Em and ex units not implemented for SVGLength
*** Bug 312165 has been marked as a duplicate of this bug. ***
Summary: Em and ex units not implemented for SVGLength → Em and ex units not implemented for SVGLength (tspan often rendered incorrectly)
Let's copy Doug's comment from bug #312165 which was marked as a duplicate of this one:
> ------- Comment #6 From Doug Schepers 2005-10-14 06:55 PST [reply] -------
>
> This breaks a lot a existing valid content, and should be fixed for the release
> of FF1.5 if possible.

Here is yet another example (all 3 columns of the rectangles there should look the same - the middle column is based on em/ex units and does not in FF1.5:

http://www.w3.org/TR/SVG/images/coords/Units.svg
This is crucial to fix because MS Visio 2003 outputs SVG text with a lot of "em" usage. So currently, the output of typical Visio business diagrams is mostly unreadable using Firefox's native SVG implementation.

Here is an example of how text becomes unreadable without "em" implementation:
On the http://wiki.svg.org/Text_Wrapping  site, the example for tspan does not display properly in Firefox 1.5:

http://www.svg-whiz.com/svg/text/textWrapViaTspan.svg

Even if this were fixed in a nightly unofficial build... that would be great! As long as it's a fairly solid usable build, most FireFox users within corporations still tend to be technically savvy... and don't mind having to install an "unofficial beta." This would allow me, within my company, to promote the use of SVG.
Given the MS Visio problem mentioned above, I now believe that this big is more severe than I originally marked it as.

SVG shows a great deal of promise, but unless the SVG support in Firefox can be interoperable with common software like Visio, it will be difficult to convince people to use it.

Especially since this is a Firefox bug, and Visio is apparently producing conformant SVG output.

In short: while it's possible to work around this problem when writing your own SVG, we now have an attested case of a major piece of software which produces conformant SVG output that Firefox renders in a massively broken way.

Note that Gwen Epstein asked me to bump up the severity on this.  Why, I'm not entirely sure, since I think she should have been able to do it herself.  But I agree with her assessment.
Severity: normal → major
My company would also use SVG in a some major projects. The correct SVG support is a very big issue while discussing the browser decision. 
Unfortunately FF is not usable with MS Visio output in the current version (1.5). Therefore I think FF and even SVG will get a boom if MS Visio output is supported without doing any kind of SVG code modification before the graphic is displayed correctly in the browser. And one big step in this direction would be the fix of this "em" bug.
The text placement in svg output from the scientific plotting package gnuplot is mangled by this bug.  I have struggled unsuccessfully to find a work-around.  A fix would be much appreciated.

               sfeam@users.sourceforge.net (gnuplot development team)
More than 12 monthes since it was reported as major and it is still marked as new and nor fixed nor even assigned... :(:( This bug really criples the usability of Firefox on svg-based websites. The number of such sites it low, but growing. Please raise the priority of this bug.
Bug is still present in Firefox 3.0 Alpha 1: Grand Paradiso (Gecko 1.9a1)
I知 taking a look for the em/ex units.
My initially work depends on Attachment 247233 [details] [diff] from bug 353172.
Supporting em on the width and height of the outer svg element would already be a big functionality and interoperability win.
Setting to blocking1.9? now that bug 177805 has landed. Being able to at least establish the size of the outer SVG viewport in em units in compound documents is important for interoperability in use cases where the graphics are indeed meant to scale.
Flags: blocking1.9?
Partial patch (nsSVGLength2 only). This patch does not solve tspan problems.
minor fixes
Attachment #255684 - Attachment is obsolete: true
Oops, In previous patch, I forgot nsSVGLength2.h.
Attachment #255748 - Attachment is obsolete: true
Comment on attachment 255749 [details] [diff] [review]
Partial patch (nsSVGLength2 only) #2.0.0.1

>Index: content/svg/content/src/nsSVGElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGElement.cpp,v
>retrieving revision 1.113
>diff -u -8 -p -r1.113 nsSVGElement.cpp
>--- content/svg/content/src/nsSVGElement.cpp	16 Feb 2007 22:59:07 -0000	1.113
>+++ content/svg/content/src/nsSVGElement.cpp	20 Feb 2007 03:40:31 -0000
>@@ -934,23 +934,30 @@ nsSVGElement::GetAnimatedLengthValues(fl
> 
> 
>   while (f && i < info.mLengthCount) {
>+    PRUint8 type;
>     if (!ctx) {
>-      PRUint8 type = info.mLengths[i].GetSpecifiedUnitType();
>+      type = info.mLengths[i].GetSpecifiedUnitType();
>       if (type != nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER &&
>-          type != nsIDOMSVGLength::SVG_LENGTHTYPE_PX)
>+          type != nsIDOMSVGLength::SVG_LENGTHTYPE_PX &&
>+          type != nsIDOMSVGLength::SVG_LENGTHTYPE_EMS &&
>+          type != nsIDOMSVGLength::SVG_LENGTHTYPE_EXS)
>         ctx = nsSVGUtils::GetCoordContextProvider(this);
>     }
>-    *f = info.mLengths[i++].GetAnimValue(ctx);
>+    if (type == nsIDOMSVGLength::SVG_LENGTHTYPE_EMS ||
>+        type == nsIDOMSVGLength::SVG_LENGTHTYPE_EXS)

type possibly undefined here. In fact this code could go first if you don't need ctx.

>+      *f = info.mLengths[i++].GetAnimValue(this);
>+    else
>+      *f = info.mLengths[i++].GetAnimValue(ctx);
>     f = va_arg(args, float*);
>   }
> 
> nsSVGElement::NumberAttributesInfo
> nsSVGElement::GetNumberInfo()
>Index: content/svg/content/src/nsSVGLength2.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGLength2.cpp,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 nsSVGLength2.cpp
>--- content/svg/content/src/nsSVGLength2.cpp	14 Apr 2006 15:09:38 -0000	1.1
>+++ content/svg/content/src/nsSVGLength2.cpp	20 Feb 2007 03:40:31 -0000
>@@ -38,16 +38,21 @@
> 
> #include "nsContentUtils.h"
> #include "nsSVGLength2.h"
> #include "nsGkAtoms.h"
> #include "prdtoa.h"
> #include "nsCRT.h"
> #include "nsTextFormatter.h"
> #include "nsSVGCoordCtx.h"
>+#include "nsStyleContext.h"
>+#include "nsPresContext.h"
>+#include "nsIFontMetrics.h"
>+#include "nsIPresShell.h"
>+#include "nsInspectorCSSUtils.h"
> 
> NS_IMPL_ADDREF(nsSVGLength2::DOMBaseVal)
> NS_IMPL_RELEASE(nsSVGLength2::DOMBaseVal)
> 
> NS_IMPL_ADDREF(nsSVGLength2::DOMAnimVal)
> NS_IMPL_RELEASE(nsSVGLength2::DOMAnimVal)
> 
> NS_IMPL_ADDREF(nsSVGLength2::DOMAnimatedLength)
>@@ -165,16 +170,57 @@ GetAxisLength(nsSVGCoordCtx *aCtx)
>   if (d == 0.0f) {
>     NS_WARNING("zero axis length");
>     d = 1e-20f;
>   }
> 
>   return d;
> }
> 
>+float
>+nsSVGLength2::GetEmLength(nsSVGElement *aSVGElement)
>+{
>+  if (!aSVGElement)
>+    return 0;
>+
>+  nsIDocument* doc = aSVGElement->GetCurrentDoc();
>+  nsIPresShell *presShell = doc->GetShellAt(0);
>+  nsPresContext *presContext = presShell->GetPresContext();
>+  nsRefPtr<nsStyleContext> styleContext =
>+    nsInspectorCSSUtils::GetStyleContextForContent(aSVGElement, nsnull,
>+                                                   presShell);
>+  NS_ASSERTION(styleContext, "no styleContext");
>+
>+  const nsStyleFont* font = styleContext->GetStyleFont();
>+  float textZoom = presContext->TextZoom();
>+  return nsPresContext::AppUnitsToFloatCSSPixels(font->mSize) / textZoom;

create a helper in nsSVGUtils for the above lines and call from nsSVGGlyphFrame::SelectFont too as it has basically the same code. Pass in presContext and font.

>+}
>+
>+float
>+nsSVGLength2::GetExLength(nsSVGElement *aSVGElement)
>+{
>+  if (!aSVGElement)
>+    return 0;
>+
>+  nsIDocument* doc = aSVGElement->GetCurrentDoc();
>+  nsIPresShell *presShell = doc->GetShellAt(0);
>+  nsPresContext *presContext = presShell->GetPresContext();
>+  nsRefPtr<nsStyleContext> styleContext =
>+    nsInspectorCSSUtils::GetStyleContextForContent(aSVGElement, nsnull,
>+                                                   presShell);
>+  NS_ASSERTION(styleContext, "no styleContext");

helper somewhere e.g. nsSVGElement, nsSVGLength2 or nsSVGUtils for this repeated code (c.f. previous method)

>+
>+  const nsStyleFont* font = styleContext->GetStyleFont();
>+  nsCOMPtr<nsIFontMetrics> fontMetrics = presContext->GetMetricsFor(font->mFont);
>+  nscoord xHeight;
>+  fontMetrics->GetXHeight(xHeight);
>+  float textZoom = presContext->TextZoom();
>+  return nsPresContext::AppUnitsToFloatCSSPixels(xHeight) / textZoom;
>+}
>+
> /* Implementation */
> 
> nsresult
> nsSVGLength2::SetBaseValueString(const nsAString &aValueAsString,
>                                  nsSVGElement *aSVGElement,
>                                  PRBool aDoSetAttr)
> {
>   nsresult rv = NS_OK;
>@@ -249,16 +295,21 @@ void
> nsSVGLength2::GetAnimValueString(nsAString & aValueAsString)
> {
>   GetValueString(aValueAsString, mAnimVal, mSpecifiedUnitType);
> }
> 
> float
> nsSVGLength2::ConvertToUserUnits(float aVal, nsSVGElement *aSVGElement)
> {
>+  if (mSpecifiedUnitType == nsIDOMSVGLength::SVG_LENGTHTYPE_EMS)
>+      return aVal * GetEmLength(aSVGElement);
>+  if (mSpecifiedUnitType == nsIDOMSVGLength::SVG_LENGTHTYPE_EXS)
>+      return aVal * GetExLength(aSVGElement);
>+

switch rather than repeated if perhaps?

>   nsSVGCoordCtx *ctx = nsnull;
> 
>   if (mSpecifiedUnitType != nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER &&
>       mSpecifiedUnitType != nsIDOMSVGLength::SVG_LENGTHTYPE_PX)
>     ctx = aSVGElement->GetCtxByType(mCtxType);
> 
>   return ConvertToUserUnits(aVal, ctx);
> }
>@@ -292,17 +343,19 @@ nsSVGLength2::ConvertToUserUnits(float a
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_PT:
>       return aVal * 25.4f / 72.0f / GetMMPerPixel(aCtx);
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_PC:
>       return aVal * 25.4f * 12.0f / 72.0f / GetMMPerPixel(aCtx);
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE:
>       return aVal * GetAxisLength(aCtx) / 100.0f;
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_EMS:
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_EXS:
>-      NS_NOTYETIMPLEMENTED("nsIDOMSVGLength::SVG_LENGTHTYPE_EXS");
>+      // We should not reach this. Because, em and ex should be calculrated

s/calculrated/calculated/ but better to omit the comment and lines below and just use NS_NOTREACHED though

>+      // from the current element.
>+      NS_ASSERTION(1==0, "em/ex should not be calculated from nsSVGCoordCtx");
>       return 0;
>     default:
>       NS_NOTREACHED("Unknown unit type");
Sorry, meant to elide more code than that.
Attached patch Patch rv.3.0 (obsolete) — Splinter Review
Patch for nsSVGLength and nsSVGLength2.
Assignee: general → taken.spc
Attachment #255749 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 266309 [details] [diff] [review]
Patch rv.3.0

>+float nsSVGLength::EmLength() {

Nit: Prevailing SVG style is for methods to have { on a new line. 
There are a number of instances of this.

>+float nsSVGLength::ExLength() {
>+  nsCOMPtr<nsIContent> element = do_QueryReferent(mElement);
>+  if (!element) {
>+    NS_WARNING("no element in ExLength()");
>+    return 1.0f;
>+  }
>+
>+  nsSVGElement *ctx = NS_STATIC_CAST(nsSVGElement*, element.get());
>+
>+  const nsStyleFont *font;
>+  nsPresContext *presContext;
>+  nsresult rv = nsSVGUtils::GetStyleFontForElement(ctx, &font, &presContext);
>+  if (NS_FAILED(rv))
>+    return 1.0f;
>+
>+  nsCOMPtr<nsIFontMetrics> fontMetrics = presContext->GetMetricsFor(font->mFont);
>+  nscoord xHeight;
>+  fontMetrics->GetXHeight(xHeight);
>+  float textZoom = presContext->TextZoom();
>+  return nsPresContext::AppUnitsToFloatCSSPixels(xHeight) / textZoom;
>+}
>+
>Index: content/svg/content/src/nsSVGLength2.cpp
>+float
>+nsSVGLength2::GetEmLength(nsSVGElement *aSVGElement)
>+{
>+  if (!aSVGElement)
>+    return 1;

Add an NS_WARNING on this test perhaps?

>+
>+  const nsStyleFont* font;
>+  nsPresContext *presContext;
>+  nsresult rv = nsSVGUtils::GetStyleFontForElement(aSVGElement, &font,
>+                                                   &presContext);
>+  if (NS_FAILED(rv))
>+    return 1;
>+
>+  float textZoom = presContext->TextZoom();
>+  return nsPresContext::AppUnitsToFloatCSSPixels(font->mSize) / textZoom;
>+}
>+
>+float
>+nsSVGLength2::GetExLength(nsSVGElement *aSVGElement)
>+{
>+  if (!aSVGElement)
>+    return 1;
>+

Add an NS_WARNING on this test perhaps?

>+  const nsStyleFont* font;
>+  nsPresContext *presContext;
>+  nsresult rv = nsSVGUtils::GetStyleFontForElement(aSVGElement, &font,
>+                                                   &presContext);
>+  if (NS_FAILED(rv))
>+    return 1;
>+
>+  float textZoom = presContext->TextZoom();
>+
>+  return nsPresContext::AppUnitsToFloatCSSPixels(font->mSize) / textZoom;
>+}
>+

GetExLength is the same as GetEmLength shouldn't GetExLength be calling 
fontMetrics->GetXHeight(xHeight); ?

>+nsresult
>+nsSVGUtils::GetStyleFontForElement(nsSVGElement *aSVGElement,
>+                                   const struct nsStyleFont **aStyleFont,
>+                                   nsPresContext **aPresContext) {
>+  nsIDocument *doc = aSVGElement->GetCurrentDoc();
>+  nsIPresShell *presShell = doc->GetPrimaryShell();
>+  
>+  *aPresContext = presShell->GetPresContext();
>+  NS_ENSURE_TRUE(aPresContext, NS_ERROR_FAILURE);

Use NS_ENSURE_ARG_POINTER perhaps and put the tests at the start of the method?

>+
>+  nsRefPtr<nsStyleContext> styleContext =
>+    nsInspectorCSSUtils::GetStyleContextForContent(aSVGElement, nsnull,
>+                                                   presShell);
>+
>+  *aStyleFont = styleContext->GetStyleFont();
>+  NS_ENSURE_TRUE(aStyleFont, NS_ERROR_FAILURE);

See above.

>+   * Get a nsStyleFont and PresContext of given SVGElement
>+   */
>+  static nsresult GetStyleFontForElement(nsSVGElement *aSVGElement,
>+                                         const struct nsStyleFont **aStyleFont,
>+                                         nsPresContext **aPresContext);
>+  /*

Is the const in the right place? Also the struct keyword should be removed here and a struct nsStyleFont added further up nsSVGUtils.h if required.
Attached patch Patch rv.3.1 (obsolete) — Splinter Review
> >+   * Get a nsStyleFont and PresContext of given SVGElement
> >+   */
> >+  static nsresult GetStyleFontForElement(nsSVGElement *aSVGElement,
> >+                                         const struct nsStyleFont **aStyleFont,
> >+                                         nsPresContext **aPresContext);
> >+  /*
> 
> Is the const in the right place? Also the struct keyword should be removed here
> and a struct nsStyleFont added further up nsSVGUtils.h if required.
> 

I think |const| is necessary here. Without |const|, the build fails on my environment.
# Ubuntu Linux 7.04 - kernel 2.6.20-16, gcc 4.1.2
Attachment #266309 - Attachment is obsolete: true
(In reply to comment #23)
> I think |const| is necessary here. Without |const|, the build fails on my
> environment.
> # Ubuntu Linux 7.04 - kernel 2.6.20-16, gcc 4.1.2
> 

OK.

Comment on attachment 266863 [details] [diff] [review]
Patch rv.3.1

Oops, I forget to request the reivew.
Attachment #266863 - Flags: review?(longsonr)
Tor, are you happy for me to formally review this?
Comment on attachment 266863 [details] [diff] [review]
Patch rv.3.1


>Index: content/svg/content/src/nsSVGLength.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGLength.cpp,v
>retrieving revision 1.27
>diff -u -8 -p -w -r1.27 nsSVGLength.cpp
>--- content/svg/content/src/nsSVGLength.cpp	26 Apr 2007 00:24:28 -0000	1.27
>+++ content/svg/content/src/nsSVGLength.cpp	1 Jun 2007 04:35:34 -0000
>@@ -46,16 +46,18 @@
> #include "prdtoa.h"
> #include "nsCRT.h"
> #include "nsSVGSVGElement.h"
> #include "nsIDOMSVGNumber.h"
> #include "nsISVGValueUtils.h"
> #include "nsWeakReference.h"
> #include "nsContentUtils.h"
> #include "nsIDOMSVGAnimatedRect.h"
>+#include "nsCOMPtr.h"

I don't think you need above include

>+#include "nsIFontMetrics.h"


>+float nsSVGLength::EmLength()
>+{
>+  nsCOMPtr<nsIContent> element = do_QueryReferent(mElement);
>+  if (!element) {
>+    NS_WARNING("no element in EmLength()");
>+    return 1.0f;
>+  }
>+  nsSVGElement *ctx = NS_STATIC_CAST(nsSVGElement*, element.get());
>+
>+  const nsStyleFont *font;
>+  nsPresContext *presContext;
>+  nsresult rv = nsSVGUtils::GetStyleFontForElement(ctx, &font, &presContext);
>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("no StyleFont in EmLength()");
>+    return 1.0f;
>+  }
>+
>+  float textZoom = presContext->TextZoom();
>+  return nsPresContext::AppUnitsToFloatCSSPixels(font->mSize) / textZoom;
>+}

Create a helper to calculate the font size and use it above. Something like this...

gfxFloat
nsSVGUtils::GetFontSize(presContext *aPresContext, const nsStyleFont *aFontData)
{
  return nsPresContext::AppUnitsToFloatCSSPixels(aFontData->mSize) /
         presContext->TextZoom();
}

>+
>+float nsSVGLength::ExLength()
>+{
>+  nsCOMPtr<nsIContent> element = do_QueryReferent(mElement);
>+  if (!element) {
>+    NS_WARNING("no element in ExLength()");
>+    return 1.0f;
>+  }
>+
>+  nsSVGElement *ctx = NS_STATIC_CAST(nsSVGElement*, element.get());
>+
>+  const nsStyleFont *font;
>+  nsPresContext *presContext;
>+  nsresult rv = nsSVGUtils::GetStyleFontForElement(ctx, &font, &presContext);
>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("no StyleFont in ExLength()");
>+    return 1.0f;
>+  }
>+
>+  nsCOMPtr<nsIFontMetrics> fontMetrics = presContext->GetMetricsFor(font->mFont);
>+  nscoord xHeight;
>+  fontMetrics->GetXHeight(xHeight);
>+  float textZoom = presContext->TextZoom();
>+  return nsPresContext::AppUnitsToFloatCSSPixels(xHeight) / textZoom;
>+}

Create a helper to return the XHeight perhaps

gfxFloat
nsSVGUtils::GetFontXHeight(presContext *aPresContext, const nsStyleFont *aFontData)

>Index: content/svg/content/src/nsSVGLength2.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGLength2.cpp,v
>retrieving revision 1.3
>diff -u -8 -p -w -r1.3 nsSVGLength2.cpp
>--- content/svg/content/src/nsSVGLength2.cpp	26 Apr 2007 00:24:28 -0000	1.3
>+++ content/svg/content/src/nsSVGLength2.cpp	1 Jun 2007 04:35:34 -0000
>@@ -39,16 +39,18 @@
> #include "nsContentUtils.h"
> #include "nsSVGLength2.h"
> #include "nsGkAtoms.h"
> #include "prdtoa.h"
> #include "nsCRT.h"
> #include "nsTextFormatter.h"
> #include "nsIDOMSVGNumber.h"
> #include "nsSVGSVGElement.h"
>+#include "nsCOMPtr.h"

I don't think you need the above include

>+#include "nsIFontMetrics.h"
> 

>+float
>+nsSVGLength2::GetEmLength(nsSVGElement *aSVGElement)
>+{
>+  if (!aSVGElement)
>+    return 1;

Worth a warning or even a NS_ASSERTION if aSVGElement was null

>+
>+  const nsStyleFont* font;
>+  nsPresContext *presContext;
>+  nsresult rv = nsSVGUtils::GetStyleFontForElement(aSVGElement, &font,
>+                                                   &presContext);
>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("no StyleFont in GetEmLengt()");

typo s/GetEmLengt/GetEmLength/

>+    return 1;
>+  }
>+
>+  float textZoom = presContext->TextZoom();
>+  return nsPresContext::AppUnitsToFloatCSSPixels(font->mSize) / textZoom;
>+}

Use one of the above helpers here.

>+
>+float
>+nsSVGLength2::GetExLength(nsSVGElement *aSVGElement)
>+{
>+  if (!aSVGElement)
>+    return 1;

Worth a warning or even a NS_ASSERTION if aSVGElement was null

>+
>+  const nsStyleFont *font;
>+  nsPresContext *presContext;
>+  nsresult rv = nsSVGUtils::GetStyleFontForElement(aSVGElement, &font,
>+                                                   &presContext);
>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("no StyleFont in GetExLength()");
>+    return 1;
>+  }
>+
>+  nsCOMPtr<nsIFontMetrics> fontMetrics = presContext->GetMetricsFor(font->mFont);
>+  nscoord xHeight;
>+  fontMetrics->GetXHeight(xHeight);
>+  float textZoom = presContext->TextZoom();
>+  return nsPresContext::AppUnitsToFloatCSSPixels(xHeight) / textZoom;
>+}

Use one of the above helpers here.

> void
> nsSVGLength2::SetBaseValue(float aValue, nsSVGElement *aSVGElement)
> {
>   nsSVGSVGElement *ctx = nsnull;
> 
>   if (mSpecifiedUnitType != nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER &&
>-      mSpecifiedUnitType != nsIDOMSVGLength::SVG_LENGTHTYPE_PX)
>+      mSpecifiedUnitType != nsIDOMSVGLength::SVG_LENGTHTYPE_PX &&
>+      mSpecifiedUnitType != nsIDOMSVGLength::SVG_LENGTHTYPE_EMS &&
>+      mSpecifiedUnitType != nsIDOMSVGLength::SVG_LENGTHTYPE_EXS)
>     ctx = aSVGElement->GetCtx();
> 
>   switch (mSpecifiedUnitType) {
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER:
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_PX:
>       mBaseVal = aValue;
>       break;
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_MM:
>@@ -315,19 +366,20 @@ nsSVGLength2::SetBaseValue(float aValue,
>       break;
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_PC:
>       mBaseVal = aValue * GetMMPerPixel(ctx) * 72.0f / 24.4f / 12.0f;
>       break;
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE:
>       mBaseVal = aValue * 100.0f / GetAxisLength(ctx);
>       break;
>     case nsIDOMSVGLength::SVG_LENGTHTYPE_EMS:
>+      mBaseVal = aValue / GetEmLength(ctx);
>+      break;

Didn't you mean to pass aSVGElement rather than ctx here? ctx will be null because of the if test change above.

>     case nsIDOMSVGLength::SVG_LENGTHTYPE_EXS:
>-      NS_NOTYETIMPLEMENTED("nsIDOMSVGLength::SVG_LENGTHTYPE_EXS");
>-      mBaseVal = 0;
>+      mBaseVal = aValue / GetExLength(ctx);

Didn't you mean to pass aSVGElement rather than ctx here too?

>       break;
>     default:
>       NS_NOTREACHED("Unknown unit type");
>       mBaseVal = 0;
>       break;
>   }
> 
>   mAnimVal = mBaseVal;
>Index: layout/svg/base/src/nsSVGUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGUtils.cpp,v
>retrieving revision 1.77
>diff -u -8 -p -w -r1.77 nsSVGUtils.cpp
>--- layout/svg/base/src/nsSVGUtils.cpp	30 May 2007 22:32:56 -0000	1.77
>+++ layout/svg/base/src/nsSVGUtils.cpp	1 Jun 2007 04:35:34 -0000
>@@ -76,16 +76,17 @@
> 
>+nsresult
>+nsSVGUtils::GetStyleFontForElement(nsSVGElement *aSVGElement,
>+                                   const nsStyleFont **aStyleFont,
>+                                   nsPresContext **aPresContext)

I think you should split this up into two functions, one to return the
nsPresContext...

nsPresContext*
nsSVGUtils::GetContextForContent(nsIContent* aContent)
{
  nsIDocument* doc = aContent->GetCurrentDoc();

  if (doc) {
    nsIPresShell *presShell = doc->GetPrimaryShell();
    if (presShell) {
      return presShell->GetPresContext();
    }
  }
  return nsnull;
}

and one to return the nsStyleFont given an nsIContent *

const nsStyleFont*
nsSVGUtils::GetStyleFontForContent(nsIContent* aContent)

There may be some overlap given that you will get the nsIDocument and presShell
again in this function but those are simple accessor methods.
Attachment #266863 - Attachment is obsolete: true
Attachment #267665 - Flags: review?(longsonr)
Attachment #266863 - Flags: review?(longsonr)
Comment on attachment 267665 [details] [diff] [review]
Patch rv.3.2 (address review comments)

>Index: layout/svg/base/src/nsSVGUtils.h
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGUtils.h,v
> #include "nsRect.h"
>+#include "nsStyleStruct.h"
>+#include "gfxContext.h"

I don't think you need the above includes, especially once you implement the comments below. If you need nsStyleFont then just make a forward declare for it as in the lines below.

> class nsStyleCoord;
> class nsIDOMSVGRect;
> class nsFrameList;
> class nsIFrame;

>+  /*
>+   * Get an x-height of the font
>+   */
>+  static gfxFloat GetFontXHeight(nsPresContext *aPresContext, 
>+                                 const nsStyleFont *aFontData);

Make this function return a float rather than a gfxFloat to be compatible with the return of its callers. My fault as I suggested gfxFloat in the first place.

>+  /*
>+   * Get an font-size (em) of the font
>+   */
>+  static gfxFloat GetFontSize(nsPresContext *aPresContext,
>+                              const nsStyleFont *aFontData);
>+

return a float rather than a gfxFloat here too.
Attachment #267665 - Attachment is obsolete: true
Attachment #267824 - Flags: review?(longsonr)
Attachment #267665 - Flags: review?(longsonr)
Attachment #267824 - Attachment description: Patch rv.3.0 (address review comments) → Patch rv.3.3 (address review comments)
Comment on attachment 267824 [details] [diff] [review]
Patch rv.3.3 (address review comments)

> float
> nsSVGLength2::ConvertToUserUnits(float aVal, nsSVGElement *aSVGElement)
> {
>-  if (mSpecifiedUnitType == nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER ||
>-      mSpecifiedUnitType == nsIDOMSVGLength::SVG_LENGTHTYPE_PX)
>+  switch (mSpecifiedUnitType) {
>+    case nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER:
>+    case nsIDOMSVGLength::SVG_LENGTHTYPE_PX:
>     return aVal;
>-
>+    case nsIDOMSVGLength::SVG_LENGTHTYPE_EMS:
>+      return aVal * GetEmLength(aSVGElement);
>+    case nsIDOMSVGLength::SVG_LENGTHTYPE_EXS:
>+      return aVal * GetExLength(aSVGElement);
>+    default:
>   return ConvertToUserUnits(aVal, aSVGElement->GetCtx());
> }
>+}

Nit: Fix indenting of } in switch above.
Attachment #267824 - Flags: review?(longsonr) → review+
Attached patch Patch rv.3.4 (fixing the nit) (obsolete) — Splinter Review
Attachment #267824 - Attachment is obsolete: true
Attachment #267961 - Flags: superreview?(tor)
Attachment #267961 - Flags: review+
Rather than duplicate code in nsSVGLength and nsSVGLength2 for EmLength and ExLength, why not just have two methods in nsSVGUtils:

  float GetFontSize(nsIContent *);
  float GetFontXHeight(nsIContent *);
(In reply to comment #33)
> Rather than duplicate code in nsSVGLength and nsSVGLength2 for EmLength and
> ExLength, why not just have two methods in nsSVGUtils:
> 
>   float GetFontSize(nsIContent *);
>   float GetFontXHeight(nsIContent *);
> 

address this comment.
Tor, could you super-review my patch again?
Attachment #267961 - Attachment is obsolete: true
Attachment #268068 - Flags: superreview?(tor)
Attachment #268068 - Flags: review+
Attachment #267961 - Flags: superreview?(tor)
Comment on attachment 268068 [details] [diff] [review]
Patch rv.3.5 (address the comment)

>Index: content/svg/content/src/nsSVGElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGElement.cpp,v
>retrieving revision 1.120
>diff -u -8 -p -r1.120 nsSVGElement.cpp
>--- content/svg/content/src/nsSVGElement.cpp	14 May 2007 09:11:44 -0000	1.120
>+++ content/svg/content/src/nsSVGElement.cpp	12 Jun 2007 09:48:25 -0000
>@@ -841,32 +841,24 @@ nsSVGElement::DidChangeLength(PRUint8 aA
> void
> nsSVGElement::GetAnimatedLengthValues(float *aFirst, ...)
> {
>   LengthAttributesInfo info = GetLengthInfo();
> 
>   NS_ASSERTION(info.mLengthCount > 0,
>                "GetAnimatedLengthValues on element with no length attribs");
> 
>-  nsSVGSVGElement *ctx = nsnull;
>-    if (!ctx) {
>-      PRUint8 type = info.mLengths[i].GetSpecifiedUnitType();
>-      if (type != nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER &&
>-          type != nsIDOMSVGLength::SVG_LENGTHTYPE_PX)
>-        ctx = GetCtx();
>-    }
>-    *f = info.mLengths[i++].GetAnimValue(ctx);
>+    *f = info.mLengths[i++].GetAnimValue(this);
>     f = va_arg(args, float*);
>   }

I'd like to keep this optimization similar to what you had in earlier versions of your patch, as I think percentage lengths might be common enough to keep it around.  This could probably be done by just adding this after the "if (!ctx) {}":

if (type == SVG_LENGTHTYPE_EMS || type == ...EMX) {
  *f = info.mLengths[i++].GetAnimValue(this);
} else {
  *f = info.mLengths[i++].GetAnimValue(ctx);
}

>+float nsSVGLength::EmLength()
>+{
>+  nsCOMPtr<nsIContent> element = do_QueryReferent(mElement);
>+  if (!element) {
>+    NS_WARNING("no element in EmLength()");
>+    return 1.0f;
>+  }
>+
>+  return nsSVGUtils::GetFontSize(element);
>+}
>+
>+float nsSVGLength::ExLength()
>+{
>+  nsCOMPtr<nsIContent> element = do_QueryReferent(mElement);
>+  if (!element) {
>+    NS_WARNING("no element in ExLength()");
>+    return 1.0f;
>+  }
>+
>+  return nsSVGUtils::GetFontXHeight(element);
>+}

If you change the nsSVGUtils methods to check for a null passed element, you could remove the test here.

>+float
>+nsSVGLength2::GetEmLength(nsSVGElement *aSVGElement)
>+{
>+  if (!aSVGElement) {
>+    NS_WARNING("no element in GetEmLength()");
>+    return 1;
>+  }
>+
>+  return nsSVGUtils::GetFontSize(aSVGElement);
>+}
>+
>+float
>+nsSVGLength2::GetExLength(nsSVGElement *aSVGElement)
>+{
>+  if (!aSVGElement) {
>+    NS_WARNING("no element in GetExLength()");
>+    return 1;
>+  }
>+
>+  return nsSVGUtils::GetFontXHeight(aSVGElement);
>+}

As above, and these methods could be dispensed with entirely for nsSVGLength2.

>+nsPresContext*
>+nsSVGUtils::GetContextForContent(nsIContent* aContent)
>+{
>+  nsIDocument* doc = aContent->GetCurrentDoc();
>+
>+  if (doc) {
>+    nsIPresShell *presShell = doc->GetPrimaryShell();
>+    if (presShell) {
>+      return presShell->GetPresContext();
>+    }
>+  }
>+  return nsnull;
>+}
>+
>+const nsStyleFont*
>+nsSVGUtils::GetStyleFontForContent(nsIContent* aContent)
>+{
>+
>+  nsIDocument *doc = aContent->GetCurrentDoc();
>+
>+  if (doc) {
>+    nsIPresShell *presShell = doc->GetPrimaryShell();
>+    if (presShell) {
>+      nsRefPtr<nsStyleContext> styleContext =
>+        nsInspectorCSSUtils::GetStyleContextForContent(aContent, nsnull,
>+                                                       presShell);
>+      return styleContext->GetStyleFont();

Check for valid styleContext?

>+    }
>+  }
>+  
>+  return nsnull;
>+}

This contradicts Robert's comment, but since these two methods are only used by GetFontSize/GetXHeight, I'd be inclined to have a single method to look up both bits of data like some of your previous patches had.

>+float
>+nsSVGUtils::GetFontSize(nsIContent *aContent)
>+{
>+  nsPresContext *presContext = nsSVGUtils::GetContextForContent(aContent);
>+  const nsStyleFont* fontData = nsSVGUtils::GetStyleFontForContent(aContent);
>+  if (!fontData) {
>+    NS_WARNING("no StyleFont in GetFontSize()");
>+    return 1.0f;
>+  }
>+
>+  return (float)nsPresContext::AppUnitsToFloatCSSPixels(fontData->mSize) /
>+                presContext->TextZoom();
>+}
>+
>+
>+float
>+nsSVGUtils::GetFontXHeight(nsIContent *aContent)
>+{
>+  nsPresContext *presContext = nsSVGUtils::GetContextForContent(aContent);
>+  const nsStyleFont* fontData = nsSVGUtils::GetStyleFontForContent(aContent);
>+  if (!fontData) {
>+    NS_WARNING("no StyleFont in GetFontSize()");
>+    return 1.0f;
>+  }
>+
>+  nsCOMPtr<nsIFontMetrics> fontMetrics = presContext->
>+                                         GetMetricsFor(fontData->mFont);
>+  nscoord xHeight;
>+  fontMetrics->GetXHeight(xHeight);

Should probably check that fontMetrics is valid.

>+  return (float)nsPresContext::AppUnitsToFloatCSSPixels(xHeight) /
>+                presContext->TextZoom();
>+}

In both, check for valid presContext?

> class nsSVGUtils
> {
> public:
>   /*
>+   * Get an nsPresContext from an nsIContent
>+   */
>+  static nsPresContext* GetContextForContent(nsIContent* aContent);
>+
>+  /*
>+   * Get an nsStyleFont from an nsIContent
>+   */
>+  static const nsStyleFont* GetStyleFontForContent(nsIContent *aContent);

These should be private methods (or single method - see above).
(In reply to comment #35)
> 
> This contradicts Robert's comment, but since these two methods are only used by
> GetFontSize/GetXHeight, I'd be inclined to have a single method to look up both
> bits of data like some of your previous patches had.
> 

My thinking was to simplify nsSVGSVGElement::GetPixelUnitToMillimeterX and nsSVGSVGElement::GetScreenPixelToMillimeterX in a later follow up using GetContextForContent and even move the method at that point to nsContentUtils to share with nsScriptElement
c.f. 384409
Address the comments.
|GetContextForContent| and |GetStyleFontForContent| are left but are |private| now.
Attachment #268068 - Attachment is obsolete: true
Attachment #268663 - Flags: superreview?(tor)
Attachment #268663 - Flags: review+
Attachment #268068 - Flags: superreview?(tor)
Attachment #268663 - Flags: superreview?(tor) → superreview+
tor, thanks your super-view. I don't have the commit access. So, could you check the patch in?
Checked in.  Thanks again for working on this.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
When I load the SVG from this bugs URL field and change my font-size via the Keyboard Shortcuts, only the stroke-width of the boxes changes, but the boxes stay the same size.
With the unit test from the official SVG testsuite ( http://www.w3.org/Graphics/SVG/Test/20061213/svggen/coords-units-03-b.svg ) the width of the em line stays the same, however the width of the ex line changes.
But it doesn't change how one would expect. It first grows, then shrinks and grows again when I press ctrl+- 3 times. Could this also be a problem with my default font, since x-height is font-stpecific, isn't it?
Sorry for double posting btw :/
(In reply to comment #43)
> With the unit test from the official SVG testsuite (
> http://www.w3.org/Graphics/SVG/Test/20061213/svggen/coords-units-03-b.svg ) the
> width of the em line stays the same, however the width of the ex line changes.
> But it doesn't change how one would expect. It first grows, then shrinks and
> grows again when I press ctrl+- 3 times. Could this also be a problem with my
> default font, since x-height is font-stpecific, isn't it?
> Sorry for double posting btw :/
> 

Those are rounding errors. Font sizes are integers in gecko. Nothing should happen when you change the font size see bug 291785 for details.

Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.