938555 - SVG Elements not being rendered after manipulating transform

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jens Podzierski]

Attachment: Very simple testcase - open and click to reproduce

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36

Steps to reproduce:

Create arbitrary SVG element within a <g> element. Change transform of g and inner element in script, so element lies outside previous bounds of <g>.


Actual results:

Inner element is not being rendered. It is there in the HTML view of Firebug, but not visible in browser window.


Expected results:

Inner element should be rendered at new position.

Comment 1

[Paul Silaghi, QA [:pauly]]

Confirmed FF 28.0a1 (2013-11-13) win 7 x64 using the testcase

Comment 2

[Robert Longson [:longsonr]]

I suspect this used to work. Could someone try to get a regression window?

Comment 3

[Paul Silaghi, QA [:pauly]]

Last good nightly: 2013-05-08
First bad nightly: 2013-05-09

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b980d32c366f&tochange=ea059733677c

Comment 4

[Robert Longson [:longsonr]]

Looks like this is due to one of Jonathan's fixes that landed in the regression window.

Comment 5

[Alice0775 White]

Last Good: 7b7eea794c8e
First Bad: c0b7f22692c3

Regressed by:
	c0b7f22692c3	Jonathan Watt — Bug 869611 - Stop reflowing in nsSVGPathGeometryFrame::NotifySVGChanged for ancestor transform changes. r=dholbert

Comment 6

[Sebastian Müller]

This problem is actually not that contrived as it may seem at first glance. In fact it is currently affecting our customers (devs) and their users on FF and makes us/and/or FF look bad. The only feasible workaround I can see is to artificially increase the bounds of the subtree with invisible or almost invisible elements in order to increase the size of the clipping rectangle, which is not only ugly but very likely hurts performance, too. I can provide details on how to reproduce this problem easily in our online demo apps, but the test-case is perfect already. It's just that it looks so contrived, which is only due to it being a stripped down version of the actual problem.

Comment 7

[Robert Longson [:longsonr]]

If I flush the display i.e. change the code so it looks like this...

// change translate of outer g
outerG.transform.baseVal.getItem(0).setTranslate(20, 20);
document.rootElement.forceRedraw();
// move rect0 within the old bounds - this works as expected
//rect0.transform.baseVal.getItem(0).setTranslate(70, 70);

which internally does a doc->FlushPendingNotifications(Flush_Display); everything works.

In fact Flush_Style is enough, Flush_ContentAndNotify is not enough though.

It seems that the subsequent calls to adjust the transform use the wrong bounds somehow and when I look at the overflowAreas of the <g> element without the flush they are all wrong.

Is adding a flush somewhere in content the right thing to do Jonathan? If so where, if not what is the right thing to do. It seems it's only likely necessary for a <g> as other container elements such as <svg> have fixed sizes so I could maybe also check that Element() is a <g>

Comment 8

[Robert Longson [:longsonr]]

WFIW SVGTransform::NotifyElementDidChange seems like it might be the place to flush if a flush is needed.

Comment 9

[Robert Longson [:longsonr]]

Attachment: patch

Comment 10

[Robert Longson [:longsonr]]

Attachment: without the tabs

Comment 11

[Robert Longson [:longsonr]]

This patch also fixes bug 944886 and all its duplicates.

Comment 12

[Robert Longson [:longsonr]]

Attachment: turns out reflow will do

Comment 13

[Jonathan Watt [:jwatt]]

This looks wrong. The nsChangeHint_UpdateOverflow and nsChangeHint_UpdateTransformLayer hints should be sufficient. I'll take a look.

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: Simpler SVG testcase

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: Same, but HTML only

Comment 16

[Jonathan Watt [:jwatt]]

I tracked this down to something being rotten in OverflowChangedTracker::Flush, and Matt pointed me to bug 944291 where as it happens roc has just attached a patch. I built with that patch and it fixes the issue here.

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 8340775 [details] [diff] [review]
turns out reflow will do

It would be good to have your reftest as a separate patch.

Comment 18

[Robert Longson [:longsonr]]

Check-in for bug 944291 fixes this.

Comment 19

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=0994212349f9

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #19)
> https://tbpl.mozilla.org/?tree=Try&rev=0994212349f9

One nit on the cset in that Try push:

>+<svg xmlns="http://www.w3.org/2000/svg" class="reftest-wait" onload="startTest()">
>+  <script>
>+    function startTest() {
>+      document.addEventListener("MozReftestInvalidate", doTest, false);
>+    }

Normally in MozReftestInvalidate reftests, I don't think we bother using an onload handler to call addEventListener. (It's a bit simpler to just directly call addEventListener at the top level inside the <script> tag, IMO; it removes one layer of async callback.)

(Doesn't matter much, though.)

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed using the testcases on 29.0a1 (2014-01-07), win 7 x64.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Bug 944291 was uplifted for Fx28.

Comment 23

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8287550616ee

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8287550616ee

Comment 25

[u279076]

(In reply to Paul Silaghi, QA [:pauly] from comment #21)
> Verified fixed using the testcases on 29.0a1 (2014-01-07), win 7 x64.

Please also verify this in Aurora.

Comment 26

[Paul Silaghi, QA [:pauly]]

(In reply to Paul Silaghi, QA [:pauly] from comment #21)
> Verified fixed using the testcases on 29.0a1 (2014-01-07), win 7 x64.
Verified fixed 28.0a2 (2014-01-16)