Last Comment Bug 309924 - Removing <stop> elements from gradients is not live
: Removing <stop> elements from gradients is not live
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- normal (vote)
: ---
Assigned To: Scooter Morris
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-24 18:04 PDT by Jesse Ruderman
Modified: 2006-03-12 18:56 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase: pservers-grad-04-b.svg with a script that removes the yellow stops (2.74 KB, image/svg+xml)
2005-09-24 18:05 PDT, Jesse Ruderman
no flags Details
Change gradientframebase to nsSVGDefsFrame (2.57 KB, patch)
2005-09-25 16:53 PDT, Scooter Morris
no flags Details | Diff | Review
testcase for a crash that happens with that patch (318 bytes, image/svg+xml)
2005-09-25 19:11 PDT, Jesse Ruderman
no flags Details
Try nsSVGGenericContainer as a base class (2.30 KB, patch)
2005-09-26 18:34 PDT, Scooter Morris
no flags Details | Diff | Review
OK, this is better (3.10 KB, patch)
2005-10-01 15:38 PDT, Scooter Morris
no flags Details | Diff | Review
As before, but overload removeFrame so we can send a notification (3.98 KB, patch)
2005-10-01 22:44 PDT, Scooter Morris
no flags Details | Diff | Review
As before, including Jonathan's requests (4.18 KB, patch)
2005-10-02 17:52 PDT, Scooter Morris
jwatt: review+
asa: approval1.8b5+
Details | Diff | Review

Description Jesse Ruderman 2005-09-24 18:04:07 PDT
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 2005-09-24 18:05:13 PDT
Created attachment 197301 [details]
testcase: pservers-grad-04-b.svg with a script that removes the yellow stops
Comment 2 Scooter Morris 2005-09-25 16:53:57 PDT
Created attachment 197372 [details] [diff] [review]
Change gradientframebase to nsSVGDefsFrame
Comment 3 Jesse Ruderman 2005-09-25 17:12:37 PDT
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 2005-09-25 17:37:05 PDT
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 2005-09-25 18:30:16 PDT
(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 2005-09-25 18:43:29 PDT
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 2005-09-25 19:11:29 PDT
Created attachment 197378 [details]
testcase for a crash that happens with that patch
Comment 8 Scooter Morris 2005-09-26 18:33:47 PDT
(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 2005-09-26 18:34:30 PDT
Created attachment 197498 [details] [diff] [review]
Try nsSVGGenericContainer as a base class
Comment 10 Scooter Morris 2005-09-26 19:36:54 PDT
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 2005-10-01 15:38:26 PDT
Created attachment 198169 [details] [diff] [review]
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] 2005-10-01 20:34:25 PDT
(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 2005-10-01 22:17:37 PDT
(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 2005-10-01 22:44:48 PDT
Created attachment 198192 [details] [diff] [review]
As before, but overload removeFrame so we can send a notification
Comment 15 Jonathan Watt [:jwatt] 2005-10-02 07:25:01 PDT
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 2005-10-02 17:52:29 PDT
Created attachment 198258 [details] [diff] [review]
As before, including Jonathan's requests
Comment 17 Scooter Morris 2005-10-02 19:40:09 PDT
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] 2005-10-03 10:51:10 PDT
Comment on attachment 198258 [details] [diff] [review]
As before, including Jonathan's requests

last day for non-critical changes.

Note You need to log in before you can comment on or make changes to this bug.