534526 - Removing transform attribute does not un-match selector [transform="scale(2)"]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (should be green)

Based on layout/reftests/svg/selector-01.svg, a reftest from bug 477996.

Comment 1

[Robert Longson [:longsonr]]

WFM on Windows on both Trunk and 3.5. Mac only?

Comment 2

[Jonathan Watt [:jwatt]]

WFM on Mac 10.5 on both trunk and 3.6.

Comment 3

[Jesse Ruderman]

Weird, it's still red for me (10.5, trunk).

Comment 4

[Robert Longson [:longsonr]]

It's now broken for me. When we repaint after the transform is removed the style system seems to be saying we're still red rather than green.

I'm pretty sure this is a regression and presumably happened around the time Jesse first found it so a regression range would be most useful.

Comment 5

[Robert Longson [:longsonr]]

Boris, I think one bug 523294, bug 523288 or bug 523288

regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-12-10+14%3A00%3A41&enddate=2009-12-10+18%3A28%3A20

Thu Dec 10 14:00:42 c3a97b86e763 OK
Thu Dec 10 18:28:19 813718244b94 Bad

What happens is that we get the style change in nsSVGPathGeometryFrame::DidSetStyleContext OK where we invalidate but when we redraw we end up in nsSVGGeometryFrame::SetupCairoFill where GetStyleSVG()->mFill.mPaint.mColor says we're still red.

If we do another unrelated style change then we refresh to green so there's some lazy update somewhere now.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Almost certainly bug 523294.  DOM inspector shows that our style context is still taking into account the [transforme="scale(2)"] rule.

Comment 7

[Boris Zbarsky [:bzbarsky]]

And removeAttribute does work in general for an HTML element, as expected.  Fun.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  So inside the AttributeWillChange notification for the UnsetAttr call, when we're doing nsAttrValue::Equals to check whether we match the attr, ToString() on the attr value gives the empty string.

Comment 9

[Boris Zbarsky [:bzbarsky]]

And this->Type() on the nsAttrValue is eSVGValue.

Comment 10

[Boris Zbarsky [:bzbarsky]]

And _that_ happens because the relevant nsSVGTransformList has mTransforms.Length() == 0.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Aha.  nsSVGElement::UnsetAttr actually clears the attribute value before ever calling into the superclass UnsetAttr implementation.  That's just broken; the assumption is that AttributeWillChange has the old attribute value, and that assumption is being broken here.

Can we fix that on the SVG end?  For example, could all this clearing of 
stuff happen after the actual attr removal?  Or do we need a new hook in between AttributeWillChange and AttributeChanged in UnsetAttr.

Comment 12

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

(In reply to comment #11)
> Can we fix that on the SVG end?  For example, could all this clearing of 
> stuff happen after the actual attr removal?

I think so.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I looked at some of our other SetAttr and UnsetAttr impls, and this is actually a problem in a few other cases (esp where link states are concerned, after Shawn's changes).  Ideally, we'd have a hook in both SetAttr and UnsetAttr that would get called between the WillChange and Changed notifications that all those reset actions could happen on....

In particular, for the SVG case it's not obvious to me how to restructure the code so that it doesn't depend on being able to get the attribute that's about to be removed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

And in SVG, for the setAttribute case, things still fail because ParseAttribute (which actually nukes the old value) comes before AttributeWillChange....

Jonas, I think we need to pretty seriously consider moving to a setup where the old and new nsAttrValue are both available between AttributeWillChange and AttributeChanged, and the content is notified with both, as we discussed in bug 314286.  Or back out bug 523294, of course....

Comment 15

[Robert Longson [:longsonr]]

(In reply to comment #13)
> I looked at some of our other SetAttr and UnsetAttr impls, and this is actually
> a problem in a few other cases (esp where link states are concerned, after
> Shawn's changes).  Ideally, we'd have a hook in both SetAttr and UnsetAttr that
> would get called between the WillChange and Changed notifications that all
> those reset actions could happen on....

Yes.

> 
> In particular, for the SVG case it's not obvious to me how to restructure the
> code so that it doesn't depend on being able to get the attribute that's about
> to be removed.

Calling the base class implementation first in nsSVGElement::UnsetAttr seems to fix this particular case. Comment 14 hints that that is naive though.

Comment 16

[Boris Zbarsky [:bzbarsky]]

A possible fix that doesn't involve fixing bug 314286 full-out is to move the AttributeWillChange calls much earlier in the attr-setting process (to before ParseAttribute, and before BeforeSetAttr) and to move the SVG code that's currently running in UnsetAttr into BeforeSetAttr.  And auditing the various non-SVG code that might have similar issues (all SetAttr and UnsetAttr implementations, in fact).

I'll probably work on that tomorrow.

Comment 17

[Robert Longson [:longsonr]]

FWIW putting the base class implementation first in ParseAttribute breaks all the css styling SVG testsuite tests:

http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-styling-css-01-b.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-styling-css-02-b.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-styling-css-03-b.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-styling-css-04-b.html

So it's worth checking that these work after your rewrite or incorporating them into reftests.

Comment 18

[Robert Longson [:longsonr]]

If this is going to take a while Boris then it might be best to back out bug  523294.

Alternatively we could do something simple like move the base class call in nsSVGElement::UnsetAttr to the start of the method rather than the end. Even if that isn't quite right it still gets the attribute removal functionality working again.

Comment 19

[Boris Zbarsky [:bzbarsky]]

It doesn't, actually.  Just changing the testcase to use :not will make it break again if you do that.

Fixing this is pretty easy, actually; I know exactly how to do it.  The plan is to just move AttributeWillChange much earlier in attr changes and remove all current overrides of SetAttr and UnsetAttr (and document that one shouldn't do that).  I'm just waiting for Shawn to land his async history stuff so we don't stomp on each other's changes...

If this is urgent (as in, breaking something in a way that people are prevented from using trunk), I can back out, but if it can wait until Shawn lands that would be much nicer in terms of time spent on it.

Comment 20

[Robert Longson [:longsonr]]

People will start to notice that it's broken. They did so with reasonable frequently before it was implemented. 

If it's going to be a fairly short time thing I'm sure they can wait.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> They did so with reasonable frequently before it was implemented. 

Which?  Attribute selectors not changing whether they match?

Note that the attr removal per se works fine.  What doesn't work right is dynamic updating of whether CSS attribute selectors match.

Comment 22

[Robert Longson [:longsonr]]

Ahh OK, in which case it's rarer than I thought.

Comment 23

[Shawn Wilsher :sdwilsh]

Note that bug 461199 disables two tests because of this bug:
layout/reftests/svg/dynamic-link-style-01.svg
content/html/content/test/test_bug209275.xhtml (is changed to a todo)

Comment 24

[Shawn Wilsher :sdwilsh]

Bug 461199 also disables this test case because of this bug:
content/html/content/test/test_bug481335.xhtml

Comment 25

[Boris Zbarsky [:bzbarsky]]

OK, I started looking into this and discovered that AttributeWillChange notifications for the DidModifySVGObservable case are and have been all along completely broken.

Furthermore, I don't see a way to send an AttributeWillChange from WillModifySVGObservable, because at that point we don't know what the right attr name is, unless I'm missing something....  Robert, please tell me I'm missing something?

Comment 26

[Robert Longson [:longsonr]]

We have been moving away from nsISVGValue. One day WillModifySVGObservable will be a distant memory I hope.

Till that day...

check out nsSVGElement::DidModifySVGObservable. There is a mMappedAttributes object that lets you look up the attribute.

You could put a public lookup method in nsSVGElement to get the attribute given an nsISVGValue *

Is this what you wanted? 

You would need to know the element that the WillModifyObservable is observing. New types (nsSVGAngle etc) have DOM tear offs with nsRefPtr<nsSVGElement> mSVGElement that points to the element, old types don't I don't think. I think they rely on the nsSVGElement:Did/WillModify to do things for them.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> here is a mMappedAttributes object that lets you look up the attribute.

Yes, but is that already all set up with the relevant nsISVGValue inside WillModifySVGObservable?  That is, if I just copy that code into nsSVGElement::WillModifySVGObservable (or refactor it) and send AttributeWillChange notifications from there, will that do the right thing?

Comment 28

[Robert Longson [:longsonr]]

I think that's right. It's certainly what I'd try if I was doing it.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Remove a SetAttrAndNotify caller

This just reduces the number of callsites the second patch needs to deal with... though I suspect it's not strictly needed in any case.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. Main fix for SetAttr and inline style

This touches all three of SVG, inline style, and generic element, sadly.  The inline style change is somewhat hacky; if sicking or dbaron would prefer an explicit boolean passed to GetDeclaration() instead I can do that too.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3. Fix for the original testcase this bug was filed on

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4. Tests.

Comment 33

[Robert Longson [:longsonr]]

> Created an attachment (id=426768) [details]
> +  // Why is this code needed for SVG but not other nsStyledElements?

See bug 387466 for the answer to the why question in part 1. I never did manage to get a working reftest for that which matched properly, however bug 309020 does have a reftest which may cover the same ground.(In reply to comment #29)

Comment 34

[Robert Longson [:longsonr]]

Do the tests cover the case you mentioned in the first sentence in comment 19? If not I think you ought to have a test for that too.

Comment 35

[Robert Longson [:longsonr]]

Comment on attachment 426770 [details] [diff] [review]
Part 2.  Main fix for SetAttr and inline style

>From: Boris Zbarsky <[e-mail]>
>+  struct ObservableModificationData {
>+    // Only to be used if |name| is non-null.  Otherwise, modType will
>+    // be 0 to indicate NS_OK shold be returned and 1 to indicate

s/shold/should/

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Do the tests cover the case you mentioned in the first sentence in comment 19?

Er, they don't, but that comment was wrong.  I'll definitely add such a test.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 426770 [details] [diff] [review]
Part 2.  Main fix for SetAttr and inline style

r=dbaron on the layout/style/* part; if you want me to understand the rest of what's going on in this patch, let me know

Comment 38

[Boris Zbarsky [:bzbarsky]]

Nah, the layout/style part is what I wanted from you.  ;)

Comment 39

[Boris Zbarsky [:bzbarsky]]

> See bug 387466 for the answer to the why question in part 1. 

Ah, hacking around xbl bogosity in multiple different ways.  I'll make that clearer in the comment.

> s/shold/should/

Fixed.

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 426768 [details] [diff] [review]
Part 1.  Remove a SetAttrAndNotify caller

>@@ -1968,16 +1907,17 @@ nsXULElement::GetStyle(nsIDOMCSSStyleDec
>             nsCOMPtr<nsICSSStyleRule> styleRule = do_QueryInterface(ru...
>             value.SetTo(styleRule);
> 
>             rv = mAttrsAndChildren.SetAndTakeAttr(nsGkAtoms::style, va...
>             NS_ENSURE_SUCCESS(rv, rv);
>         }
>     }
> 
>+    // XXXbz could this call nsStyledElement::GetStyle now?

As far as I can see, this entire method can be removed and we can fall back to nsStyledElement::GetStyle. The only thing this accomplishes is so that we don't share the stylerule with the prototype nodes style rule when .style is used. However any modifications to .style just clones the stylerule and so there is no harm in sharing right?

I'd also not be opposed to introducing an nsXULElementBase typedef.

r=me either way.

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 426770 [details] [diff] [review]
Part 2.  Main fix for SetAttr and inline style

Ugh, this is certainly a bit nasty, but I don't know of a better way.

r=me

Comment 42

[Boris Zbarsky [:bzbarsky]]

> The only thing this accomplishes is so that we don't
> share the stylerule with the prototype nodes style rule when .style is used.

It also accomplishes us actually seeing the prototype's style attribute at all.  That part can't go away.  The part that could, I think, is the slots stuff after the |if (mPrototype)| block.

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Won't nsDOMCSSAttributeDeclaration call GetInlineStyle(), which is implemented on nsXULElement?

Comment 44

[Boris Zbarsky [:bzbarsky]]

Oh, right you are.  But modifying the style rule does not in fact clone the style rule.  Or more precisely it does not clone the rule's nsCSSDeclaration....

Comment 45

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/mozilla-central/rev/08a105de8f0b
  http://hg.mozilla.org/mozilla-central/rev/45fac6dd87f9
  http://hg.mozilla.org/mozilla-central/rev/f643c44fe147
  http://hg.mozilla.org/mozilla-central/rev/8853f7e8d843