339220 - Tracking bug for SVG on the 1.8 branch

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Mike Kaply [:mkaply]]

Rather than mess with the existing bugs, I'm going to attach patches here and seek approval here.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: 310996 - We shouldn't assert in nsSVGEnum::SetValueString (checked in)

Comment 2

[Mike Kaply [:mkaply]]

Attachment: 311031 - implement SVGTSpanElement.getComputedTextLength

DOM compliance

Comment 3

[Mike Kaply [:mkaply]]

Attachment: 311063 - Bogus assertions, and tspan attributes x/y/dx/dy aren't live (checked in)

DOM compliance issue, low risk.

Comment 4

[Mike Kaply [:mkaply]]

Attachment: 312269 - marker leaves poopies along the 4 cardinal directions (on my machine anyway) (checked in)

Low risk - visual bug

Comment 5

[Mike Kaply [:mkaply]]

Attachment: 312376 - Repair textPath from bug 311063 (checked in)

Fallout from above checkin for 311063

Comment 6

[Mike Kaply [:mkaply]]

Attachment: 315861 - SVG Mouse events inhibited by clippath in proximity (checked in)

Low risk bug fix

Comment 7

[Mike Kaply [:mkaply]]

Attachment: 317709 - onclick does not work with textPath SVG elements (checked in)

Low risk fix

Comment 8

[Mike Kaply [:mkaply]]

Attachment: 318379 - Firefox 1.5 should not crash when viewing an SVG file, or a page embedding that file (checked in)

Crash fix

Comment 9

[Mike Kaply [:mkaply]]

Attachment: 320623 - overflow="visible" doesn't work on <marker> (checked in)

Correctness fix

Comment 10

[Mike Kaply [:mkaply]]

Attachment: 322922 - We have interface map entries for nsSupportsWeakReference! (checked in)

This was just bad code

Comment 11

[Mike Kaply [:mkaply]]

Attachment: 317027 - Classinfo is incorrect for some SVG nodes (checked in)

Correctness

Comment 12

[Mike Kaply [:mkaply]]

Attachment: 323589 - Trying to access .style of <svg:marker> throws "Component does not have requested interface" (NS_NOINTERFACE) (checked in)

Correctness fix

Comment 13

[Mike Kaply [:mkaply]]

Attachment: 323669 - Percent attribute values in userSpace gradients don't work (checked in)

bug

Comment 14

[Mike Kaply [:mkaply]]

Attachment: 323704 - Circular reference in 'clipPath' crashes browser instantly (checked in)

Recursive loop crash

Comment 15

[Mike Kaply [:mkaply]]

Attachment: 323753 - initialise meetOrSlice variable properly (checked in)

wrong variable initialized/set

Comment 16

[Mike Kaply [:mkaply]]

Attachment: 327437 - createSVGAngle isn't implemented (checked in)

correctness

Comment 17

[Mike Kaply [:mkaply]]

Attachment: 328137 - "ASSERTION: not a container" making one <svg:stop> a child of another (checked in)

fix assertion

Comment 18

[Mike Kaply [:mkaply]]

Attachment: 325728 - changes to markerWidth/markerHeight and orient not handled properly (checked in)

correctness.

Comment 19

[Takeshi Kurosawa :takenspc]

How about bug 318597.
IMHO, It may help svg widget.

Comment 20

[Mike Kaply [:mkaply]]

(In reply to comment #19)
> How about bug 318597.
> IMHO, It may help svg widget.
> 

That bug is on my list. Because it is a large patch, I'm waiting until some other stuff gets in. Here are the other bugs we are considering:

bug 310436
bug 314627
bug 318597
bug 329758
bug 336451

Comment 21

[Robert Longson [:longsonr]]

bug 318597 has a problem c.f. bug 339039

bug 314627 is a bit of a bodge. Most of its code was rewritten in bug 318597. It was one of my earliest efforts. It makes nsTSpanElement an nsGraphicElement which it really shouldn't be. bug 318597 reverses this.

Comment 22

[Mike Kaply [:mkaply]]

(In reply to comment #21)
> bug 318597 has a problem c.f. bug 339039
> 
> bug 314627 is a bit of a bodge. Most of its code was rewritten in bug 318597.
> It was one of my earliest efforts. It makes nsTSpanElement an nsGraphicElement
> which it really shouldn't be. bug 318597 reverses this.
> 

Obviously, since you are the creator of these patches, I would defer to you for the right thing to do.

What do you think we should do with these three bugs on the 1.8 branch?

Comment 23

[Robert Longson [:longsonr]]

I am working on patch for bug 339039. That should be ready for review Tue/Wed next week.

I would recommend either taking all of  bug 314627, bug 318597 and bug 339039 or none. This depends on the timescales as bug 339039 would require some baking time.

Comment 24

[Mike Kaply [:mkaply]]

Shaver - this is the list of SVG things for 1.8

Comment 25

[Mike Shaver (:shaver emeritus)]

This looks fantastic!

Comment 26

[Robert Longson [:longsonr]]

(In reply to comment #22)
> (In reply to comment #21)
> > bug 318597 has a problem c.f. bug 339039
> > 
> > bug 314627 is a bit of a bodge. Most of its code was rewritten in bug 318597.
> > It was one of my earliest efforts. It makes nsTSpanElement an nsGraphicElement
> > which it really shouldn't be. bug 318597 reverses this.
> > 
> 
> Obviously, since you are the creator of these patches, I would defer to you for
> the right thing to do.
> 
> What do you think we should do with these three bugs on the 1.8 branch?
> 

I have now checked in bug 339039 which fixes issues with bug 318597.

Your mileage may vary but the trunk has had a number of SVG infrastructure changes and integrating bugs 318597 and bug 339039 into a branch will not be straightforward as they are both large and therefore undoubtedly depend on various other trunk fixes. I have tried to update the dependencies of these bugs with the things I know about but that's bound to be incomplete.

If I were in your position, I'd probably reluctantly omit bug 314627, bug 318597 and bug 339039 for the above reason. I remember Tor's list of patches he integrated for textPath on the branch and that only just missed the cut for FF 1.5.

Having said that, I'd certainly like to see this functionality appear before FF 3 so if you have the time and inclination I'll try to help if I can.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 223294 [details] [diff] [review]
311031 - implement SVGTSpanElement.getComputedTextLength

This code was changed significantly in bug 318597.  Is this just a dummy implementation or is it really something close enough to what we have on the trunk that it's ok that authors depend on the difference?

Comment 28

[Mike Kaply [:mkaply]]

clickable view

bug 310996 - We shouldn't assert in nsSVGEnum::SetValueString
bug 311031 - implement SVGTSpanElement.getComputedTextLength
bug 311063 - Bogus assertions, and tspan attributes x/y/dx/dy aren't live
bug 312269 - marker leaves poopies along the 4 cardinal directions (on my machine anyway)
bug 312376 - Repair textPath from bug 311063
bug 315861 - SVG Mouse events inhibited by clippath in proximity
bug 317709 - onclick does not work with textPath SVG elements
bug 318379 - Firefox 1.5 should not crash when viewing an SVG file, or a page embedding that file
bug 320623 - overflow="visible" doesn't work on <marker>
bug 322922 - We have interface map entries for nsSupportsWeakReference!
bug 317027 - Classinfo is incorrect for some SVG nodes
bug 323589 - Trying to access .style of <svg:marker> throws 'Component does not have requested interface' (NS_NOINTERFACE)
bug 323669 - Percent attribute values in userSpace gradients don't work
bug 323704 - Circular reference in 'clipPath' crashes browser instantly
bug 323753 - initialise meetOrSlice variable properly
bug 327437 - createSVGAngle isn't implemented
bug 328137 - "ASSERTION: not a container" making one <svg:stop> a child of another<
bug 325728 - changes to markerWidth/markerHeight and orient not handled properly

Comment 29

[Darin Fisher]

Comment on attachment 223297 [details] [diff] [review]
312269 - marker leaves poopies along the 4 cardinal directions (on my machine anyway) (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 30

[Darin Fisher]

Comment on attachment 223299 [details] [diff] [review]
315861 - SVG Mouse events inhibited by clippath in proximity (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 31

[Darin Fisher]

Comment on attachment 223300 [details] [diff] [review]
317709 - onclick does not work with textPath SVG elements (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 32

[Darin Fisher]

Comment on attachment 223301 [details] [diff] [review]
318379 - Firefox 1.5 should not crash when viewing an SVG file, or a page embedding that file (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 33

[Darin Fisher]

Comment on attachment 223302 [details] [diff] [review]
320623 - overflow="visible" doesn't work on <marker> (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 34

[Darin Fisher]

Comment on attachment 223303 [details] [diff] [review]
322922 - We have interface map entries for nsSupportsWeakReference! (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 35

[Darin Fisher]

Comment on attachment 223306 [details] [diff] [review]
317027 - Classinfo is incorrect for some SVG nodes (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 36

[Darin Fisher]

Comment on attachment 223307 [details] [diff] [review]
323589 - Trying to access .style of <svg:marker> throws "Component does not have requested interface" (NS_NOINTERFACE) (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 37

[Darin Fisher]

Comment on attachment 223308 [details] [diff] [review]
323669 - Percent attribute values in userSpace gradients don't work (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 38

[Darin Fisher]

Comment on attachment 223309 [details] [diff] [review]
323704 - Circular reference in 'clipPath' crashes browser instantly (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 39

[Darin Fisher]

Comment on attachment 223310 [details] [diff] [review]
323753 - initialise meetOrSlice variable properly (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 40

[Darin Fisher]

Comment on attachment 223311 [details] [diff] [review]
327437 - createSVGAngle isn't implemented (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 41

[Darin Fisher]

Comment on attachment 223314 [details] [diff] [review]
328137 - "ASSERTION: not a container" making one <svg:stop> a child of another (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 42

[Darin Fisher]

Comment on attachment 223323 [details] [diff] [review]
325728 - changes to markerWidth/markerHeight and orient not handled properly (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 43

[Darin Fisher]

Comment on attachment 223296 [details] [diff] [review]
311063 - Bogus assertions, and tspan attributes x/y/dx/dy aren't live (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 44

[Darin Fisher]

Comment on attachment 223298 [details] [diff] [review]
312376 - Repair textPath from bug 311063 (checked in)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)

Comment 45

[Robert Longson [:longsonr]]

(In reply to comment #27)
> (From update of attachment 223294 [details] [diff] [review] [edit])
> This code was changed significantly in bug 318597.  Is this just a dummy
> implementation or is it really something close enough to what we have on the
> trunk that it's ok that authors depend on the difference?
> 

It's a real implementation but it has a side effect that I eventually realised was undesirable, it changed tspan elements from being inherited from nsSVGStyleableElement to nsSVGGraphicalElement in order to use a method in that class. Unfortunately nsSVGGraphicalElements should be transformable and tspans are not. This was reverted as part of bug 318597.

As far as the different implementation goes, the trunk implementation should work with non rtl text while the branch implementation does not. This does not seem to have been a big problem as that's how text elements work on the branch currently.

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #45)
> It's a real implementation but it has a side effect that I eventually realised
> was undesirable,

Is it undesirable in the sense that it causes problems (including problems handling unexpected things, which could be potential security problems), or just undesirable from a code maintainance perspective?

Comment 47

[Robert Longson [:longsonr]]

(In reply to comment #46)
> (In reply to comment #45)
> > It's a real implementation but it has a side effect that I eventually realised
> > was undesirable,
> 
> Is it undesirable in the sense that it causes problems (including problems
> handling unexpected things, which could be potential security problems), or
> just undesirable from a code maintainance perspective?
> 

As far as I know the fix does not have a security problem. 

It's probably not a big issue if you take this and don't take the other bigger fixes that rewrite it. On the branch it goes wrong in the same way that it goes wrong for text elements and that does not seem to have been something anyone has reported. 

After revisiting the code I don't think a user is affected by the element being transformable, the frame isn't so it should still draw the same.

Comment 48

[Mike Kaply [:mkaply]]

Attachment: implement SVGTSpanElement.getComputedTextLength

This version of SVGTSpanElement.getComputedTextLength does not change the parent class.

Comment 49

[Robert Longson [:longsonr]]

(In reply to comment #48)
> Created an attachment (id=227281) [edit]
> implement SVGTSpanElement.getComputedTextLength
> 
> This version of SVGTSpanElement.getComputedTextLength does not change the
> parent class.
> 

This would address my concerns about inheritance, although for what its worth we removed all the NS_ASSERTION calls in this area under bug 339807.

You might also want to consider adding the IsEventName code to nsSVGTSpanElement.cpp similar to attachment (id=223300). Otherwise text and textPath will respond to events but tspan will not. The trunk has this fix already as part of bug 318597.

Comment 50

[tor]

Comment on attachment 227281 [details] [diff] [review]
implement SVGTSpanElement.getComputedTextLength

> NS_IMETHODIMP nsSVGTSpanElement::GetComputedTextLength(float *_retval)
> {
>-  NS_NOTYETIMPLEMENTED("nsSVGTSpanElement::GetComputedTextLength");
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+//  nsIDOMSVGRect* bbox;
>+  nsCOMPtr<nsIDOMSVGRect> bbox;
>+
>+//  GetBBox(getter_AddRefs(bbox));

You can remove the commented lines.

>+  if (frame) {
>+    nsISVGChildFrame* svgframe;
>+    frame->QueryInterface(NS_GET_IID(nsISVGChildFrame),(void**)&svgframe);

While CallQueryInterface would be more appropriate, since this is essentially a clone of nsSVGGraphicsElement::GetBBox and that uses QI, we should probably leave it.

With the former change, r=tor.

Comment 51

[Mike Kaply [:mkaply]]

Comment on attachment 227281 [details] [diff] [review]
implement SVGTSpanElement.getComputedTextLength

I realize this isn't blocking, but I believe this is the last SVG change we'll take for 1.8

Comment 52

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 227281 [details] [diff] [review]
implement SVGTSpanElement.getComputedTextLength

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have.

Comment 53

[Mike Kaply [:mkaply]]

We're going to mark this bug done.

There are a few other individual issues we'll handle in the bugs.