Last Comment Bug 550047 - Removing 'x' attribute from foreignObject doesn't move it
: Removing 'x' attribute from foreignObject doesn't move it
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Robert Longson
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: refdyn 534526 550065
  Show dependency treegraph
 
Reported: 2010-03-03 15:29 PST by Jesse Ruderman
Modified: 2011-10-09 08:11 PDT (History)
9 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (dynamic) (464 bytes, image/svg+xml)
2010-03-03 15:29 PST, Jesse Ruderman
no flags Details
reference (static) (307 bytes, image/svg+xml)
2010-03-03 15:29 PST, Jesse Ruderman
no flags Details
patch (22.87 KB, patch)
2011-10-02 13:11 PDT, Robert Longson
no flags Details | Diff | Splinter Review
no-whitespace version of patch (17.64 KB, patch)
2011-10-06 19:41 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
address review comments (18.68 KB, patch)
2011-10-07 04:45 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2010-03-03 15:29:20 PST
Created attachment 430165 [details]
testcase (dynamic)
Comment 1 Jesse Ruderman 2010-03-03 15:29:37 PST
Created attachment 430166 [details]
reference (static)
Comment 2 Robert Longson 2010-03-04 08:30:41 PST
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 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 08:57:17 PST
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.
Comment 4 Robert Longson 2010-03-04 09:02:01 PST
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 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 09:05:35 PST
> 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 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 09:07:54 PST
Also, are there other SVG frames that rely on these sorts of backdoors?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 09:10:22 PST
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 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 09:10:46 PST
Under UpdateCoveredRegion?
Comment 9 Robert Longson 2010-03-04 09:11:50 PST
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 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 09:13:48 PST
> I think that you can animate unset attributes.

Then how can the code in nsSVGElementBase::UnsetAttr possibly be correct?
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 09:14:02 PST
Er, I meant in nsSVGElement::UnsetAttr.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 09:15:05 PST
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?
Comment 13 Robert Longson 2010-03-04 09:15:51 PST
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 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 09:16:05 PST
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?
Comment 15 Robert Longson 2010-03-04 09:17:23 PST
comment 12 is correct.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 09:17:42 PST
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?
Comment 17 Robert Longson 2010-03-04 09:27:16 PST
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 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 09:33:44 PST
> 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?
Comment 19 Robert Longson 2010-03-04 09:44:34 PST
(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 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 10:00:43 PST
> 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. ;)
Comment 21 Robert Longson 2010-03-04 10:08:55 PST
(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?
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-03-04 12:57:42 PST
Losing the oldRegion == newRegion test is OK. Let's do that.
Comment 23 Daniel Holbert [:dholbert] 2010-03-04 13:15:47 PST
(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.
Comment 24 Robert Longson 2010-03-04 13:28:46 PST
We only have a problem with unmapped attributes i.e. x, y, width, height and transform I think.
Comment 25 Robert Longson 2010-03-04 14:15:39 PST
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.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-03-04 14:22:39 PST
I agree.
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-03-04 14:24:44 PST
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.
Comment 28 Robert Longson 2010-03-04 14:42:55 PST
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.
Comment 29 Robert Longson 2010-03-04 14:46:53 PST
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.
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-03-04 14:47:47 PST
OK
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-03-04 14:48:08 PST
But won't that cause flicker, as the object disappears temporarily?
Comment 32 Robert Longson 2010-03-04 14:59:36 PST
It wouldn't disappear, it would just stay in the old place. Invalidation/clearing would happen in the async code.
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-03-04 15:01:22 PST
But what if something else causes a paint in the meantime?
Comment 34 Robert Longson 2010-03-04 15:03:04 PST
Could we suppress that? Or if not, clip it to the region?
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2010-03-04 15:10:16 PST
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.
Comment 36 Robert Longson 2010-03-04 15:30:52 PST
Sounds good to me.
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2011-02-03 10:38:52 PST
So did we ever decide on a plan here?
Comment 38 Robert Longson 2011-02-03 11:36:03 PST
I think that bug 614732 is the plan.
Comment 39 Robert Longson 2011-10-02 13:11:26 PDT
Created attachment 564076 [details] [diff] [review]
patch

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.
Comment 40 Daniel Holbert [:dholbert] 2011-10-06 19:36:31 PDT
Why the ReportAttributeParseFailure removals in nsSVGElement::ParseAttribute ?
Comment 41 Daniel Holbert [:dholbert] 2011-10-06 19:41:52 PDT
Created attachment 565433 [details] [diff] [review]
no-whitespace version of patch

(attaching qdiff -w version of patch for easier skimming)
Comment 42 Daniel Holbert [:dholbert] 2011-10-06 19:54:28 PDT
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?
Comment 43 Robert Longson 2011-10-06 20:00:15 PDT
(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.
Comment 44 Robert Longson 2011-10-06 20:01:48 PDT
(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.
Comment 45 Robert Longson 2011-10-06 20:03:26 PDT
(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 Daniel Holbert [:dholbert] 2011-10-06 20:33:09 PDT
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?
Comment 47 Robert Longson 2011-10-06 20:41:52 PDT
(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 Daniel Holbert [:dholbert] 2011-10-06 23:06:22 PDT
(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.
Comment 49 Robert Longson 2011-10-07 04:45:33 PDT
Created attachment 565492 [details] [diff] [review]
address review comments

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.
Comment 51 :Ms2ger (⌚ UTC+1/+2) 2011-10-09 07:37:01 PDT
https://hg.mozilla.org/mozilla-central/rev/d85e3d033bf1

Note You need to log in before you can comment on or make changes to this bug.