Closed Bug 534526 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(5 files)

Based on layout/reftests/svg/selector-01.svg, a reftest from bug 477996.
WFM on Windows on both Trunk and 3.5. Mac only?
WFM on Mac 10.5 on both trunk and 3.6.
Weird, it's still red for me (10.5, trunk).
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.
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.
OS: Mac OS X → All
Hardware: x86 → All
Almost certainly bug 523294.  DOM inspector shows that our style context is still taking into account the [transforme="scale(2)"] rule.
Assignee: nobody → bzbarsky
Blocks: 523294
Status: NEW → ASSIGNED
And removeAttribute does work in general for an HTML element, as expected.  Fun.
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.
And this->Type() on the nsAttrValue is eSVGValue.
And _that_ happens because the relevant nsSVGTransformList has mTransforms.Length() == 0.
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.
(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.
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.
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....
Depends on: 314286
(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.
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.
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.
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.
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.
> 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.
Ahh OK, in which case it's rarer than I thought.
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)
Bug 461199 also disables this test case because of this bug:
content/html/content/test/test_bug481335.xhtml
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?
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.
> 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?
I think that's right. It's certainly what I'd try if I was doing it.
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.
Attachment #426768 - Flags: review?(jonas)
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.
Attachment #426770 - Flags: review?(longsonr)
Attachment #426770 - Flags: review?(jonas)
Attachment #426770 - Flags: review?(dbaron)
Attached patch Part 4. Tests.Splinter Review
> 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)
Attachment #426772 - Attachment is patch: true
Attachment #426772 - Attachment mime type: application/octet-stream → text/plain
Attachment #426771 - Flags: review?(longsonr) → review+
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 on attachment 426770 [details] [diff] [review]
Part 2.  Main fix for SetAttr and inline style

>From: Boris Zbarsky <bzbarsky@mit.edu>
>+  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/
Attachment #426770 - Flags: review?(longsonr) → review+
> 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 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
Attachment #426770 - Flags: review?(dbaron) → review+
Nah, the layout/style part is what I wanted from you.  ;)
> 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 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.
Attachment #426768 - Flags: review?(jonas) → review+
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
Attachment #426770 - Flags: review?(jonas) → review+
> 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.
Won't nsDOMCSSAttributeDeclaration call GetInlineStyle(), which is implemented on nsXULElement?
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....
No longer blocks: async-visited-check
Depends on: 550065
Depends on: 550047
Depends on: 1468851
You need to log in before you can comment on or make changes to this bug.