Closed Bug 398825 Opened 17 years ago Closed 11 years ago

selectSubString not implemented

Categories

(Core :: SVG, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: arunsic, Assigned: longsonr)

References

Details

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1) ; .NET CLR 2.0.50727)
Build Identifier: 

I get the following message in error console:

Error: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOMSVGTextPositioningElement.selectSubString]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: 

Reproducible: Always

Steps to Reproduce:
1.
3.
Component: General → SVG
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
selectSubString is not implemented in any version of mozilla. 

This is documented in http://www.mozilla.org/projects/svg/status.html and http://developer.mozilla.org/en/docs/SVG_in_Firefox (for firefox 2.0)
So the mystery is not why doesn't it work in firefox 3 but why do you think it does work in firefox 2
(In reply to comment #2)
> So the mystery is not why doesn't it work in firefox 3 but why do you think it
> does work in firefox 2
> 

You are right. My mistake. selectSubString is not working in any versions of Gran Paradiso Alpha x.x. Sorry about the confusion. 

I wonder if it is scheduled to be fixed in future dated alpha version ?????
Summary: SVG - selectSubString broken in Mozilla 3.0 Alpha. Works in Mozilla 2.0 → SVG - selectSubString not working in Mozilla 3.0 (Gran Paradiso Alpha versions)
(In reply to comment #3)

No, Firefox 3.0 is feature complete at this point.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: SVG - selectSubString not working in Mozilla 3.0 (Gran Paradiso Alpha versions) → selectSubString not implemented
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #720273 - Flags: review?(dholbert)
Attached patch without extra file (obsolete) — Splinter Review
Attachment #720273 - Attachment is obsolete: true
Attachment #720273 - Flags: review?(dholbert)
Attachment #720274 - Flags: review?(dholbert)
This only implements selection if svg.text.css-frames.enabled is true.

It removes the non-functional selection code from nsSVGGlyphFrame.cpp

We pass the following with this patch:

http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/text-tselect-02-f.html

http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/text-tselect-03-f.html
OS: Windows XP → All
Hardware: x86 → All
Blocks: svg11tests
Depends on: svgtext
(In reply to Robert Longson from comment #7)
> It removes the non-functional selection code from nsSVGGlyphFrame.cpp

Is this code completely unused?  If so, could you split it into a separate patch?
Comment on attachment 720274 [details] [diff] [review]
without extra file

So -- this code is pretty much 100% new to me (since it's largely code that heycam just recently wrote).

I think heycam should probably look at it, or at least someone who's more familiar than I am with our text / selection APIs (and the security concerns or non-concerns involved).

Hence, canceling review & bumping myself to "feedback".

Here's my feedback:

>diff --git a/content/svg/content/src/SVGTextContentElement.cpp b/content/svg/content/src/SVGTextContentElement.cpp
>+void
>+SVGTextContentElement::SelectSubString(uint32_t charnum, uint32_t nchars, ErrorResult& rv)
>+{
>+  if (FrameIsSVGText()) {
>+    nsSVGTextFrame2* textFrame = GetSVGTextFrame();
>+    if (!textFrame)
>+      return;

This check seems redundant to me. Are there any circumstances where FrameIsSVGText() returns true, but GetSVGTextFrame() returns null?

That might be a question for heycam... In any case, this is probably fine for now, since the extra check doesn't harm and it matches code elsewhere in this file (but I wonder about that code as well).

If you agree that the GetSVGTextFrame null-check looks redundant there, maybe file a followup bug on cleaning those up, and CC me and heycam?

>+++ b/content/svg/content/src/SVGTextContentElement.h
>   int32_t GetNumberOfChars();
>   float GetComputedTextLength();
>+  void SelectSubString(uint32_t charnum, uint32_t nchars, ErrorResult& rv);
>   float GetSubStringLength(uint32_t charnum, uint32_t nchars, ErrorResult& rv);
>   already_AddRefed<nsISVGPoint> GetStartPositionOfChar(uint32_t charnum, ErrorResult& rv);
>   already_AddRefed<nsISVGPoint> GetEndPositionOfChar(uint32_t charnum, ErrorResult& rv);
>   already_AddRefed<nsIDOMSVGRect> GetExtentOfChar(uint32_t charnum, ErrorResult& rv);
>   float GetRotationOfChar(uint32_t charnum, ErrorResult& rv);
>   int32_t GetCharNumAtPosition(nsISVGPoint& point);

Perhaps SelectSubString should be at the end of this list -- after GetCharNumAtPosition() -- to match the WebIDL file (and the ordering at http://www.w3.org/TR/SVG/text.html#DOMInterfaces which I presume the WebIDL file is based on)...?

This applies to the positioning of the impl in the .cpp file, as well, and the positioning of the helper-function in nsSVGTextFrame2.h/cpp (for consistency).

I'm also fine with the current ordering, though... Setting aside the arbitrary ordering chosen in the SVG spec, I think it makes logical sense for this to be next to GetSubStringLength, where you've put it.

>diff --git a/layout/svg/nsSVGGlyphFrame.cpp b/layout/svg/nsSVGGlyphFrame.cpp
> NS_IMETHODIMP
>-nsSVGGlyphFrame::IsSelectable(bool* aIsSelectable,
>-                              uint8_t* aSelectStyle) const
[...]
>-
>-// Utilities for converting from indices in the uncompressed content
>-// element strings to compressed frame string and back:
>-static int
>-CompressIndex(int index, const nsTextFragment*fragment)
>-{
>-nsresult
>-nsSVGGlyphFrame::GetHighlight(uint32_t *charnum, uint32_t *nchars,
>-                              nscolor *foreground, nscolor *background)
>-{
[...]
>-}

(per previous comment, I'd prefer that this code-cleanup happen in a separate patch, either on this bug or on another bug.)

>diff --git a/layout/svg/nsSVGTextFrame2.cpp b/layout/svg/nsSVGTextFrame2.cpp
>+void
>+nsSVGTextFrame2::SelectSubString(nsIContent* aContent,
>+                                 uint32_t charnum, uint32_t nchars)
>+{
>+  UpdateGlyphPositioning(false);
>+
>+  nsIFrame* kid = GetFirstPrincipalChild();
>+  if (!kid) {
>+    return;
>+  }
>+
>+  // Convert charnum/nchars from addressable characters relative to
>+  // aContent to global character indices.
>+  CharIterator chit(this, CharIterator::eAddressable, aContent);
>+  if (!chit.AdvanceToSubtree() ||
>+      !chit.Next(charnum) ||
>+      chit.IsAfterSubtree() ||
>+      chit.AtEnd()) {
>+    return;
>+  }
>+  charnum = chit.TextElementCharIndex();
>+  chit.NextWithinSubtree(nchars);
>+  nchars = chit.TextElementCharIndex() - charnum;

A few things:
 (A) I think those first 19 lines are 100% identical to the first 19 lines of GetSubStringLength (aside from "return" vs "return 0.0"). It'd be great if we could do better code-sharing there... not sure if that's possible, though.

 (B)  RE the early-returns there -- if this frame method ever takes one of those early-returns (e.g. from Next(charnum) failing), that probably means we hit a situation where our content-node should be throwing NS_ERROR_DOM_INDEX_SIZE_ERR, right?  (But right now, your patch will have it just silently fail.) I suspect this method might need to return a bool indicating success/failure, so that its caller can throw if necessary.  (Its caller might even be able to just assume that it always succeeds, due to its own bounds-checks -- but if that's true, it assert that this always returns true, or something.)

 (C) Shouldn't nchars be the same before & after the NextWithinSubtree call?  Why are we bothering to re-set it? (I suppose it could differ if NextWithinSubtree fails to advance nchars characters and returns early -- but if that happens, I'd think we should fail out of this method as well.)

>+  const nsFrameSelection* frameSelection = GetConstFrameSelection();
>+
>+  if (frameSelection) {
>+    nsRefPtr<nsFrameSelection> fc = const_cast<nsFrameSelection*>(frameSelection);

Looks like nsIFrame::GetFrameSelection handles this const_cast ugliness for you:
  https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=a3d0d869bd50#5591

You probably just want to call that (and stick its result directly into a nsRefPtr), instead of calling GetConstFrameSelection.

>+    fc->HandleClick(chit.TextFrame()->GetContent(),
>+                    charnum, charnum + nchars, false, false, false);

I suspect we should possibly be passing "true" for the last arg there (assuming LTR text) -- the HandleClick documentation says "false" means we'll show the selection-caret at the left edge of the selection, "true" means we'll show it at the right edge.  Maybe the caret won't even be painted in this situation, though, in which case I suppose it doesn't matter.
Attachment #720274 - Flags: review?(dholbert) → feedback+
> So -- this code is pretty much 100% new to me (since it's largely code that heycam just recently wrote).

Me too! Someone other than him needs to understand it though.

Removal of old selection code moved to bug 848465
(In reply to Robert Longson from comment #10)
> > So -- this code is pretty much 100% new to me (since it's largely code that heycam just recently wrote).
> 
> Me too! Someone other than him needs to understand it though.

True.  But for review purposes -- I haven't worked directly w/ our text layout code (in SVG or html) much at all, and I think this should get review from someone who has -- e.g. roc or heycam. Or you, except that you wrote the patch :)
Attached patch updated patch (obsolete) — Splinter Review
Attachment #720274 - Attachment is obsolete: true
Attachment #723184 - Flags: review?(cam)
> >+{
> >+  if (FrameIsSVGText()) {
> >+    nsSVGTextFrame2* textFrame = GetSVGTextFrame();
> >+    if (!textFrame)
> >+      return;
> 
> This check seems redundant to me. Are there any circumstances where
> FrameIsSVGText() returns true, but GetSVGTextFrame() returns null?

No, but...

> If you agree that the GetSVGTextFrame null-check looks redundant there,
> maybe file a followup bug on cleaning those up, and CC me and heycam?

What we want to do eventually is rip out the 

  if (FrameIsSVGText()) {

line and the else code. At which point we'd need the other test. It's a simpler, less error prone process if we leave in the currently redundant check.

> 
>  (B)  RE the early-returns there -- if this frame method ever takes one of
> those early-returns (e.g. from Next(charnum) failing), that probably means
> we hit a situation where our content-node should be throwing
> NS_ERROR_DOM_INDEX_SIZE_ERR, right?  (But right now, your patch will have it
> just silently fail.) I suspect this method might need to return a bool
> indicating success/failure, so that its caller can throw if necessary.  (Its
> caller might even be able to just assume that it always succeeds, due to its
> own bounds-checks -- but if that's true, it assert that this always returns
> true, or something.)

I think we should assert but we need to ask heycam.

> 
>  (C) Shouldn't nchars be the same before & after the NextWithinSubtree call?
> Why are we bothering to re-set it? (I suppose it could differ if
> NextWithinSubtree fails to advance nchars characters and returns early --
> but if that happens, I'd think we should fail out of this method as well.)

We're converting it from being relative to content to relative to global character indices.

> 
> You probably just want to call that (and stick its result directly into a
> nsRefPtr), instead of calling GetConstFrameSelection.

OK.

> I suspect we should possibly be passing "true" for the last arg there
> (assuming LTR text) -- the HandleClick documentation says "false" means
> we'll show the selection-caret at the left edge of the selection, "true"
> means we'll show it at the right edge.  Maybe the caret won't even be
> painted in this situation, though, in which case I suppose it doesn't matter.

We don't pass true if we handle the selection using the mouse. Maybe heycam can tell me what's right.
(In reply to Robert Longson from comment #13)
> >  (C) Shouldn't nchars be the same before & after the NextWithinSubtree call?
> > Why are we bothering to re-set it? (I suppose it could differ if
> > NextWithinSubtree fails to advance nchars characters and returns early --
> > but if that happens, I'd think we should fail out of this method as well.)
> 
> We're converting it from being relative to content to relative to global
> character indices.

But nchars is a character-count (the number of characters we're selecting), so it shouldn't matter what we're indexing relative to, right?
(In reply to Robert Longson from comment #13)
> >  (B)  RE the early-returns there -- if this frame method ever takes one of
> > those early-returns (e.g. from Next(charnum) failing), that probably means
> > we hit a situation where our content-node should be throwing
> > NS_ERROR_DOM_INDEX_SIZE_ERR, right?  (But right now, your patch will have it
> > just silently fail.) I suspect this method might need to return a bool
> > indicating success/failure, so that its caller can throw if necessary.  (Its
> > caller might even be able to just assume that it always succeeds, due to its
> > own bounds-checks -- but if that's true, it assert that this always returns
> > true, or something.)
> 
> I think we should assert but we need to ask heycam.

Yes, the check for valid charnum/nchars in SVGTextContentElement::SelectSubString should prevent us from getting in here, so an assertion is warranted.

> > I suspect we should possibly be passing "true" for the last arg there
> > (assuming LTR text) -- the HandleClick documentation says "false" means
> > we'll show the selection-caret at the left edge of the selection, "true"
> > means we'll show it at the right edge.  Maybe the caret won't even be
> > painted in this situation, though, in which case I suppose it doesn't matter.

It won't be painted, but if you then press Shift+Left/Right you'll be able to tell where it is.

> We don't pass true if we handle the selection using the mouse. Maybe heycam
> can tell me what's right.

Where were you looking for the selection by the mouse call to HandleClick?  Anyway, I think it makes sense for the caret to end up at the end of the selection, as if you had selected with the mouse from the earlier characters to the later ones.  That'll be right for RTL text too.

(In reply to Daniel Holbert [:dholbert] from comment #14)
> But nchars is a character-count (the number of characters we're selecting),
> so it shouldn't matter what we're indexing relative to, right?

This is actually DOM characters, including non-displayed and white space collapsed characters, as opposed to "addressable" characters, which is what the SVG DOM methods should be using.  If you have:

  <text>   A   B   </text>

and call selectSubString(0, 2) it should select the A and the B.  The nchars would change from 2 to 5.

I think the SVG spec needs to make this more clear, though.  getNumberOfChars is actually defined to return the number of actual DOM characters (modulo <tref>, which we don't support), but I think this doesn't make sense given the positioning attributes like x="" work on addressable characters.  It's also not what implementations do.
Comment on attachment 723184 [details] [diff] [review]
updated patch

Review of attachment 723184 [details] [diff] [review]:
-----------------------------------------------------------------

The changes in SVGElementFactory.cpp aren't required for this bug, right?  Can you move that into a separate patch/bug?  I'd also like to see some tests, especially handling cases where the global character indices don't match up with the addressable ones (e.g. with some display:none <tspan>s, collapsed white space, etc.).
Attachment #723184 - Flags: review?(cam)
Could you explain how you thought I'd create tests? How do I create the ref for a reftest? Against a html equivalent?
Yes, or against a version where you use the DOM range/selection APIs to select the text, which should also work.  If you want to compare against HTML equivalents, you can follow for example:

https://hg.mozilla.org/mozilla-central/file/456cb08f8509/layout/reftests/svg/text/simple-selection.svg
https://hg.mozilla.org/mozilla-central/file/456cb08f8509/layout/reftests/svg/text/simple-selection-ref.html

but I think using SVG text and DOM range/selection in the reference would be better.
Attached patch patchSplinter Review
Attachment #731142 - Flags: review?
Attachment #731142 - Flags: review? → review?(cam)
Attachment #723184 - Attachment is obsolete: true
Comment on attachment 731142 [details] [diff] [review]
patch

Oh yeah, we do need to check that the nchars in GetSubStringLength is valid.  The tests look good too.
Attachment #731142 - Flags: review?(cam) → review+
Flags: in-testsuite+
(In reply to Robert Longson from comment #7)
> This only implements selection if svg.text.css-frames.enabled is true.

Why not pref it off in the webidl file? If you added [Pref="svg.text.css-frames.enabled"], that would allow feature detection.
Didn't know you could do that.
Attached patch like so?Splinter Review
Attachment #731809 - Flags: review?(Ms2ger)
Comment on attachment 731809 [details] [diff] [review]
like so?

Review of attachment 731809 [details] [diff] [review]:
-----------------------------------------------------------------

Like so, indeed. A test that |"selectSubString" in element| is false if the pref is off would be nice.
Attachment #731809 - Flags: review?(Ms2ger) → review+
https://hg.mozilla.org/mozilla-central/rev/bb9f6df94fe5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: