If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: heycam, Assigned: Craig Topper)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed by bug 374111]
(Assignee)

Updated

9 years ago
Status: RESOLVED → UNCONFIRMED
Resolution: FIXED → ---
(Assignee)

Comment 2

9 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

9 years ago
Created attachment 361211 [details] [diff] [review]
Patch to restore AfterSetAttr and UnsetAttr for viewbox

This patch removes DidChangeViewBox from nsSVGSVGElement and puts back UnsetAttr and AfterSetAttr to do the invalidation.
Attachment #361211 - Flags: review?(roc)
(Assignee)

Comment 4

9 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

9 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

9 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

9 years ago
Created attachment 361227 [details] [diff] [review]
Patch that uses a valid flag instead of relying on HasAttr
Attachment #361211 - Attachment is obsolete: true
Attachment #361227 - Flags: review?(roc)
Attachment #361211 - Flags: review?(roc)
(Assignee)

Comment 11

9 years ago
The above patch depends on bug 477530
Depends on: 477530
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

9 years ago
And make IsValid const please.
(Assignee)

Comment 14

9 years ago
Created attachment 361475 [details] [diff] [review]
Cleaned up a couple minor things and merge in patch for bug 477530

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)
(Assignee)

Updated

9 years ago
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?
(Assignee)

Comment 17

9 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

9 years ago
Still missing const on IsValid FWIW.

Also nsSVGViewBox::SetBaseValueString could just call nsSVGViewBox::SetBaseValue
(Assignee)

Comment 20

9 years ago
Created attachment 362184 [details] [diff] [review]
Patch to address roc's and Robert's comments
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

9 years ago
I think this patch causes reftest failures in the svg sizing reftests.

Comment 22

9 years ago
I've rerun this patch with a fresh trunk and I can't reproduce the reftest issues I saw before.

Comment 23

9 years ago
checked in http://hg.mozilla.org/mozilla-central/rev/28da25d1e3ea
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.