Closed Bug 842224 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: SVG, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached image test
It seems to return 0 always, with the attached test.
I think that ConvertTextElementCharIndexToAddressableIndex is wrong but I'm not sure how to fix it.
Blocks: 839955
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #731312 - Flags: review?(cam)
Attached file with test (obsolete) —
Attachment #731312 - Attachment is obsolete: true
Attachment #731312 - Flags: review?(cam)
Attachment #731458 - Flags: review?(cam)
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.
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.
(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.
Attachment #731458 - Attachment is obsolete: true
Attachment #731458 - Attachment is patch: false
Attachment #731458 - Flags: review?(cam)
Assignee: longsonr → nobody
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.
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.
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.
(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.
(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.
(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!
Attached patch patch v2Splinter Review
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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #737842 - Flags: review?(longsonr)
With this patch test_text.html and test_text_scaled.html pass.
Duplicate of this bug: 862159
Duplicate of this bug: 862161
Attachment #737842 - Flags: review?(longsonr) → review+
https://hg.mozilla.org/mozilla-central/rev/6258cfdfe2d9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.