Last Comment Bug 305859 - Em and ex units not implemented for SVGLength (tspan often rendered incorrectly)
: Em and ex units not implemented for SVGLength (tspan often rendered incorrectly)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- major with 8 votes (vote)
: ---
Assigned To: Takeshi Kurosawa
: Hixie (not reading bugmail)
Mentors:
http://www.w3.org/TR/SVG11/images/coo...
: 312165 367883 373601 401467 410162 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-08-24 22:20 PDT by John Prevost
Modified: 2009-07-06 05:15 PDT (History)
17 users (show)
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Partial patch (nsSVGLength2 only) (7.01 KB, patch)
2007-02-19 07:30 PST, Takeshi Kurosawa
no flags Details | Diff | Splinter Review
Partial patch (nsSVGLength2 only) #2 (6.56 KB, patch)
2007-02-19 19:37 PST, Takeshi Kurosawa
no flags Details | Diff | Splinter Review
Partial patch (nsSVGLength2 only) #2.0.0.1 (7.81 KB, patch)
2007-02-19 19:44 PST, Takeshi Kurosawa
no flags Details | Diff | Splinter Review
Patch rv.3.0 (15.17 KB, patch)
2007-05-27 17:48 PDT, Takeshi Kurosawa
no flags Details | Diff | Splinter Review
Patch rv.3.1 (16.35 KB, patch)
2007-05-31 21:47 PDT, Takeshi Kurosawa
no flags Details | Diff | Splinter Review
Patch rv.3.2 (address review comments) (15.77 KB, patch)
2007-06-07 18:30 PDT, Takeshi Kurosawa
no flags Details | Diff | Splinter Review
Patch rv.3.3 (address review comments) (15.77 KB, patch)
2007-06-09 14:58 PDT, Takeshi Kurosawa
longsonr: review+
Details | Diff | Splinter Review
Patch rv.3.4 (fixing the nit) (15.82 KB, patch)
2007-06-11 06:38 PDT, Takeshi Kurosawa
taken.spc: review+
Details | Diff | Splinter Review
Patch rv.3.5 (address the comment) (15.08 KB, patch)
2007-06-12 03:08 PDT, Takeshi Kurosawa
taken.spc: review+
Details | Diff | Splinter Review
Patch rv.3.6 (address review comments) (14.91 KB, patch)
2007-06-17 01:18 PDT, Takeshi Kurosawa
taken.spc: review+
tor: superreview+
Details | Diff | Splinter Review

Description John Prevost 2005-08-24 22:20:57 PDT
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.
Comment 1 John Prevost 2005-08-24 22:29:46 PDT
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.
Comment 2 John Prevost 2005-08-24 22:38:14 PDT
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.
Comment 3 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2005-10-14 07:16:26 PDT
*** Bug 312165 has been marked as a duplicate of this bug. ***
Comment 4 Standa Opichal 2005-12-10 16:18:33 PST
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
Comment 5 Gwen Epstein 2006-02-03 15:05:25 PST
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.
Comment 6 John Prevost 2006-02-04 13:27:40 PST
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.
Comment 7 Johannes Wierny 2006-02-05 23:04:19 PST
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.
Comment 8 E A Merritt 2006-10-23 22:08:16 PDT
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)
Comment 9 Mihail Golub 2006-11-25 05:12:43 PST
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.
Comment 10 Catalin Hritcu 2006-12-11 01:49:36 PST
Bug is still present in Firefox 3.0 Alpha 1: Grand Paradiso (Gecko 1.9a1)
Comment 11 Takeshi Kurosawa 2006-12-11 03:16:26 PST
I知 taking a look for the em/ex units.
My initially work depends on Attachment 247233 [details] [diff] from bug 353172.
Comment 12 Robert Longson 2007-01-23 06:23:22 PST
*** Bug 367883 has been marked as a duplicate of this bug. ***
Comment 13 Henri Sivonen (:hsivonen) 2007-01-23 12:24:04 PST
Supporting em on the width and height of the outer svg element would already be a big functionality and interoperability win.
Comment 14 Henri Sivonen (:hsivonen) 2007-02-19 03:59:47 PST
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.
Comment 15 Takeshi Kurosawa 2007-02-19 07:30:25 PST
Created attachment 255684 [details] [diff] [review]
Partial patch (nsSVGLength2 only)

Partial patch (nsSVGLength2 only). This patch does not solve tspan problems.
Comment 16 Takeshi Kurosawa 2007-02-19 19:37:51 PST
Created attachment 255748 [details] [diff] [review]
Partial patch (nsSVGLength2 only) #2

minor fixes
Comment 17 Takeshi Kurosawa 2007-02-19 19:44:05 PST
Created attachment 255749 [details] [diff] [review]
Partial patch (nsSVGLength2 only) #2.0.0.1

Oops, In previous patch, I forgot nsSVGLength2.h.
Comment 18 Robert Longson 2007-03-10 03:15:11 PST
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");
Comment 19 Robert Longson 2007-03-10 03:16:20 PST
Sorry, meant to elide more code than that.
Comment 20 Robert Longson 2007-04-04 09:05:28 PDT
*** Bug 373601 has been marked as a duplicate of this bug. ***
Comment 21 Takeshi Kurosawa 2007-05-27 17:48:05 PDT
Created attachment 266309 [details] [diff] [review]
Patch rv.3.0

Patch for nsSVGLength and nsSVGLength2.
Comment 22 Robert Longson 2007-05-28 02:21:27 PDT
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.
Comment 23 Takeshi Kurosawa 2007-05-31 21:47:11 PDT
Created attachment 266863 [details] [diff] [review]
Patch rv.3.1

> >+   * 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
Comment 24 Robert Longson 2007-06-01 03:52:03 PDT
(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 25 Takeshi Kurosawa 2007-06-02 04:33:52 PDT
Comment on attachment 266863 [details] [diff] [review]
Patch rv.3.1

Oops, I forget to request the reivew.
Comment 26 Robert Longson 2007-06-03 09:36:39 PDT
Tor, are you happy for me to formally review this?
Comment 27 Robert Longson 2007-06-05 10:11:07 PDT
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.
Comment 28 Takeshi Kurosawa 2007-06-07 18:30:14 PDT
Created attachment 267665 [details] [diff] [review]
Patch rv.3.2 (address review comments)
Comment 29 Robert Longson 2007-06-08 01:59:57 PDT
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.
Comment 30 Takeshi Kurosawa 2007-06-09 14:58:23 PDT
Created attachment 267824 [details] [diff] [review]
Patch rv.3.3 (address review comments)
Comment 31 Robert Longson 2007-06-10 14:08:14 PDT
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.
Comment 32 Takeshi Kurosawa 2007-06-11 06:38:36 PDT
Created attachment 267961 [details] [diff] [review]
Patch rv.3.4 (fixing the nit)
Comment 33 tor 2007-06-11 15:43:37 PDT
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 *);
Comment 34 Takeshi Kurosawa 2007-06-12 03:08:27 PDT
Created attachment 268068 [details] [diff] [review]
Patch rv.3.5 (address the comment)

(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?
Comment 35 tor 2007-06-13 16:18:26 PDT
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).
Comment 36 Robert Longson 2007-06-14 00:08:19 PDT
(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
Comment 37 Robert Longson 2007-06-14 03:36:38 PDT
c.f. 384409
Comment 38 Robert Longson 2007-06-14 03:37:00 PDT
c.f. bug 384409
Comment 39 Takeshi Kurosawa 2007-06-17 01:18:41 PDT
Created attachment 268663 [details] [diff] [review]
Patch rv.3.6 (address review comments)

Address the comments.
|GetContextForContent| and |GetStyleFontForContent| are left but are |private| now.
Comment 40 Takeshi Kurosawa 2007-06-23 04:50:31 PDT
tor, thanks your super-view. I don't have the commit access. So, could you check the patch in?
Comment 41 tor 2007-06-25 09:13:00 PDT
Checked in.  Thanks again for working on this.
Comment 42 Arpad Borsos [:Swatinem] 2007-06-26 10:44:06 PDT
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.
Comment 43 Arpad Borsos [:Swatinem] 2007-06-26 10:55:16 PDT
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 :/
Comment 44 Robert Longson 2007-06-26 16:34:04 PDT
(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.

Comment 45 Robert Longson 2007-10-28 15:59:27 PDT
*** Bug 401467 has been marked as a duplicate of this bug. ***
Comment 46 Robert Longson 2007-12-29 12:59:16 PST
*** Bug 410162 has been marked as a duplicate of this bug. ***

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