Closed
Bug 309220
Opened 19 years ago
Closed 16 years ago
SVG markers should be live to id changes in document
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
VERIFIED
FIXED
People
(Reporter: tor, Assigned: longsonr)
References
Details
(Keywords: qawanted, verified1.9.1)
Attachments
(2 files, 2 obsolete files)
(deleted),
image/svg+xml
|
Details | |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•19 years ago
|
||
Is this the same issue as bug 329758 or something else?
Assignee | ||
Comment 2•17 years ago
|
||
I understand now, it's a different issue. The problem is that if you mess with the id of marker elements we don't track that.
Updated•17 years ago
|
Summary: SVG markers should be live → SVG markers should be live to id changes in document
I have infrastructure in my tree that will make this pretty easy to fix.
Assignee | ||
Comment 4•16 years ago
|
||
(In reply to comment #3)
> I have infrastructure in my tree that will make this pretty easy to fix.
>
Indeed.
Assignee: general → longsonr
Status: NEW → ASSIGNED
Attachment #334699 -
Flags: superreview?(roc)
Attachment #334699 -
Flags: review?(roc)
Assignee | ||
Comment 5•16 years ago
|
||
Assignee | ||
Comment 6•16 years ago
|
||
Oops, imagine I've also deleted these lines from nsSVGUtils.h too...
- #define NS_STATE_SVG_HAS_MARKERS 0x00200000
-
Sorry Robert, but it gets even easier to fix a few patches later in my svg-integration branch. I think you should make your patch against http://hg.mozilla.org/users/rocallahan_mozilla.com/svg-integration/, or just wait until trunk catches up enough. Revision d56beffbeb10 should contain everything you'd need:
http://hg.mozilla.org/users/rocallahan_mozilla.com/svg-integration/index.cgi/rev/d56beffbeb10
So on trunk we're live for ID changes for filter, mask, clip-path and use. My branch adds gradients and patterns. Here you do markers. Are there any other cases?
Assignee | ||
Comment 9•16 years ago
|
||
Just one that we currently implement: text-path.
Assignee | ||
Comment 10•16 years ago
|
||
Comment on attachment 334699 [details] [diff] [review]
patch
I guess I can wait
Attachment #334699 -
Flags: superreview?(roc)
Attachment #334699 -
Flags: review?(roc)
OK, now that bug 455984 has landed, we can fix this bug using nsSVGRenderingObservers.
Depends on: 455984
BTW is there a bug filed on making text-path live for ID changes? It's the last user of nsSVGUtils::GetReferencedFrame ... it would be nice to get rid of that function, since anything that uses it is basically wrong.
Assignee | ||
Comment 13•16 years ago
|
||
No, there is no bug for text-path.
We should do this for 1.9.1 since it's really required for incremental XML loading.
Flags: wanted1.9.1?
Assignee | ||
Comment 15•16 years ago
|
||
nsSVGPaintingProperty::DoUpdate has to change because marker changes (and eventually path changes in a text-path) can change the covered region of the object.
Attachment #334699 -
Attachment is obsolete: true
Attachment #341417 -
Flags: superreview?(roc)
Attachment #341417 -
Flags: review?(roc)
I think it would be best to subclass nsSVGPaintingProperty and override its DoUpdate to do the extra work for markers.
At some point we might want to optimize UpdateEffects. The marker, stroke and fill properties only need to be deleted for IsFrameOfType(eSVG) frames, and we could use SVG frame state bits to indicate whether those properties exist. But it probably doesn't matter yet.
+nsSVGMarkerFrame::AttributeChanged(PRInt32 aNameSpaceID,
+ nsIAtom* aAttribute,
+ PRInt32 aModType)
Fix indent of names
+ if (!(properties.MarkersExist()))
Just "if (!properties.MarkersExist())"
+ nsSVGPaintingProperty* mMarkerStart;
+ nsSVGPaintingProperty* mMarkerMid;
+ nsSVGPaintingProperty* mMarkerEnd;
Fix indent.
Otherwise, looks great. I'm really pleased with the way this approach to references is turning out.
Assignee | ||
Comment 17•16 years ago
|
||
(In reply to comment #16)
> I think it would be best to subclass nsSVGPaintingProperty and override its
> DoUpdate to do the extra work for markers.
>
fill and stroke would need to use the same override. If you change to fill or stroke="none" from something else or vice versa you can change the object bounds. Filter uses its own class anyway so the original code is correct only for mask and clipPath.
Perhaps therefore it is mask and clipPath which should have the optimised version. Any thoughts on a name? nsSVGFixedRegionPaintingProperty?
Assignee | ||
Comment 18•16 years ago
|
||
You probably remember this anyway but you did put in a check in UpdateAndInvalidateCoveredRegion that if the regions are the same then it only does the one invalidate so we are only doing a redundant UpdateCoveredRegion in the mask and clipPath case.
(In reply to comment #17)
> (In reply to comment #16)
> > I think it would be best to subclass nsSVGPaintingProperty and override its
> > DoUpdate to do the extra work for markers.
>
> fill and stroke would need to use the same override. If you change to fill or
> stroke="none" from something else or vice versa you can change the object
> bounds. Filter uses its own class anyway so the original code is correct only
> for mask and clipPath.
If the fill or stroke style changes, then we reach nsSVGPathGeometryFrame::DidSetStyleContext which calls nsSVGUtils::UpdateGraphic which does an UpdateAndInvalidateCoveredRegion. All nsSVGPaintingProperty needs to take care of is changes caused by the *referenced* content. I don't think paint servers can change the covered region of the path frame that references them.
Assignee | ||
Comment 20•16 years ago
|
||
In that case text-path should be able to use an nsSVGPaintingProperty too so markers are the odd one out.
Attachment #341417 -
Attachment is obsolete: true
Attachment #341426 -
Flags: superreview?(roc)
Attachment #341426 -
Flags: review?(roc)
Attachment #341417 -
Flags: superreview?(roc)
Attachment #341417 -
Flags: review?(roc)
Attachment #341426 -
Flags: superreview?(roc)
Attachment #341426 -
Flags: superreview+
Attachment #341426 -
Flags: review?(roc)
Attachment #341426 -
Flags: review+
Assignee | ||
Comment 21•16 years ago
|
||
checked in 0569b4a4b379
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Assignee | ||
Comment 22•16 years ago
|
||
checked in reftest http://hg.mozilla.org/mozilla-central/rev/4f32fd012f1a
Flags: in-testsuite? → in-testsuite+
Flags: wanted1.9.1? → wanted1.9.1+
Keywords: fixed1.9.1
Comment 23•16 years ago
|
||
Reftest covers the fix.
Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090123 Shiretoko/3.1b3pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•