As per the following message to www-svg I think the way we're counting characters in SVG text metrics APIs is wrong with regards to characters marked display:none. http://lists.w3.org/Archives/Public/www-svg/2011Nov/0135.html For example, getNumberOfChars does not count characters in tspans that are display:none but the spec says of this method: "Effectively, this is equivalent to the length of the Node::textContent attribute from DOM Level 3 Core" which doesn't talk about display:none making characters disappear. At least the following methods are affected: getNumberOfChars getComputedTextLength getSubStringLength getStartPositionOfChar/getEndPositionOfChar/getExtentOfChar For further details and interop information see the above message to www-svg. For this bug I think we should: 1. Write a lot of tests for combinations of text/tspan/textPath etc. and display:none to cover BOTH rendering and API results 2. Refactor the display stuff so we generate frames for display:none glyphs 3. Skip those frames for rendering but include them when counting somehow? I've attached a draft patch that does most of 2.

The attached patch applies on top of those for bug 376027.

Note that with Cameron's work, SVG text will go through the regular nsBlockFrame/nsInlineFrame/nsTextFrame frame construction path. We really don't want that code to have to account for possible display:none frames. I think display:none text and <tpsan>s should be treated as not existing for all these APIs. No-one replied to your email to www-svg so I guess no-one feels strongly about it. It's consistent with what browsers already do.

I haven't looked at the issue properly yet. It's on the agenda for tomorrow morning's telcon. At first glance, ignoring those display:none characters seems inconsistent with having x="", dx="" etc. attributes work on just what characters are in the content.

BTW it doesn't mean that we have to build frames for these display:none <tspan>s, right? It just means we have to make sure we count the characters in them so that the indices you pass in to getComputedTextLength etc. take them into account. We would need to define what say getStartPositionOfChar does when given an index to a character that is currently not displayed due to display:none. It's probably the same case for characters corresponding to glyphs that have fallen off the end of a text path (which also isn't handled in the definitions of these methods in the spec).

(In reply to Cameron McCormack (:heycam) from comment #4) > BTW it doesn't mean that we have to build frames for these display:none > <tspan>s, right? It just means we have to make sure we count the characters > in them so that the indices you pass in to getComputedTextLength etc. take > them into account. Yes, I guess so.

Since the nsIFrame methods are only useful for SVG as nobody else should have display:none frames I think an assertion in each that the content namespace is SVG is warranted.

(In reply to Cameron McCormack (:heycam) from comment #3) > I haven't looked at the issue properly yet. It's on the agenda for tomorrow > morning's telcon. At first glance, ignoring those display:none characters > seems inconsistent with having x="", dx="" etc. attributes work on just what > characters are in the content. Right. Also, since display:none can be set by a CSS stylesheet, it would be surprising to someone writing script that the return value of getNumberOfChars suddenly reports a different number because, e.g. a :hover selector on an unknown stylesheet matched and now all their indices are incorrect. I think the way it's defined in SVG now makes sense, it's just that most implementations don't do it that way at present. (In reply to Cameron McCormack (:heycam) from comment #4) > BTW it doesn't mean that we have to build frames for these display:none > <tspan>s, right? It just means we have to make sure we count the characters > in them so that the indices you pass in to getComputedTextLength etc. take > them into account. Right, there's no requirement to make frames. I just imagined that's probably the easiest way forward but I haven't looked into it so there may well be an easier/better way to juggle the numbers.

Splitting the previous patch out into two parts: * One which adds the convenience methods for getting displayed frames * One which creates frames for display:none glyphs but doesn't paint them I've split them out since depending on which direction this bug goes we might want one but not the other.

(In reply to Robert Longson from comment #6) > Since the nsIFrame methods are only useful for SVG as nobody else should > have display:none frames I think an assertion in each that the content > namespace is SVG is warranted. I started doing this but the assertion was failing. It seems the content of a glyph frame is a text node which isn't marked as being in the SVG namespace.

You could have something that checks for either IsSVG on the content or a text node: IsNodeOfType(nsINode::eTEXT). It may well be that the text node check could be removed once Cameron completes his text rework/glyph frame removal.

(In reply to Robert Longson from comment #11) > You could have something that checks for either IsSVG on the content or a > text node: IsNodeOfType(nsINode::eTEXT). It may well be that the text node > check could be removed once Cameron completes his text rework/glyph frame > removal. Good idea, I've added that to the patch now.

This time uploading after qrefresh.

Comment on attachment 582186 [details] [diff] [review] Create convenience methods for getting displayed frames v1c >+{ >+ nsIFrame* child = mFirstChild; >+ NS_ABORT_IF_FALSE( >+ child->GetContent()->GetNameSpaceID() == kNameSpaceID_SVG || Use child->GetContent()->IsSVG() instead >+ nsIFrame* GetNextDisplayedSibling() const { >+ NS_ABORT_IF_FALSE(GetContent()->GetNameSpaceID() == kNameSpaceID_SVG || IsSVG() here too.