Closed Bug 861188 Opened 7 years ago Closed 7 years ago

Firefox 20 makes svg-edit place opened SVG images in lower-right corner instead of center

Categories

(Core :: SVG, defect)

20 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox20 --- wontfix
firefox21 --- verified
firefox22 --- fixed
firefox23 --- verified

People

(Reporter: ed, Assigned: longsonr)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130409194949

Steps to reproduce:

Using the latest version of svg-edit (http://code.google.com/p/svg-edit/):

http://svg-edit.googlecode.com/svn/branches/2.6/editor/svg-editor.html

Click the "SVG-Edit" menu button, then "Open Image".  Open any .svg file.


Actual results:

In Firefox 20, the image appears off the canvas to the bottom right (you may have to scroll right and down to see it).


Expected results:

In Firefox 19 and earlier, the image appeared on the canvas.

See also:

http://code.google.com/p/svg-edit/issues/detail?id=1077
http://code.google.com/p/svg-edit/issues/detail?id=1080
http://code.google.com/p/svg-edit/issues/detail?id=1081
If this has changed, perhaps you can obtain a regression range? There's some instructions here: http://mozilla.github.io/mozregression/
Last good nightly: 2013-01-06
First bad nightly: 2013-01-07

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20d1a5916ef6&tochange=605ae260b7c8
Very likely to be one of dzbarsky's patches. Unfortunately a large number of patches were landed together rather than breaking them up, so it could be a pain to figure out what broke things exactly.

I'd think that the fastest way to find the underlying problem is probably to reduce an instance of svg-edit that shows this problem to a minimal testcase. Anyone able to do that?
(I can repro on linux, BTW. Generalizing 'platform' fields, and clarifying bug summary.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Summary: Firefox 20 breaks svg-edit → Firefox 20 makes svg-edit place opened SVG images in lower-right corner instead of center
hg bisect says the first bad revision is:

changeset:   117773:0515cea84af1
user:        David Zbarsky <dzbarsky@gmail.com>
date:        Sun Jan 06 01:25:52 2013 -0500
summary:     Bug 824229 Part 2: SVGGraphicsElement should implement SVGTests r=longsonr
Blocks: 824229
OK - so I believe the "regression" actually exposed an underlying bug, rather than introducing a new bug.

I can trigger the exact same bug in old builds (e.g. the parent of the cset in comment 6) by performing the STR and then running this in the web console:
>  document.getElementById("svgcontent").setAttribute("transform", "")

This sets an explicit empty "transform" attribute on the imported SVG file's root <svg> node, which makes it return true from IsTransformed(), which makes us call nsDisplayTransform::TransformRect() on its overflow rects in e.g. nsIFrame::FinishAndStoreOverflow() and incorrectly use a transform generated from its "x" and "y" values in those overflow rects.

In new builds, we trigger this without the need for the web-console tweak because for some reason we already have an empty transform list on our <svg> element (rather than no transform list), which makes us return true (i.e. non-null) from the content->GetAnimatedTransformList() call in  nsSVGDisplayContainerFrame::IsSVGTransformed(), which makes us return true from nsIFrame::IsTransformed.

If I change that GetAnimatedTransformList() call to also check IsExplicitlySet, then it "fixes" the regression, but (like old builds) we still suffer from the bug if you explicitly set an empty transform value in the web console.
Attached patch strawman patch v1 (obsolete) — Splinter Review
This patch fixes the svg-edit bug, even after setAttribute("transform", ""), by checking for an actual non-empty transform list in nsSVGDisplayContainerFrame::IsSVGTransformed().

While we might want to take this, we probably should fix the underlying bug, too -- seems like we *should* be able to return true from IsSVGTransformed() without messing up the rendering.
We're supposed to ignore the x and y values of outer SVG elements always. Are we not doing that if a transform is set?
In this case, the loaded file's outermost SVG element ends up becoming an *inner* <svg> element, inside of svg-edit's own outer SVG element.

So, the svg element involved is an inner svg element, not an outer one.
Attached patch patchSplinter Review
The transformHint change is because we can't send a transform change hint to something that doesn't have a transform and if we're saying that having an empty transform means IsTransformed() is false we need to be consistent (or we get asserts).
Attachment #737040 - Attachment is obsolete: true
Attachment #737136 - Flags: review?(dholbert)
https://tbpl.mozilla.org/?tree=Try&rev=9d3bb091c221

And if you want to play along at home, once completed, builds and logs will be available at:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/longsonr@gmail.com-9d3bb091c221
And yes, we'll still need a follow up to make transform work properly on <svg> elements.
Comment on attachment 737136 [details] [diff] [review]
patch

>diff --git a/content/svg/content/src/SVGAnimatedTransformList.h b/content/svg/content/src/SVGAnimatedTransformList.h
>--- a/content/svg/content/src/SVGAnimatedTransformList.h
>+++ b/content/svg/content/src/SVGAnimatedTransformList.h
>@@ -66,16 +66,23 @@ public:
>   bool IsExplicitlySet() const;
> 
>+  /**
>+   * Returns true if the corresponding "transform" attribute defined a
>+   * transform. Returns false if the transform was set to an invalid string.
>+   */
>+  bool HasTransform() const
>+    { return (mAnimVal && !mAnimVal->IsEmpty()) || !mBaseVal.IsEmpty(); }

Two things:
 (1) The documentation could use a bit of massaging -- it implies that we know that the transform attribute has been set, but we don't actually know that.  Maybe replace with something like this:
       Returns true if the corresponding transform attribute is set (or animated) to a
       valid value, such that we have at least one transform in our list. Returns false
       otherwise (e.g. if the transform attribute is missing or empty or invalid).

 (2) It's not clear to the casual reader how "HasTransform()" differs from "IsExplicitlySet()" or when you should use one instead of the other.  It'd be worth adding a comment above IsExplicitlySet to indicate what it does, and also adding a note somewhere to call out the intended difference between these two methods and/or when you might want one or the other.

>+++ b/layout/svg/nsSVGForeignObjectFrame.cpp
>@@ -187,17 +187,19 @@ nsSVGForeignObjectFrame::IsSVGTransforme
>   nsIFrame *parent = GetParent();
>   if (parent &&
>       parent->IsFrameOfType(nsIFrame::eSVG | nsIFrame::eSVGContainer)) {
>     foundTransform = static_cast<nsSVGContainerFrame*>(parent)->
>                        HasChildrenOnlyTransform(aFromParentTransform);
>   }
> 
>   nsSVGElement *content = static_cast<nsSVGElement*>(mContent);
>-  if (content->GetAnimatedTransformList()) {
>+  SVGAnimatedTransformList* transformList =
>+    content->GetAnimatedTransformList();
>+  if (transformList && transformList->HasTransform()) {

I wonder why this doesn't check for content->GetAnimateMotionTransform(), like the other chunks in this patch -- perhaps that's a as-yet-undiscovered bug?  That might need a followup.

r=me with the documentation addressed & a followup filed (or at least your opinion on whether a followup would be appropriate -- I can file the followup, but I wanted a sanity-check first :))

HOWEVER: I sort of think we should fix the underlying transform rendering issue first.  Per the end of comment 8, I'd expect we should be able to overzealously say "yes we're transformed" even when we're not, and still get correct (albeit less efficient) behavior.  I worry slightly that this patch papers over the only way we know of triggering that underlying bug, and if we take this patch now, we won't be able to tell whether we've fixed that underlying bug.  (and it might crop up in another way at some point in the future)
Attachment #737136 - Flags: review?(dholbert) → review+
>  (2) It's not clear to the casual reader how "HasTransform()" differs from
> "IsExplicitlySet()" or when you should use one instead of the other.  It'd
> be worth adding a comment above IsExplicitlySet to indicate what it does,
> and also adding a note somewhere to call out the intended difference between
> these two methods and/or when you might want one or the other.

if we have transform="" then IsExplicitlySet is true but HasTransform is false. They are the same otherwise. We may want to expand HasTransform in future to return false if the transform was the identity transform perhaps.

> 
> I wonder why this doesn't check for content->GetAnimateMotionTransform(),
> like the other chunks in this patch -- perhaps that's a as-yet-undiscovered
> bug?  That might need a followup.

That would be yet another bug that requires a follow up. I suspect I can write a reftest for that one though.

> 
> HOWEVER: I sort of think we should fix the underlying transform rendering
> issue first.  Per the end of comment 8, I'd expect we should be able to
> overzealously say "yes we're transformed" even when we're not, and still get
> correct (albeit less efficient) behavior.  I worry slightly that this patch
> papers over the only way we know of triggering that underlying bug, and if
> we take this patch now, we won't be able to tell whether we've fixed that
> underlying bug.  (and it might crop up in another way at some point in the
> future)

If you want to look under the wallpaper, take a firefox with my patch in, load svg-edit and then using DOM Inspector give the svgcontent element a transform of translate(0,0). While that isn't exactly a reduced testcase it is STR post bugfix.
Given we still know how to break things I landed this...

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e5eb8741ee
(In reply to Robert Longson from comment #16)
> give the svgcontent element a
> transform of translate(0,0)

Good point -- we didn't add a special-case for a no-op transform, so that should still trigger the underlying bug. Maybe file a followup on that?

(In reply to Robert Longson from comment #17)
> Given we still know how to break things I landed this...

Sounds good.
Assignee: nobody → longsonr
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/f1e5eb8741ee
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 737136 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 824229
User impact if declined: svg-edit a popular SVG editor is unusable
Testing completed (on m-c, etc.): landed on m-c. Feedback from svg-edit users is that it fixes this issue: https://groups.google.com/forum/?fromgroups=#!topic/svg-edit/6qhMS-Utkxs
Risk to taking this patch (and alternatives if risky): not entirely without risk as there's a few lines there but it's pretty low.
String or IDL/UUID changes made by this patch: none
Attachment #737136 - Flags: approval-mozilla-beta?
Attachment #737136 - Flags: approval-mozilla-aurora?
Just to follow up Roberts comment, I've tested the Original Posters process using the following Firefox nightly on OSX-Mountain Lion (10.8.3):

Firefox 23.0a1 (2013-04-15)

On the svg-edit trunk branch:
http://svg-edit.googlecode.com/svn/trunk/editor/svg-editor.html

I was able to successfully use the application as with versions prior to Firefox 20.0

Thank you very much!
(In reply to Robert Longson from comment #20)
> Comment on attachment 737136 [details] [diff] [review]
> patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): bug 824229
> User impact if declined: svg-edit a popular SVG editor is unusable
> Testing completed (on m-c, etc.): landed on m-c. Feedback from svg-edit
> users is that it fixes this issue:
> https://groups.google.com/forum/?fromgroups=#!topic/svg-edit/6qhMS-Utkxs
> Risk to taking this patch (and alternatives if risky): not entirely without
> risk as there's a few lines there but it's pretty low.
 Looks good to get uplifted given the verification in comment 21. Please add qawanted if we need any additional verfiication on any other editors that may have been impacted here .
Attachment #737136 - Flags: approval-mozilla-beta?
Attachment #737136 - Flags: approval-mozilla-beta+
Attachment #737136 - Flags: approval-mozilla-aurora?
Attachment #737136 - Flags: approval-mozilla-aurora+
I don't think we know of any other impacted sites.

Flagging needinfo=me to make sure this gets landed on aurora.  (I suspect RyanVM will land it in a batch in the next few days, but I can take care of it if he doesn't get to it soonish.)
Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #15)
> I wonder why this doesn't check for content->GetAnimateMotionTransform(),
> like the other chunks in this patch -- perhaps that's a as-yet-undiscovered
> bug?  That might need a followup.

bug 862817
Duplicate of this bug: 863157
Verified fixed based on STR in comment 0 on FF 21b3 Win 7, Ubuntu 12.04 and Mac OS X 10.8.3
QA Contact: paul.silaghi
Blocks: 863205
Found a related issue - bug 863205
Attached patch reftestSplinter Review
Attachment #739946 - Flags: review?(dholbert)
Attached image testcase
Any red showing = failure
Keywords: testcase-wanted
(In reply to Daniel Holbert [:dholbert] from comment #18)
> Good point -- we didn't add a special-case for a no-op transform, so that
> should still trigger the underlying bug. Maybe file a followup on that?
> 

bug 863994
Keywords: testcase
Comment on attachment 739946 [details] [diff] [review]
reftest

>diff --git a/layout/reftests/svg/reftest.list b/layout/reftests/svg/reftest.list
>--- a/layout/reftests/svg/reftest.list
>+++ b/layout/reftests/svg/reftest.list
>@@ -295,16 +295,17 @@ pref(svg.text.css-frames.enabled,true) =
> == text-scale-01.svg text-scale-01-ref.svg
> HTTP(..) == text-scale-02.svg text-scale-02-ref.svg
> HTTP(..) == text-scale-03.svg text-scale-03-ref.svg
> == text-stroke-scaling-01.svg text-stroke-scaling-01-ref.svg
> == stroke-dasharray-and-pathLength-01.svg pass.svg
> == stroke-dasharray-and-text-01.svg stroke-dasharray-and-text-01-ref.svg 
> == stroke-linecap-square-w-zero-length-segs-01.svg pass.svg
> == stroke-linecap-square-w-zero-length-segs-02.svg pass.svg
>+== svg-transform-01.svg pass.svg
> == textPath-01.svg textPath-01-ref.svg

nit: put this higher up in reftest.list, after svg-in-foreignObject-02.xhtml:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/reftest.list#264

(This chunk of "stroke-*" tests in reftest.list are in the wrong spot, alphabetically; so, sticking "svg-*" after them initially looks like it makes sense, but not if you look at the broader reftest.list file)

r=me with that
Attachment #739946 - Flags: review?(dholbert) → review+
(nice work on spinning up testcases for this and for bug 863994, BTW!)
Duplicate of this bug: 864281
Verified on Windows XP using latest Aurora: Mozilla/5.0 (Windows NT 5.1; rv:23.0) Gecko/20130611 Firefox/23.0

Using the steps from comment #0 I am able to reproduce the problem on Firefox 20, but not on the latest aurora.
Status: RESOLVED → VERIFIED
Keywords: verifyme
This same bug has creeped into v23 and up. It is a problem in the nightly right now v 25.0a1 (2013-07-29) and in v23.0b8. It should be activated again. Just to reiterate, the mouse cursor does not follow an SVG element/object correctly.
David: it sounds like you're describing a different bug -- you seem to be describing an issue with the mouse cursor, and your comment is the first mention of "mouse" or "cursor" on this page.

Please file a new bug on the issue you're describing, here:
 https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=SVG
sorry - to clarify, in SVG-edit -  (Zoom at 100%), when clicking the left mouse button to select or place an object, the mouse cursor and the actual x/y do not match. New objects are placed down and right, or selections are down and right. THis does not occur in Chrome or IE, or FF v22. This does not happen when zoom is not 100% however.
Again, that has nothing to do with this bug. (aside from the fact that they're both about svg-edit)

Please file a new bug to track that, using the link I provided in Comment 39.
(In reply to Daniel Holbert [:dholbert] from comment #41)
> Again, that has nothing to do with this bug.

(or rather: there's no reason to assume it has anything to do with this bug.)
(In reply to David Berger from comment #40)
> sorry - to clarify, in SVG-edit -  (Zoom at 100%), when clicking the left
> mouse button to select or place an object, the mouse cursor and the actual
> x/y do not match. New objects are placed down and right, or selections are
> down and right. THis does not occur in Chrome or IE, or FF v22. This does
> not happen when zoom is not 100% however.
This sounds like bug 863205.
Ah, it does indeed. @David: never mind about filing a new bug -- what you're describing is bug 863205.
(In reply to Daniel Holbert [:dholbert] from comment #44)
> Ah, it does indeed. @David: never mind about filing a new bug -- what you're
> describing is bug 863205.

I don't think that's actually the same bug.  For bug 863205, you have to zoom in to observe the bug (see the instructions to reproduce), but for the bug which David Berger is describing, there is no need to zoom at all.  Also, bug 863205 is described as "a regression between FF 12 - 13", but the new bug is a regression between Firefox 22 and 23 (it's clearly not present in FF 22).
correction to my comment #40 - it happens when zoom is NOT 100%. Don't know on which bug, but it was merged into one of them back in Apr., fixed in v21,v22, and now has regressed in v23 thru nightly.
(In reply to David Berger from comment #46)
> correction to my comment #40 - it happens when zoom is NOT 100%.

Hmm, I'm seeing the bug at any zoom level, including 100%.
If it isn't bug 863205, please create another bug.
I believe they are the same. I won't be creating another bug for it, however I commented 863205 for help.
You need to log in before you can comment on or make changes to this bug.