550047 - Removing 'x' attribute from foreignObject doesn't move it

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (dynamic)

Comment 1

[Jesse Ruderman]

Attachment: reference (static)

Comment 2

[Robert Longson [:longsonr]]

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 [:bzbarsky]]

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 [:longsonr]]

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 [:bzbarsky]]

> 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 [:bzbarsky]]

Also, are there other SVG frames that rely on these sorts of backdoors?

Comment 7

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Under UpdateCoveredRegion?

Comment 9

[Robert Longson [:longsonr]]

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 [:bzbarsky]]

> I think that you can animate unset attributes.

Then how can the code in nsSVGElementBase::UnsetAttr possibly be correct?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Er, I meant in nsSVGElement::UnsetAttr.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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 [:longsonr]]

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 [:bzbarsky]]

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 [:longsonr]]

comment 12 is correct.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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 [:longsonr]]

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 [:bzbarsky]]

> 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 [:longsonr]]

(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 [:bzbarsky]]

> 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 [:longsonr]]

(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)]

Losing the oldRegion == newRegion test is OK. Let's do that.

Comment 23

[Daniel Holbert [:dholbert]]

(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 [:longsonr]]

We only have a problem with unmapped attributes i.e. x, y, width, height and transform I think.

Comment 25

[Robert Longson [:longsonr]]

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)]

I agree.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 [:longsonr]]

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 [:longsonr]]

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)]

OK

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But won't that cause flicker, as the object disappears temporarily?

Comment 32

[Robert Longson [:longsonr]]

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)]

But what if something else causes a paint in the meantime?

Comment 34

[Robert Longson [:longsonr]]

Could we suppress that? Or if not, clip it to the region?

Comment 35

[Boris Zbarsky [:bzbarsky]]

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 [:longsonr]]

Sounds good to me.

Comment 37

[Boris Zbarsky [:bzbarsky]]

So did we ever decide on a plan here?

Comment 38

[Robert Longson [:longsonr]]

I think that bug 614732 is the plan.

Comment 39

[Robert Longson [:longsonr]]

Attachment: 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]]

Why the ReportAttributeParseFailure removals in nsSVGElement::ParseAttribute ?

Comment 41

[Daniel Holbert [:dholbert]]

Attachment: no-whitespace version of patch

(attaching qdiff -w version of patch for easier skimming)

Comment 42

[Daniel Holbert [:dholbert]]

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 [:longsonr]]

(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 [:longsonr]]

(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 [:longsonr]]

(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]]

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 [:longsonr]]

(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]]

(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 [:longsonr]]

Attachment: 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 50

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/d85e3d033bf1

Comment 51

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/d85e3d033bf1