Closed
Bug 309924
Opened 19 years ago
Closed 19 years ago
Removing <stop> elements from gradients is not live
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: scootermorris)
Details
(Keywords: fixed1.8)
Attachments
(3 files, 4 obsolete files)
2.74 KB,
image/svg+xml
|
Details | |
318 bytes,
image/svg+xml
|
Details | |
4.18 KB,
patch
|
jwatt
:
review+
asa
:
approval1.8b5+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Assignee | ||
Comment 2•19 years ago
|
||
Assignee: general → scootermorris
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•19 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•19 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•19 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•19 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•19 years ago
|
||
Assignee | ||
Comment 8•19 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•19 years ago
|
||
Attachment #197372 -
Attachment is obsolete: true
Assignee | ||
Comment 10•19 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•19 years ago
|
||
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•19 years ago
|
Attachment #198169 -
Flags: review?(jonathan.watt)
Comment 12•19 years ago
|
||
(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•19 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•19 years ago
|
Attachment #198169 -
Flags: review?(jonathan.watt)
Assignee | ||
Comment 14•19 years ago
|
||
Attachment #198169 -
Attachment is obsolete: true
Attachment #198192 -
Flags: review?(jonathan.watt)
Comment 15•19 years ago
|
||
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•19 years ago
|
||
Attachment #198192 -
Attachment is obsolete: true
Updated•19 years ago
|
Attachment #198258 -
Flags: review+
Updated•19 years ago
|
Attachment #198192 -
Flags: review+
Updated•19 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 17•19 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•19 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+
Reporter | ||
Updated•19 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.
Description
•