842224 - getCharNumAtPosition return values wrong with svg.text.css-frames.enabled

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: test

It seems to return 0 always, with the attached test.

Comment 1

[Robert Longson [:longsonr]]

I think that ConvertTextElementCharIndexToAddressableIndex is wrong but I'm not sure how to fix it.

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Comment 3

[Robert Longson [:longsonr]]

Attachment: with test

Comment 4

[Cameron McCormack (:heycam)]

This doesn't seem to make attachment 715039 [details] work.  It shows 0 for both the "a" and "b", and 6 for the two leftmost Hebrew characters.  If I replace the content of the <text> element with "abc", even without any white space, it still shows 0 for the "a" and "b".  *checks*  But you know I think this is because I'm on a 2 dev px per CSS pixel machine.  I switched layout.css.devPixelsPerPx to 1 and get the correct results.

However, I still don't get the correct results with a test like:

  <text>abc<tspan display="none">def</tspan>ghi</text>

where the "g" is reported as 3 when it should be 6.

Comment 5

[Robert Longson [:longsonr]]

That's wouldn't be consistent with what you implemented in bug 842119 comment 5 though, nor would it be consistent with http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/text-tselect-02-f.html

bug 842119 changed things so that positioning works with uncompressed, unrendered characters but most text DOM methods now index only rendered characters.

Given

<text>abc<tspan display="none">def</tspan>ghi</text>
<text>abcghi</text>

Are you saying that the two text elements above should return different values for getComputedTextLength? They certainly look like they have the same lenth to the user.

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #5)
> bug 842119 changed things so that positioning works with uncompressed,
> unrendered characters but most text DOM methods now index only rendered
> characters.

Not rendered characters, but characters that you would normally be able to address with the positioning attributes.  Which means all characters in the DOM after collapsing white space, regardless of display:none.

But if my patch in bug 842119 made getSubStringLength include any display:none text, then that's a mistake.

> Given
> 
> <text>abc<tspan display="none">def</tspan>ghi</text>
> <text>abcghi</text>
> 
> Are you saying that the two text elements above should return different
> values for getComputedTextLength? They certainly look like they have the
> same lenth to the user.

No, those should return the same length.  But I think what we're after here is just consistency in how character indices are treated.  So it's fine that getSubStringLength and getComputedTextLength don't include any display:none <tspan>s when accumulating the lengths, but the index/length passed into getSubStringLength, as with the other SVG DOM methods that take an index/length, should be able to address display:none <tspan>s.

Basically I wanted to minimise the effect of styling properties on how character indexes are interpreted by these SVG DOM methods.  Unfortunately white-space needed to influence this, since authors seem to write things like

  <text>
    a b
  </text>

often and think of it as 3 characters.

Comment 7

[Robert Longson [:longsonr]]

Changing to 

      result = index + run.mTextElementCharIndex;

And keeping ConvertTextElementCharIndexToAddressableIndex seems to work pretty much the same as my patch i.e. display:none is ignored. Unfortunately the CharIterator skips undisplayed characters so we don't increment result enough. 

I don't know how you envision distinguishing between display: none characters and collapsed whitespace within the current iterator model that you have.

Comment 8

[Robert Longson [:longsonr]]

BTW selectSubString will currently ignore display:none characters. I think I'll need to implement it using the DOM Range API if we want it not to do that. Probably needs this bug fixing first so I can understand how to tell the difference between compressed whitespace and display:none.

Comment 9

[Robert Longson [:longsonr]]

Attachment: interactive testcase for substringlength

Adding some display:none text to the DOM shouldn't make any difference to the total length but should it change the addressing at all e.g. if we have

<text>a<tspan display="none">foo</tspan>bc</text>

and we ask for subStringLength(0, 2) that should be length of "af" or the length of "afoob"? Looks to me like we're getting the latter at the moment.

Comment 10

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #7)
> I don't know how you envision distinguishing between display: none
> characters and collapsed whitespace within the current iterator model that
> you have.

Maybe CharIterator needs to track the addresable index in addition to mTextElementCharIndex, which is the original index.  And then we can store that on the TextRenderedRuns we create.

CharIterator::IsOriginalCharAddressable() does the check we want.  We could call that from within NextCharacter() see whether we should increment an mTextElementAddressableCharIndex.

(These variable names are confusing even me.  It might be good to consistently name things in terms of "original" and "addressable", rather than just "char index".)

I do have a slight concern that we are doing a bunch of work inside CharIterator, which we use a lot, and not all of that work is necessary for each use of the iterator.  Still, I haven't done any profiling to see if it's a real problem.

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #8)
> BTW selectSubString will currently ignore display:none characters. I think
> I'll need to implement it using the DOM Range API if we want it not to do
> that. Probably needs this bug fixing first so I can understand how to tell
> the difference between compressed whitespace and display:none.

If DOM Range can actually set selections in the middle of display:none elements, then that sounds like a good idea.

(In reply to Robert Longson from comment #9)
> Adding some display:none text to the DOM shouldn't make any difference to
> the total length but should it change the addressing at all e.g. if we have
> 
> <text>a<tspan display="none">foo</tspan>bc</text>
> 
> and we ask for subStringLength(0, 2) that should be length of "af" or the
> length of "afoob"? Looks to me like we're getting the latter at the moment.

I think it should actually return the advance of "a" only; so, it addresses "af" but since the "f" is display:none it doesn't get included.  We can't get the advances of characters for which we haven't built text frames/runs, unless we go and build text runs manually ourselves, which I think we should avoid.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Cameron McCormack (:heycam) from comment #11)
> If DOM Range can actually set selections in the middle of display:none
> elements, then that sounds like a good idea.

It certainly can!

Comment 13

[Cameron McCormack (:heycam)]

Attachment: patch v2

I think this does it.  This iterates over "original" characters and notes when we are at an "addressable" character, keeping a track of the input and result values each time through the loop.  Not sure if this is the best way to do it, or if CharIterator should be able to tell you what the current addressable index is, but this seemed easier for now.

Comment 14

[Cameron McCormack (:heycam)]

With this patch test_text.html and test_text_scaled.html pass.

Comment 17

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6258cfdfe2d9

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6258cfdfe2d9