Closed
Bug 550047
Opened 14 years ago
Closed 13 years ago
Removing 'x' attribute from foreignObject doesn't move it
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: jruderman, Assigned: longsonr)
References
Details
(Keywords: regression, testcase)
Attachments
(3 files, 2 obsolete files)
No description provided.
Reporter | ||
Comment 1•14 years ago
|
||
Assignee | ||
Comment 2•14 years ago
|
||
nsSVGElementBase::UnsetAttr now runs before the data is reparsed and when we get to the nsSVGForeignObject::AttributeChanged we read the old values from the nsSVGForeignObjectElement rather than the new values.
Comment 3•14 years ago
|
||
Hmm. Is the AttributeChanged reading from some back-door data structures instead of reading from the attribute? That's what bug 534526 comment 12 said wasn't supposed to happen.
Assignee | ||
Comment 4•14 years ago
|
||
Yes, it doesn't read the attribute it reads the animated length value. The code in nsSVGElementBase::UnsetAttr updates that, but that now happens after the base class method is called. That comment was incorrect I guess.
Comment 5•14 years ago
|
||
> Yes, it doesn't read the attribute it reads the animated length value.
Can the method that returns this value not do so if the attribute is not set? Or can this code get the value from the attribute instead of whatever backdoor it uses now? Or are there cases when the animated value is set but the attribute is not set?
Or do I just need to slow down UnsetAttr on all elements to work around this SVG stuff? :(
Comment 6•14 years ago
|
||
Also, are there other SVG frames that rely on these sorts of backdoors?
Comment 7•14 years ago
|
||
Also, where is this getting of animated values happening? I assume it's somewhere under UpdateGraphic(), but I can't quite find where.
Comment 8•14 years ago
|
||
Under UpdateCoveredRegion?
Assignee | ||
Comment 9•14 years ago
|
||
I think that you can animate unset attributes. jwatt or dholbert could confirm. This applies to pretty much all SVG frames. Certainly anything that implements AttributeChanged (which is most of them).
Comment 10•14 years ago
|
||
> I think that you can animate unset attributes.
Then how can the code in nsSVGElementBase::UnsetAttr possibly be correct?
Comment 11•14 years ago
|
||
Er, I meant in nsSVGElement::UnsetAttr.
Comment 12•14 years ago
|
||
Or is it that this code changes the base value, not the animated one, and that the AfterSetAttr call to get the animated value actually gets the base value because we're not actually animating?
Assignee | ||
Comment 13•14 years ago
|
||
nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion calls svgFrame->UpdateCoveredRegion and that reads off the values via a call to GetAnimatedLengthValues. nsSVGElementBase::UnsetAttr was written before animation was implemented and has not been updated since.
Comment 14•14 years ago
|
||
Ok, one more question. Can someone actually describe to me precisely the assumptions and invariants involved in attr changes on SVG elements, so I can evaluate how they differ from all the other attr changes in Gecko and try to figure out how to align the two?
Assignee | ||
Comment 15•14 years ago
|
||
comment 12 is correct.
Comment 16•14 years ago
|
||
In particular, it seems that SVG assumes that AfterSetAttr means some data structures other than the attr value have already been updated. Which exact ones, and why is this a reasonable assumption to be making? Do the values of those data structures have to be read synchronously from AfterSetAttr, or would doing it either async or from a script runner be ok?
Assignee | ||
Comment 17•14 years ago
|
||
Async wouldn't work because you can read them from a script so if you unset one and then checked the value in javascript you'd get the wrong value. All the data structures are held on the elements. They are things like Length2 arrays but other data types are possible.
Comment 18•14 years ago
|
||
> Async wouldn't work because you can read them from a script I didn't say an async _update_ of the values. I said an async _reading_ of the values. That is, can whatever work it is all the SVG AfterSetAttr implementations do be done asynchronously? Is the second paragraph of comment 17 intended to be an answer to comment 14?
Assignee | ||
Comment 19•14 years ago
|
||
(In reply to comment #18) > > Async wouldn't work because you can read them from a script > > I didn't say an async _update_ of the values. I said an async _reading_ of the > values. That is, can whatever work it is all the SVG AfterSetAttr > implementations do be done asynchronously? Yes, async reading would be OK as all we're doing is redrawing. > > Is the second paragraph of comment 17 intended to be an answer to comment 14? Sort of. Can you talk to Daniel? You are in the same location aren't you?
Comment 20•14 years ago
|
||
> Yes, async reading would be OK as all we're doing is redrawing. In that case, is it worth fixing this bug by doing that? It's a bit more work than adding another virtual callback to UnsetAttrm but would have the salutary benefit of only redrawing once even if multiple changes are made to the dom, right? The caveat is that I'll probably need some help with making the async thing happen. I don't understand what all the invariants in this code are or what some of it is trying to do. > You are in the same location aren't you? We're in the same country. That's about the most that can be said of our locations being "same". The only way he could be further from me is if he moved to Alaska or Hawaii. ;)
Assignee | ||
Comment 21•14 years ago
|
||
(In reply to comment #20) > > Yes, async reading would be OK as all we're doing is redrawing. > > In that case, is it worth fixing this bug by doing that? It's a bit more work > than adding another virtual callback to UnsetAttrm but would have the salutary > benefit of only redrawing once even if multiple changes are made to the dom, > right? Quite possibly. I wonder if we could just break up nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion? The first Invalidate would stay and the rest of the method (updating the covered region etc) would happen later. We might need to lose the oldRegion == newRegion test though. roc?
Losing the oldRegion == newRegion test is OK. Let's do that.
Comment 23•14 years ago
|
||
(In reply to comment #9) > I think that you can animate unset attributes. jwatt or dholbert could confirm. Correct. For mapped attributes (patch in bug 534028, awaiting review), we store the animated values in a completely different place, so it doesn't matter whether the underlying value is set or not. For un-mapped attributes, we generally have a struct that stores both base & animated value, even if the attribute is unset (accessible via e.g. elem.x.baseVal.value & elem.x.animVal.value). If the attribute isn't set, this struct has the base value set to whatever the default-value is. For example, if the "x" attribute isn't set, elem.getAttribute("x") returns null, but elem.x.baseVal.value is 0.
Assignee | ||
Comment 24•14 years ago
|
||
We only have a problem with unmapped attributes i.e. x, y, width, height and transform I think.
Assignee | ||
Comment 25•14 years ago
|
||
After some thought, I believe we could just make the whole of nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion async pretty much as is. That would be more efficient too as we'd invalidate fewer times as Boris said. I think that would fix everything.
I agree.
Hmm, actually I'm not sure. When we paint, will we use the new values and possibly paint outside the stored covered region? Then we might have an attribute change, paint outside the stored covered region, have another attribute change, then handle the async UpdateAndInvalidateCoveredRegion and find that both the old and new covered regions don't cover what was painted.
Assignee | ||
Comment 28•14 years ago
|
||
We could clip painting to the covered region (if we don't already do that). That way we'd fix up everything when we did the async invalidate and region update.
Assignee | ||
Comment 29•14 years ago
|
||
Or just suppress painting by marking the covered region dirty once an attribute changes until the async invalidate occurs. We already have dirty flags and code to handle not painting.
OK
But won't that cause flicker, as the object disappears temporarily?
Assignee | ||
Comment 32•14 years ago
|
||
It wouldn't disappear, it would just stay in the old place. Invalidation/clearing would happen in the async code.
But what if something else causes a paint in the meantime?
Assignee | ||
Comment 34•14 years ago
|
||
Could we suppress that? Or if not, clip it to the region?
Comment 35•14 years ago
|
||
I was assuming that "async" would mean that we post a restyle event (possibly with a new restyle hint if none of the existing ones do the right thing) to do the actual work. So painting would flush any pending updates.
Assignee | ||
Comment 36•14 years ago
|
||
Sounds good to me.
Comment 37•13 years ago
|
||
So did we ever decide on a plan here?
Assignee | ||
Comment 38•13 years ago
|
||
I think that bug 614732 is the plan.
Assignee | ||
Comment 39•13 years ago
|
||
As we clean up attribute changes for use so that they are all processed by the frame then removeAttribute no longer works without changes to the SVG types taking place before the unsetAttr base call. If the content->frame migration for use is done without the unsetAttr changes then dynamic-use-remove-width.svg will fail.
Assignee: nobody → longsonr
Attachment #564076 -
Flags: review?(dholbert)
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 40•13 years ago
|
||
Why the ReportAttributeParseFailure removals in nsSVGElement::ParseAttribute ?
Comment 41•13 years ago
|
||
(attaching qdiff -w version of patch for easier skimming)
Comment 42•13 years ago
|
||
Comment on attachment 564076 [details] [diff] [review] patch > nsresult > nsSVGElement::UnsetAttr(PRInt32 aNamespaceID, nsIAtom* aName, > bool aNotify) >+ bool foundMatch = false; [...] >+ if (!foundMatch) { >+ // Check if this is a length attribute going away >+ LengthAttributesInfo lenInfo = GetLengthInfo(); > >- for (PRUint32 i = 0; i < lenInfo.mLengthCount; i++) { >- if (aName == *lenInfo.mLengthInfo[i].mName) { >- lenInfo.Reset(i); >- DidChangeLength(i, PR_FALSE); >- return rv; >+ for (PRUint32 i = 0; i < lenInfo.mLengthCount; i++) { >+ if (aName == *lenInfo.mLengthInfo[i].mName) { >+ lenInfo.Reset(i); >+ DidChangeLength(i, PR_FALSE); >+ foundMatch = true; >+ break; >+ } > } > } > >- // Check if this is a length list attribute going away >- LengthListAttributesInfo lengthListInfo = GetLengthListInfo(); >+ if (!foundMatch) { I think I prefer the old logic structure (directly returning rather than needing to set / repeatedly-check |foundMatch|). The old way seems cleaner and less fragile to me. I'd prefer that we kept that structure. We can easily do that by renaming the existing UnsetAttr to say "void nsSVGElement::UnsetAttrInternal", and then have the *real* UnsetAttr just call UnsetAttrInternal() and then call the inherited UnsetAttr method. (The existing UnsetAttr[Internal] code would remain the same, except it'd lose the nsSVGElementBase::UnsetAttr() call and all the "return rv" would just become "return". How does that sound?
Assignee | ||
Comment 43•13 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #40) > Why the ReportAttributeParseFailure removals in nsSVGElement::ParseAttribute > ? Because the method already calls that on failures at the end so in these cases you would get 2 method calls. The method is so long that there's no enough context to show the proper call which is at the end of the method.
Assignee | ||
Comment 44•13 years ago
|
||
(In reply to Robert Longson from comment #43) > (In reply to Daniel Holbert [:dholbert] from comment #40) > > Why the ReportAttributeParseFailure removals in nsSVGElement::ParseAttribute > > ? > i.e. 2 reports on the console for each failure.
Assignee | ||
Comment 45•13 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #42) > Comment on attachment 564076 [details] [diff] [review] [diff] [details] [review] > patch > > > nsresult > > nsSVGElement::UnsetAttr(PRInt32 aNamespaceID, nsIAtom* aName, > > bool aNotify) > >+ bool foundMatch = false; > [...] > >+ if (!foundMatch) { > >+ // Check if this is a length attribute going away > >+ LengthAttributesInfo lenInfo = GetLengthInfo(); > > > >- for (PRUint32 i = 0; i < lenInfo.mLengthCount; i++) { > >- if (aName == *lenInfo.mLengthInfo[i].mName) { > >- lenInfo.Reset(i); > >- DidChangeLength(i, PR_FALSE); > >- return rv; > >+ for (PRUint32 i = 0; i < lenInfo.mLengthCount; i++) { > >+ if (aName == *lenInfo.mLengthInfo[i].mName) { > >+ lenInfo.Reset(i); > >+ DidChangeLength(i, PR_FALSE); > >+ foundMatch = true; > >+ break; > >+ } > > } > > } > > > >- // Check if this is a length list attribute going away > >- LengthListAttributesInfo lengthListInfo = GetLengthListInfo(); > >+ if (!foundMatch) { > > > I think I prefer the old logic structure (directly returning rather than > needing to set / repeatedly-check |foundMatch|). The old way seems cleaner > and less fragile to me. > > I'd prefer that we kept that structure. We can easily do that by renaming > (The existing UnsetAttr[Internal] code would remain the same, except it'd > lose the nsSVGElementBase::UnsetAttr() call and all the "return rv" would > just become "return". > > How does that sound? Sure, I think I should change the nsSVGElement::ParseAttribute method to match too then.
Comment 46•13 years ago
|
||
Comment on attachment 564076 [details] [diff] [review] patch >diff --git a/content/svg/content/src/nsSVGElement.cpp b/content/svg/content/src/nsSVGElement.cpp > nsSVGElement::SetLength(nsIAtom* aName, const nsSVGLength2 &aLength) > { > LengthAttributesInfo lengthInfo = GetLengthInfo(); > > for (PRUint32 i = 0; i < lengthInfo.mLengthCount; i++) { > if (aName == *lengthInfo.mLengthInfo[i].mName) { > lengthInfo.mLengths[i] = aLength; >- DidChangeLength(i, PR_TRUE); >+ DidAnimateLength(i); > return; It's not immediately clear why DidAnimateLength is appropriate here (instead of DidChangeLength), since this function sets the base value, not the animated value. I'm guessing that the reason for this change is that this method is only called by nsSVGUseElements on its dynamically-generated content (I think?), and it's not so important to set the generated content's "actual" height/width attributes, as long as mLengths[] is correct. Whether that's the explanation or not, could you add a brief comment in the code here, explaining the use of the "DidAnimate" method?
Assignee | ||
Comment 47•13 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #46) > Comment on attachment 564076 [details] [diff] [review] [diff] [details] [review] > patch > > >diff --git a/content/svg/content/src/nsSVGElement.cpp b/content/svg/content/src/nsSVGElement.cpp > > nsSVGElement::SetLength(nsIAtom* aName, const nsSVGLength2 &aLength) > > { > > LengthAttributesInfo lengthInfo = GetLengthInfo(); > > > > for (PRUint32 i = 0; i < lengthInfo.mLengthCount; i++) { > > if (aName == *lengthInfo.mLengthInfo[i].mName) { > > lengthInfo.mLengths[i] = aLength; > >- DidChangeLength(i, PR_TRUE); > >+ DidAnimateLength(i); > > return; > > It's not immediately clear why DidAnimateLength is appropriate here (instead > of DidChangeLength), since this function sets the base value, not the > animated value. Actually it sets both. > > I'm guessing that the reason for this change is that this method is only > called by nsSVGUseElements on its dynamically-generated content (I think?), > and it's not so important to set the generated content's "actual" > height/width attributes, as long as mLengths[] is correct. Whether that's > the explanation or not, could you add a brief comment in the code here, > explaining the use of the "DidAnimate" method? What we need to do is to get a refresh when the svg or symbol length is changed. Before bug 689546 landed we had to generate a content change, after that patch landed we only needed a frame attribute changed call. The DidAnimate did not do enough work before that patch but now does and as it does less work it's a performance improvement. If I'd realised at the time I'd have incorporated the change in bug 689546.
Comment 48•13 years ago
|
||
(In reply to Robert Longson from comment #43) > (In reply to Daniel Holbert [:dholbert] from comment #40) > > Why the ReportAttributeParseFailure removals in nsSVGElement::ParseAttribute > > ? > > Because the method already calls that on failures at the end so in these > cases you would get 2 method calls. The method is so long that there's no > enough context to show the proper call which is at the end of the method. Ok, cool - sounds good then. (In reply to Robert Longson from comment #45) > Sure, I think I should change the nsSVGElement::ParseAttribute method to > match too then. Cool. That could definitely happen in a separate bug, but I'd be happy to have it as part of this patch, too, if you prefer. (In reply to Robert Longson from comment #47) > > It's not immediately clear why DidAnimateLength is appropriate here (instead > > of DidChangeLength), since this function sets the base value, not the > > animated value. > > Actually it sets both. Ah right, sorry. The nsSVGLength2 has both values, righto. > What we need to do is to get a refresh when the svg or symbol length is > changed. [...] If I'd realised at the time > I'd have incorporated the change in bug 689546. Ok, fair enough. My forgetfulness about setting the base value vs. base+animated was part of the confusion - this makes more sense given that. Nevermind about this part.
Assignee | ||
Comment 49•13 years ago
|
||
In the end I didn't rework ParseAttribute as it's not quite as straightforward as I thought to do so. I have changed UnsetAttr as you suggested though.
Attachment #564076 -
Attachment is obsolete: true
Attachment #565433 -
Attachment is obsolete: true
Attachment #565492 -
Flags: review?(dholbert)
Attachment #564076 -
Flags: review?(dholbert)
Updated•13 years ago
|
Attachment #565492 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 50•13 years ago
|
||
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/d85e3d033bf1
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite+
Comment 51•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d85e3d033bf1
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in
before you can comment on or make changes to this bug.
Description
•