The default bug view has changed. See this FAQ.

Removing <stop> elements from gradients is not live

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: Scooter Morris)

Tracking

({fixed1.8})

Trunk
PowerPC
Mac OS X
fixed1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 197301 [details]
testcase: pservers-grad-04-b.svg with a script that removes the yellow stops
(Assignee)

Comment 2

12 years ago
Created attachment 197372 [details] [diff] [review]
Change gradientframebase to nsSVGDefsFrame
Assignee: general → scootermorris
Status: NEW → ASSIGNED
(Reporter)

Comment 3

12 years ago
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.
(Reporter)

Comment 4

12 years ago
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).
(Assignee)

Comment 5

12 years ago
(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.
(Assignee)

Comment 6

12 years ago
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.
(Reporter)

Comment 7

12 years ago
Created attachment 197378 [details]
testcase for a crash that happens with that patch
(Assignee)

Comment 8

12 years ago
(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.
(Assignee)

Comment 9

12 years ago
Created attachment 197498 [details] [diff] [review]
Try nsSVGGenericContainer as a base class
Attachment #197372 - Attachment is obsolete: true
(Assignee)

Comment 10

12 years ago
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?
(Assignee)

Comment 11

12 years ago
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
Attachment #197498 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
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.
(Assignee)

Comment 13

12 years ago
(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.
(Assignee)

Updated

12 years ago
Attachment #198169 - Flags: review?(jonathan.watt)
(Assignee)

Comment 14

12 years ago
Created attachment 198192 [details] [diff] [review]
As before, but overload removeFrame so we can send a notification
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+
(Assignee)

Comment 16

12 years ago
Created attachment 198258 [details] [diff] [review]
As before, including Jonathan's requests
Attachment #198192 - Attachment is obsolete: true

Updated

12 years ago
Attachment #198258 - Flags: review+

Updated

12 years ago
Attachment #198192 - Flags: review+

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

12 years ago
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 18

12 years ago
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+
(Assignee)

Updated

12 years ago
Keywords: fixed1.8
(Reporter)

Updated

12 years ago
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.