569722 - Firefox SVG support does not implement the textLength and lengthAdjust attribute functionality of <text> elements

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[hkjunkie]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.0.18) Gecko/2010020220 (CK-zz) Firefox/3.0.18 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3) Gecko/20100401 (CK-zz) Firefox/3.6.3 (.NET CLR 3.5.30729)

For SVG to be used as a viable alternative in industrial diagrams to CGM or the developing webCGM standard, it is vital that the textLength and lengthAdjust attributes of text elements be supported.  SVG graphics not having the ability to have the author specify a region that is to be filled with the text, which leaves the rendering entirely to the client font availability, cripples the use of SVG for industrial engineering drawings where not only text placement but importantly, text extent and position, are critical to a diagram. 

Reproducible: Always

Steps to Reproduce:
1.Load the example URL that test the functionality.
2.See that firefox does not render the text as it should.

Actual Results:  
The text is not expanded nor compressed to fit within the space defined by the textLength attributes of the text elements.  The PNG bitmap image on the right half of the page depicts how the text should look.

Expected Results:  
The two images (the svg on the left and png on the right) to look similar in how the text fills out the space defined by the bar-markers.  The blue text elements should match the length of the red line's below them in the SVG.

I did the due diligence of searching the bug list for textLength and found nothing.

Comment 1

[Martijn Wargers (dead)]

I'm getting this error message in the js console, when trying to iterate through the properties of a textPath element:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOMSVGTextContentElement.textLength]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"

Comment 3

[Marco Perez [:mpk]]

In this area we seem to lag behind our major competitors.
Tested with the current release versions (on Windows):
 * IE 9
 * Chrome 13
 * Safari 5.1
 * Opera 11.50

They all seem to more or less pass the W3C test at the URL
(with minor differences how they squeeze/stretch the text).

Comment 4

[Michael O'Rourke]

I'd like to add a vote to fix this. Firefox does lag other browsers on this and without it it is difficult to support full justified text. It is possible via absolute glyph positioning but this tends to look bad on some platforms like iOS.

Comment 5

[J.Vandervort]

Still badly needed for technical drawings and documentation where text alignment is important.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.1)

Here's a WIP patch I worked up today.  The test case in the URL works, except that it sometimes wildly misplaces the glyphs.  I think this is due to the same underlying issues as bug 854286, where the <object> begins lift at a very small size, we reflow the text, getting a large mFontSizeScaleFactor, then the <object> is resized to its final size, but we don't reflow the text.

Maybe we can add a new NotifySVGChanged flag to indicate that the nsSVGOuterSVGFrame changed size.

No tests written yet.

Comment 7

[Cameron McCormack (:heycam)]

There are some question, though:

* what does it mean for some character positions to be specified, while using
  textLength=""?

    <text x="10 20 30" textLength="100">abcdef</text>

  I think we could say that if we were to insert a gap of n user units between
  each glyph without the "20 30", then we would do the same with the "20 30" there
  but only between "cd" "de" and "ef".

* what does it mean for textLength="" to be used on a <tspan> at the same time
  as on the <text>?

    <text textLength="100">abc<tspan textLengt="50">def</tspan></text>

  That could get complicated when bidi text is involved.  I think I'd be happy
  supporting textLength="" only on the <text> and not on descendent text content
  elements.

Comment 8

[Robert Longson [:longsonr]]

Comment on attachment 771218 [details] [diff] [review]
WIP (v0.1)

>+nsSVGEnum*
>+nsSVGElement::GetAnimatedEnumeration(nsIAtom* aAttrName)
>+{
>+  EnumAttributesInfo info = GetEnumInfo();
>+  for (uint32_t i = 0; i < info.mEnumCount; i++) {
>+    if (aAttrName == *info.mEnumInfo[i].mName) {
>+      return &info.mEnums[i];
>+    }
>+  }
>+  NS_ABORT_IF_FALSE(false, "no matching length found");

no matching enum found

Comment 9

[J.Vandervort]

(In reply to Cameron McCormack (:heycam) from comment #7)
> There are some question, though:
> 
> 
> * what does it mean for textLength="" to be used on a <tspan> at the same
> time
>   as on the <text>?
> 
>     <text textLength="100">abc<tspan textLengt="50">def</tspan></text>
> 
>   That could get complicated when bidi text is involved.  I think I'd be
> happy
>   supporting textLength="" only on the <text> and not on descendent text
> content
>   elements.

I would think if there were textlengths on both text and tspan that the tspan textlength would just override the textlength on the text tag.  Wow that's a mouthful.

Comment 10

[Robert Longson [:longsonr]]

    <text x="10 20 30" textLength="100">abcdef</text>

Since we're ignoring the advances for the first 3 characters they get placed as if there was no textLength. We just scale the advances for the def positioning (but we calculate the scale as the total advances for all the text based on the font data).

<text textLength="100">abc<tspan textLengt="50">def</tspan></text>

So the tspan textLength is 50 which means that the abc advance values must total 100 - 50 as the def advance textLength sums to 50. If the text textLength < sum of child textLengths then presumably we ignore the textLength value.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.2)

Updated to fix the problem with <object>.  We now reflow the text whenever our context scale changes by more than a factor of two since we last computed mFontSizeScaleFactor.

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #10)
>     <text x="10 20 30" textLength="100">abcdef</text>
> 
> Since we're ignoring the advances for the first 3 characters they get placed
> as if there was no textLength. We just scale the advances for the def
> positioning (but we calculate the scale as the total advances for all the
> text based on the font data).

OK, I think that's what is happening at the moment, though I haven't tested exactly.

> <text textLength="100">abc<tspan textLengt="50">def</tspan></text>
> 
> So the tspan textLength is 50 which means that the abc advance values must
> total 100 - 50 as the def advance textLength sums to 50. If the text
> textLength < sum of child textLengths then presumably we ignore the
> textLength value.

This makes sense, but I just wonder whether it is worth supporting this.  Is there really a good use case for it?  I like the simplicity of just storing a single scale factor for the whole text (when using lengthAdjust="spacesAndGlyphs"), rather than potentially one per text frame or rendered run.  For the glyph position adjustments when using lengthAdjust="spaces", we again would need to store the adjustment amount per text frame (or it might be easier to do per characters, since in DoGlyphPositioning we are just iterating over the mPositions array), rather than a single adjustment amount to add in to each eligible character position.

Comment 13

[Robert Longson [:longsonr]]

Could always have the tspan case as a follow up.

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #13)
> Could always have the tspan case as a follow up.

Yeah, I think I'll leave it to later.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: patch (v1)

Added some tests and fixed a problem with the scale being applied in the wrong order relative to glyph rotation.

Comment 16

[Cameron McCormack (:heycam)]

Try run in progress: https://tbpl.mozilla.org/?tree=Try&rev=3d2bfc197b4b

Comment 17

[Cameron McCormack (:heycam)]

I'll work on the reftest failures.

Comment 18

[Robert Longson [:longsonr]]

Comment on attachment 771744 [details] [diff] [review]
patch (v1)

>+nsSVGElement::EnumInfo SVGTextContentElement::sEnumInfo[1] =
>+{
>+  { &nsGkAtoms::lengthAdjust, sLengthAdjustMap, SVG_LENGTHADJUST_SPACING }
>+};
>+
>+nsSVGElement::LengthInfo SVGTextContentElement::sLengthInfo[1] =
>+{
>+  { &nsGkAtoms::textLength, 0, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER, SVGContentUtils::XY }
>+};
>+

Should add a comment noting that this design will need adjusting if we want to support textLength on textPath elements as they already have length and enum attriubte and the GetEnumInfo and GetLengthInfo of SVGTextPathElement won't iterate over these properties. Note this in the follow up bug that tracks doing this for tspan/textPath too.

You get away with it for text (would be ok for tspan too) as that element doesn't have length or enum attributes.

> 
>+nsSVGEnum*
>+nsSVGElement::GetAnimatedEnumeration(nsIAtom* aAttrName)
>+{
>+  EnumAttributesInfo info = GetEnumInfo();
>+  for (uint32_t i = 0; i < info.mEnumCount; i++) {
>+    if (aAttrName == *info.mEnumInfo[i].mName) {
>+      return &info.mEnums[i];
>+    }
>+  }
>+  NS_ABORT_IF_FALSE(false, "no matching enum found");
>+  return nullptr;
>+}
>+

You don't need this, you can just cast mContent to SVGTextContentElement in nsSVGTextFrame2 and access the enum value directly.

> 
>+  nsSVGEnum* GetAnimatedEnumeration(nsIAtom* aAttrName);

As above

>+double

static double

>+GetContextScale(const gfxMatrix& aMatrix)
>+{


>+  // If the textLength="" attribute was specified, then we need ResolvePositions
>+  // to record that a new run starts with each glyph.
>+  nsSVGElement* element = static_cast<nsSVGElement*>(mContent);

Cast to SVGTextContentElement instead.

>+  nsSVGLength2* textLengthAttr =
>+    element->GetAnimatedLength(nsGkAtoms::textLength);

Then get the attribute directly from mContent to avoid iterating over all the attributes.

>+  bool adjustingTextLength = textLengthAttr->IsExplicitlySet();
>+  float expectedTextLength = textLengthAttr->GetAnimValue(element);
>+  if (adjustingTextLength) {
>+    nscoord frameWidth = GetFirstPrincipalChild()->GetRect().width;
>+    float actualTextLength =
>+      static_cast<float>(presContext->AppUnitsToGfxUnits(frameWidth) * factor);
>+
>+    uint16_t lengthAdjust =
>+      element->GetAnimatedEnumeration(nsGkAtoms::lengthAdjust)->GetAnimValue();

Just get the attribute from the textContent element directly via mContent. See above.

Would like to see some SMIL tests that animation of these properties works.

r=longsonr with above issues fixed.

Comment 19

[Robert Longson [:longsonr]]

One other thing... Will TextRenderedRun::GetCharNumAtPosition need adjusting to cope with these properties.

Comment 20

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #19)
> One other thing... Will TextRenderedRun::GetCharNumAtPosition need adjusting
> to cope with these properties.

I think it shouldn't need to, as it calls GetTransformFromRunUserSpaceToUserSpace which I've changed to take textLength="" into account.  I'll add a test for these SVG DOM methods.

Comment 21

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9865f43dbe

Comment 22

[Cameron McCormack (:heycam)]

Bug 890692 is the followup for these attributes on <altGlyph, <tspan> and <textPath>.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd9865f43dbe

Comment 24

[John Hewson]

The current implementation of textLength doesn't take into account the dx attribute on child <tspan> elements. The spacing contributed by the dx should be re-scaled proportionally, i.e. a dx which represents 10% of its parent <text> element's width should represent 10% of that width after the parent <text> element is adjusted. Instead the absolute size of the space is never adjusted.

Right now, this is implemented correctly in the other major browsers, namely:

* IE 9
* Chrome 31
* Safari 7

So the FF implementation of textLength on <text> is not in true parity with these browsers.

Here's a simple test case, the two strings should be the same length:

<svg xmlns="http://www.w3.org/2000/svg" version="1.1">
  <style>
    text { font: 20px Verdana }
    tspan { fill: red; font-weight: bold }
  </style>
  <text x="15" y="30">You are 
    <tspan dx="10">not</tspan> 
    a banana
  </text>
  <rect x="15" y="37" width="230" height="3" fill="green"/>
  <text x="15" y="60" textLength="230">You are 
    <tspan dx="10">not</tspan> 
    a banana
  </text>
</svg>

Comment 25

[Robert Longson [:longsonr]]

Please raise a new bug for this issue and Cc cam and me on it

Comment 26

[John Hewson]

Created bug 950110 for the issue with dx attribute on child <tspan> elements