Closed Bug 309924 Opened 16 years ago Closed 16 years ago

Removing <stop> elements from gradients is not live

Categories

(Core :: SVG, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: scootermorris)

Details

(Keywords: fixed1.8)

Attachments

(3 files, 4 obsolete files)

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.
Assignee: general → scootermorris
Status: NEW → ASSIGNED
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.
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).
(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.
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.
(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.
Attachment #197372 - Attachment is obsolete: true
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?
Attached patch OK, this is better (obsolete) — Splinter Review
nsSVGGenericContainer is probably the right base class, but for gradients we
need to override nsSVGPaint since gradients act like display=none
Attachment #197498 - Attachment is obsolete: true
Attachment #198169 - Flags: review?(jonathan.watt)
(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.
(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.
Attachment #198169 - Flags: review?(jonathan.watt)
Attachment #198169 - Attachment is obsolete: true
Attachment #198192 - Flags: review?(jonathan.watt)
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.
Attachment #198192 - Flags: review?(jonathan.watt) → review+
Attachment #198192 - Attachment is obsolete: true
Attachment #198258 - Flags: review+
Attachment #198192 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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.
Attachment #198258 - Flags: approval1.8b5?
Comment on attachment 198258 [details] [diff] [review]
As before, including Jonathan's requests

last day for non-critical changes.
Attachment #198258 - Flags: approval1.8b5? → approval1.8b5+
Keywords: fixed1.8
Attachment #197378 - Attachment description: crash testcase → testcase for a crash that happens with that patch
You need to log in before you can comment on or make changes to this bug.