Closed
Bug 435525
Opened 16 years ago
Closed 15 years ago
Setting viewBox="" on a referenced SVG document from script does not cause an update
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: heycam, Assigned: craig.topper)
References
()
Details
Attachments
(1 file, 3 obsolete files)
6.83 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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>.
Updated•16 years ago
|
Updated•16 years ago
|
Version: unspecified → Trunk
Comment 1•15 years ago
|
||
This was fixed with the checkin in bug 374111 (the viewbox now gets updated).
Assignee: nobody → craig.topper
Status: UNCONFIRMED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Whiteboard: [fixed by bug 374111]
Assignee | ||
Updated•15 years ago
|
Status: RESOLVED → UNCONFIRMED
Resolution: FIXED → ---
Assignee | ||
Comment 2•15 years ago
|
||
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [fixed by bug 374111]
Assignee | ||
Comment 3•15 years ago
|
||
This patch removes DidChangeViewBox from nsSVGSVGElement and puts back UnsetAttr and AfterSetAttr to do the invalidation.
Attachment #361211 -
Flags: review?(roc)
Assignee | ||
Comment 4•15 years ago
|
||
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?
Assignee | ||
Comment 6•15 years ago
|
||
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.
Assignee | ||
Comment 8•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
Attachment #361211 -
Attachment is obsolete: true
Attachment #361227 -
Flags: review?(roc)
Attachment #361211 -
Flags: review?(roc)
Attachment #361227 -
Flags: superreview+
Attachment #361227 -
Flags: review?(roc)
Attachment #361227 -
Flags: review+
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.
Comment 13•15 years ago
|
||
And make IsValid const please.
Assignee | ||
Comment 14•15 years ago
|
||
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)
+ 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?
Assignee | ||
Comment 17•15 years ago
|
||
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.
Comment 19•15 years ago
|
||
Still missing const on IsValid FWIW. Also nsSVGViewBox::SetBaseValueString could just call nsSVGViewBox::SetBaseValue
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #361475 -
Attachment is obsolete: true
Attachment #362184 -
Flags: review?(roc)
Attachment #361475 -
Flags: review?(roc)
Attachment #362184 -
Flags: superreview+
Attachment #362184 -
Flags: review?(roc)
Attachment #362184 -
Flags: review+
Comment 21•15 years ago
|
||
I think this patch causes reftest failures in the svg sizing reftests.
Comment 22•15 years ago
|
||
I've rerun this patch with a fresh trunk and I can't reproduce the reftest issues I saw before.
Comment 23•15 years ago
|
||
checked in http://hg.mozilla.org/mozilla-central/rev/28da25d1e3ea
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•