515080 - setting "fill" doesn't always cause re-styling

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase

I noticed this when style-property-not-on-script-element-01.svg started to
fail when I fixed bug 344258.
An extra flush before setting the fill attribute fixes the testcase.

(testcase works on Opera and Safari)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #0)
> An extra flush before setting the fill attribute fixes the testcase.
Um, or maybe not.
It was some variant of the testcase it fixed.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Er, not 344258, but 514487

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase + flush

Adding the .getElementById("") call flushes.

Comment 4

[Robert Longson [:longsonr]]

It's not so much about flushing but about when this runs isn't it? If make the change from the svg onload as a function then it works. If you run it before onload has happened (as here) then layout has not taken place and it doesn't work.

Comment 5

[Robert Longson [:longsonr]]

Attachment: onload

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

It has something to do with flush because attachment 399108 [details] works, but attachment 399105 [details] doesn't. The only difference is the document.getElementById("foo"); which flushes.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the problem is that the script is running while the sink has unflushed content in it (!).  It sets the attribute; nsSVGElement::BeforeSetAttr clears mContentStyleRule, but then under SetAttrAndNotify we end up flushing the sink:

#0  nsSVGElement::WalkContentStyleRules (this=0x16d95200, aRuleWalker=0x1d5b33f0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/svg/content/src/nsSVGElement.cpp:733
#1  0x11dbb6f2 in nsHTMLStyleSheet::RulesMatching (this=0x1e4a1ce0, aData=0xbfffab3c) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/style/nsHTMLStyleSheet.cpp:497
#2  0x11de33ad in EnumRulesMatching (aProcessor=0x1e4a1ce4, aData=0xbfffab3c) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/style/nsStyleSet.cpp:427
#3  0x11de4fd4 in nsStyleSet::FileRules (this=0x16d9e7d0, aCollectorFunc=0x11de3388 <EnumRulesMatching(nsIStyleRuleProcessor*, void*)>, aData=0xbfffab3c) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/style/nsStyleSet.cpp:554
#4  0x11de6119 in nsStyleSet::ResolveStyleFor (this=0x16d9e7d0, aContent=0x16d95200, aParentContext=0x2173dba8) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/style/nsStyleSet.cpp:693
#5  0x11ba6700 in nsCSSFrameConstructor::ResolveStyleContext (this=0x16d86ce0, aParentStyleContext=0x2173dba8, aContent=0x16d95200) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:4722
#6  0x11ba6815 in nsCSSFrameConstructor::ResolveStyleContext (this=0x16d86ce0, aParentFrame=0x2173dce0, aContent=0x16d95200) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:4712
#7  0x11bab3f7 in nsCSSFrameConstructor::AddFrameConstructionItems (this=0x16d86ce0, aState=@0xbfffacd8, aContent=0x16d95200, aContentIndex=7, aParentFrame=0x2173dce0, aItems=@0xbfffad24) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:5164
#8  0x11bbc40d in nsCSSFrameConstructor::ContentAppended (this=0x16d86ce0, aContainer=0x16d6f760, aNewIndexInContainer=6) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsCSSFrameConstructor.cpp:6365
#9  0x11c3174b in PresShell::ContentAppended (this=0x16d9e890, aDocument=0x1500a00, aContainer=0x16d6f760, aNewIndexInContainer=6) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsPresShell.cpp:4894
#10 0x11f2e4b0 in nsNodeUtils::ContentAppended (aContainer=0x16d6f760, aNewIndexInContainer=6) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsNodeUtils.cpp:132
#11 0x11e95560 in nsContentSink::NotifyAppend (this=0x16d921d0, aContainer=0x16d6f760, aStartIndex=6) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsContentSink.cpp:1375
#12 0x120815e8 in nsXMLContentSink::FlushTags (this=0x16d921d0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/xml/document/src/nsXMLContentSink.cpp:1680
#13 0x11e946d9 in nsContentSink::BeginUpdate (this=0x16d921d0, aDocument=0x1500a00, aUpdateType=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsContentSink.cpp:1612
#14 0x11edcbf1 in nsDocument::BeginUpdate (this=0x1500a00, aUpdateType=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsDocument.cpp:3748
#15 0x11c92593 in mozAutoDocUpdate::mozAutoDocUpdate (this=0xbfffb214, aDocument=0x1500a00, aUpdateType=1, aNotify=1) at mozAutoDocUpdate.h:53
#16 0x11f11460 in nsGenericElement::SetAttrAndNotify (this=0x16d95090, aNamespaceID=0, aName=0x121b634, aPrefix=0x0, aOldValue=@0xbfffb2a4, aParsedValue=@0xbfffb340, aModification=1, aFireMutation=0, aNotify=1, aValueForAfterSetAttr=0xbfffb43c) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsGenericElement.cpp:4386
#17 0x11f1275f in nsGenericElement::SetAttr (this=0x16d95090, aNamespaceID=0, aName=0x121b634, aPrefix=0x0, aValue=@0xbfffb43c, aNotify=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsGenericElement.cpp:4366

This causes us to resolve style for the <rect>, which uses the _old_ attribute value for creating the mContentStyleRule (since the new one hasn't been set yet).  Then we never create an mContentStyleRule with the new value.

It's weird that we don't flush the sink before running the script.  It's a bit weird to me that we don't start the attr-set update before we call BeforeSetAttr, maybe.  But given that we don't, clearing the mContentStyleRule in BeforeSetAttr is just wrong...  The right place to do it is in AfterSetAttr, I would think.

Comment 8

[Robert Longson [:longsonr]]

Attachment: patch

Comment 9

[Robert Longson [:longsonr]]

Forgot to fix up the reftest title I'll make change it to this on check in.

<title>Testcase ensuring fill works if applied before onload</title>

Comment 10

[Robert Longson [:longsonr]]

pushed http://hg.mozilla.org/mozilla-central/rev/7d03ad8e0e5b

Comment 11

[Robert Longson [:longsonr]]

Comment on attachment 400003 [details] [diff] [review]
patch

Simple fix with testcase for longstanding issue

Comment 12

[Robert Longson [:longsonr]]

pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ecda7818bb68