536677 - svg fill with url to gradient not inherited when dynamically switched more than once

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[shropshire]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)

Was building a map of the US so that when user moused over a state the states
background fill would change.  When user moused out it would change back.  Also made states have a different fill when users selected them by clicking, and yet another fill when a selected state was moused over.  Altogether 4 different background fills (not selected, mouseover+not selected, selected, selected+mouseover).  3 of the fills are radial gradients. It is when switching from a radial gradient to another radial gradient on the parent class that the 2nd radial gradient does not take effect.  According to the SVG spec, fills are inherited (http://www.w3.org/TR/SVG11/painting.html#FillProperties).

The state is drawn using a path element.  This path element is a child of a group element.  When the fill is changed on the group element from one gradient to another, the path element does not inherit the new gradient fill on the 2nd change (it inherits the 1st time only) (or at least does not display with the new fill).  It does, however, correctly inherit the groups stroke property.  The fill is changed on the group element by changing its class attribute in javascript.  If the class attribute is moved to the path element, everything works.  

I have attached 2 samples:  in the 1st sample file, the class attribute is placed on the group element (does not work in FF, but works in IEw/Adobe and Opera).  in the 2nd file the class attribute is placed on the path element (works in FF, IE, and Opera).

Reproducible: Always

Steps to Reproduce:
1.Open test.svg
2.One will see the state of Washington (I removed all the other states in the US map to make this simple).
3.Mouse over the state of Washington.  One will see a green gradient fill.  Click on the state (fill SHOULD turn to a blue gradient but does not - note it works on IE 7+Adobe SVG, and Opera (although Opera has a different bug with the stroking).
Actual Results:  
After clicking the state, the stroke correctly changes, but the fill remains the same.

Expected Results:  
The fill should change.

Comment 1

[shropshire]

Attachment: This one demonstrates the problem.

Comment 2

[shropshire]

Attachment: This one shows a workaround for the bug. It shows correctly.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Confirmed with latest trunk on Windows Vista. This seems a regression from Firefox 3.0.

Comment 4

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

The best regression range I could get with Linux x86_64 nightlies is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2e7f03bc1820&tochange=18fec592b35b
since all the nightlies in the middle crash on startup for me.

Comment 5

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

Regression range from Windows nightlies:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=15cb5d25db05&tochange=86b982e8b73e

Comment 6

[Robert Longson [:longsonr]]

When we set the fill property that holds the gradient we do so on the nsSVGPathGeometryFrame (because that's what we're drawing).

When we clear it (because the style has changed) nsCSSFrameConstructor::ProcessRestyledFrames calls nsSVGEffects::UpdateEffects but the frame passed in is the parent <g> frame.

roc, should we recurse through child frames in nsSVGEffects::UpdateEffects and clear their properties too or do you see any better solution?

Comment 7

[Robert Longson [:longsonr]]

Attachment: like so?

Comment 8

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

I'm not sure whether nsCSSFrameConstructor::ProcessRestyledFrames is supposed to operate on all the descendants of the frames in its list. I think not, in which case this isn't the right fix.

The 'fill' is being inherited from the <g> into the <path>. Changing the style of the <g> should have caused reresolution of the style on the <path>, which should have triggered an nsChangeHint_UpdateEffects for the path's frame.

Comment 9

[Boris Zbarsky [:bzbarsky]]

ProcessRestyledFrames is in fact supposed to recurse into descendants.  Or more precisely, the code that computes the changehints and the list assumes that as long as a frame with a given changehint is in the list no descendants with hints that are sub-hints of that one need to be added to the list.

Presumably during ReResolveStyleContext when we recoputed the style on the <path> and then tried to add it to the list this was optimized away by the above.  We could not do that for nsChangeHint_UpdateEffects (e.g. by removing it from the min hint at the top of ReResolveStyleContext) if it's rare for a change on a node that requires that bit to also require it on all descendants.

Comment 10

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

Comment on attachment 420366 [details] [diff] [review]
like so?

This is probably fine for 'fill', 'stroke' and markers. It's probably less fine for 'filter', 'clipPath' and 'mask', but I don't think we need to worry about it.

Comment 11

[Robert Longson [:longsonr]]

checked in http://hg.mozilla.org/mozilla-central/rev/d22694ba13c9