309924 - Removing <stop> elements from gradients is not live

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050924
Firefox/1.6a1

Gradients are live in some ways (e.g. changing the stop-color attribute of a
stop element, see bug 276316), but they are not live with respect to the removal
of <stop> elements.

Comment 1

[Jesse Ruderman]

Attachment: testcase: pservers-grad-04-b.svg with a script that removes the yellow stops

Comment 2

[Scooter Morris]

Attachment: Change gradientframebase to nsSVGDefsFrame

Comment 3

[Jesse Ruderman]

I'm trying the patch and sometimes Firefox doesn't repaint after the change. 
(Resizing the window makes it repaint correctly.)  I don't know whether that
means the patch is incomplete or whether that's a separate bug.

Comment 4

[Jesse Ruderman]

With this patch, I get lots of crashes at nsSVGDefsFrame::GetCanvasTM while
manipulating the DOM.  I noticed that nsSVGGradientFrame doesn't have its own
GetCanvasTM function and nsSVGDefsFrame assumes it has a parent (cf bug 308585).

Comment 5

[Scooter Morris]

(In reply to comment #4)
> With this patch, I get lots of crashes at nsSVGDefsFrame::GetCanvasTM while
> manipulating the DOM.  I noticed that nsSVGGradientFrame doesn't have its own
> GetCanvasTM function and nsSVGDefsFrame assumes it has a parent (cf bug 308585).

Hmmm...good point.  I probably need to use something like
nsSVGPatternFrame::GetCanvasTM to get the geometry parent of the gradient and
call its GetCanvasTM.

Comment 6

[Scooter Morris]

Jesse, can you provide a test case?  I have not been able to induce a crash with
any of my gradient test cases, but they don't do any DOM manipulations.  I
believe that a gradient must always have a parent, if only an OuterSVG frame.

Comment 7

[Jesse Ruderman]

Attachment: testcase for a crash that happens with that patch

Comment 8

[Scooter Morris]

(In reply to comment #7)
> Created an attachment (id=197378) [edit]
> crash testcase
> 

Jesse, thanks for the testcase!  It really helped.  Obviously, nsSVGDefsFrame is
the wrong base -- the appropriate base is nsSVGGenericContainerFrame.  I've made
that change and both testcases now pass.

Comment 9

[Scooter Morris]

Attachment: Try nsSVGGenericContainer as a base class

Comment 10

[Scooter Morris]

I should point out that the second test case passes, but, on a second look, I'm
not really happy with what it does.  My guess is that the <rect> should just be
ignored as a child of a <linearGradient>.  With this patch -- its actually
rendered, which I think is wrong, isn't it?

Comment 11

[Scooter Morris]

Attachment: OK, this is better

nsSVGGenericContainer is probably the right base class, but for gradients we
need to override nsSVGPaint since gradients act like display=none

Comment 12

[Jonathan Watt [:jwatt]]

(In reply to comment #10)
> I should point out that the second test case passes, but, on a second look, I'm
> not really happy with what it does.  My guess is that the <rect> should just be
> ignored as a child of a <linearGradient>.  With this patch -- its actually
> rendered, which I think is wrong, isn't it?

Yes, that would be wrong. Although you've solved that problem, the gradients
still aren't updated until you resize the screen or do something else to force a
repaint.

Comment 13

[Scooter Morris]

(In reply to comment #12)
> 
> Yes, that would be wrong. Although you've solved that problem, the gradients
> still aren't updated until you resize the screen or do something else to force a
> repaint.

Grrr..sorry, the test case had an alert which caused a repaint, so it looked
like things were dynamic.  OK, back to the drawing board.

Comment 14

[Scooter Morris]

Attachment: As before, but overload removeFrame so we can send a notification

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 198192 [details] [diff] [review]
As before, but overload removeFrame so we can send a notification

Great. This seems to be a good way to do this. r=me with a few requests.

>-#include "nsWeakReference.h"
...
>-#include "nsISVGValueObserver.h"

Can you leave these in?

>   // nsIFrame interface:
>   NS_IMETHOD DidSetStyleContext(nsPresContext* aPresContext);
>+  NS_IMETHOD RemoveFrame(nsIAtom*        aListName,
>+                         nsIFrame*       aOldFrame);
> 
>+  // nsISVGChildFrame interface:
>+  // Override PaintSVG (our frames don't directly render)
>+  NS_IMETHOD PaintSVG(nsISVGRendererCanvas* canvas,
>+                      const nsRect& dirtyRectTwips,
>+                      PRBool ignoreFilter) {return NS_OK;}
>+  

Can you put PaintSVG after GetType (below)? GetType is part of the nsIFrame
interface.

>   /**
>    * Get the "type" of the frame
>    *
>    * @see nsLayoutAtoms::svgGradientFrame
>    */
>   virtual nsIAtom* GetType() const;



> nsIAtom*
> nsSVGGradientFrame::GetType() const
> {
>   return nsLayoutAtoms::svgGradientFrame;
> }
> 
>+NS_IMETHODIMP
>+nsSVGGradientFrame::RemoveFrame(nsIAtom*        aListName,
>+                                nsIFrame*       aOldFrame)
>+{
>+  WillModify(mod_other);
>+  PRBool result = mFrames.DestroyFrame(GetPresContext(), aOldFrame);
>+  DidModify(mod_other);
>+  return result ? NS_OK : NS_ERROR_FAILURE;
>+}

Can you put the definition of RemoveFrame before the Definition of GetType to
keep the declaration and definition order the same?

With those changes please land on the trunk and request approval ASAP.

Comment 16

[Scooter Morris]

Attachment: As before, including Jonathan's requests

Comment 17

[Scooter Morris]

Comment on attachment 198258 [details] [diff] [review]
As before, including Jonathan's requests

This fixes the behaviour of gradients w.r.t. DOM scripting.  Risk of negatively
impacting other elements is low.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 198258 [details] [diff] [review]
As before, including Jonathan's requests

last day for non-critical changes.