Closed Bug 435525 Opened 12 years ago Closed 11 years ago

Setting viewBox="" on a referenced SVG document from script does not cause an update


(Core :: SVG, defect)

Not set





(Reporter: heycam, Assigned: craig.topper)





(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9) Gecko/2008051202 Firefox/3.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9) Gecko/2008051202 Firefox/3.0

If an SVG document with no viewBox="" is referenced from an <object> element, and then a viewBox="" is added with script after the document loads, the SVG is not zoomed to the viewBox.

Reproducible: Always

Steps to Reproduce:
Open the above URL.

Actual Results:  
Only the (0,0)->(100,100) region of the SVG document is rendered into the 100x100 <object>, with scroll bars.

Expected Results:  
The (0,0)->(400,400) region of the SVG document should be scaled to fit the 100x100 <object>.
Depends on: 374111
OS: Linux → All
Hardware: PC → All
Version: unspecified → Trunk
This was fixed with the checkin in bug 374111 (the viewbox now gets updated).
Assignee: nobody → craig.topper
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed by bug 374111]
Resolution: FIXED → ---
This bug has not been fixed. setAttribute is not correctly forcing an update. DidChangeViewBox attempt to force an update, but InvalidateTransformNotifyFrame relies on HasAttr which isn't correct at the time DidChangeViewBox is called. A temporary fix is to restore AfterSetAttr and UnsetAttr to the nsSVGSVGElement. The HasAttr code will need to corrected for animation to work anyway.

Even with the the above fix, the testcase here can't produce the expected result because of the explicit width/height on the svg element. There is commented out code to remove the width and height and if you restore that code then you get the expected results.

Unfortunately, as soon as you restore that code you hide the bug in DidChangeViewBox because DidChangeLength will force an invalidate and at that point HasAttr(viewbox) will return the correct answer. So to really test this you need to set width and height on the <svg> element to 100% instead of 400.
Ever confirmed: true
Whiteboard: [fixed by bug 374111]
This patch removes DidChangeViewBox from nsSVGSVGElement and puts back UnsetAttr and AfterSetAttr to do the invalidation.
Attachment #361211 - Flags: review?(roc)
Forgot to mention that setAttribute isn't actually calling DidChangeViewBox at this time, but that has been filed as bug 477530. The fix here does not rely on that fix though.
Who is actually calling HasAttr(viewBox)? should that code be doing something differently, perhaps calling GetViewBox?
It's called in nsSVGSVGElement::GetLength and nsSVGSVGElement::GetViewboxToViewportTransform the latter is the one giving the trouble here.

It should definitely be doing something different since HasAttr won't work for animated viewboxes that have no base value defined. I don't think GetViewBox is the answer cause all that always returns something regardless of whether the viewbox is valid. I think we need a valid flag within the nsSVGViewBox or see if we can use the values of the viewbox to determine if it's valid.

nsSVGMarkerElement has some issues with it's viewbox implementation as well. It attempts to keep its base viewbox in sync with its width and height if HasAttr(viewbox) is false. In addition to that not being animation friendly, percentage lengths don't work entirely correctly there due to lack of a proper notification system to fix up the viewbox if you change what the percentages are relative to.
I think nsSVGViewBox should be able to represent a "none" value somehow, which I guess is what you mean by "invalid". One way to do that would be to use a negative width or height. Add an IsValid() method. That seems like the right approach here.
Looks like the parsing logic and even the DOM interfaces don't prevent negative values from being passed in so we can't use that as the "no viewbox" flag. There is some checking of negative values in GetLength and GetViewboxToViewportTransform that prevent negative's from propagating anywhere.
If you want to distinguish "no viewbox" from "error", then I guess we can just use a flag.
Attachment #361211 - Attachment is obsolete: true
Attachment #361227 - Flags: review?(roc)
Attachment #361211 - Flags: review?(roc)
The above patch depends on bug 477530
Depends on: 477530
Comment on attachment 361227 [details] [diff] [review]
Patch that uses a valid flag instead of relying on HasAttr

Add a comment explaining what IsValid means.
And make IsValid const please.
Re-requesting review since I merged in the patch for bug 477530. Also fixed it so calling SetBaseValue with aDoSetAttr being PR_FALSE doesn't mark it as having a base value.
Attachment #361227 - Attachment is obsolete: true
Attachment #361475 - Flags: review?(roc)
Duplicate of this bug: 477530
+  mHasBaseVal |= aDoSetAttr;

> fixed it so calling SetBaseValue with aDoSetAttr being PR_FALSE doesn't mark it
> as having a base value.

Why don't we want to mark it?
Well the one place that uses SetBaseValue with PR_FALSE is in marker element where it tries to morph the viewbox to match the width and height if it doesn't have one. In that case it doesn't really have a viewbox, but marker element also doesn't call IsValid and needs some cleanup anyway because the fake viewbox system can't work anyway. So in the end the aDoSetAttr should probably be removed from SetBaseValue when marker element is fixed. Which I plan to do soon.

So really is doesn't much matter right now whether I force mHasBaseVal to PR_TRUE or OR in aDoSetAttr.
Let's go with PR_TRUE there since if we're setting the baseval rectangle to some values, it doesn't really make sense for mHasBaseVal to be false.
Still missing const on IsValid FWIW.

Also nsSVGViewBox::SetBaseValueString could just call nsSVGViewBox::SetBaseValue
Attachment #361475 - Attachment is obsolete: true
Attachment #362184 - Flags: review?(roc)
Attachment #361475 - Flags: review?(roc)
I think this patch causes reftest failures in the svg sizing reftests.
I've rerun this patch with a fresh trunk and I can't reproduce the reftest issues I saw before.
checked in
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.