1340715 - {inc}nsSVGOuterSVGFrame::GetPrefISize depends on results of previous layout

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 1162418 made nsSVGOuterSVGFrame::GetPrefISize call GetLogicalSize on ancestors going up the frame tree.  It's incorrect to do this, because it can depend on the results in a *previous* layout rather than the current one.


What led me to this code was trying to explain to fantasai the Firefox behavior in:
http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cdiv%20style%3D%22width%3A%20500px%3B%20border%3A%20magenta%20solid%20thick%22%3E%0A%3Cdiv%20style%3D%22float%3A%20left%3B%20border%3A%20orange%20solid%20thick%3B%22%3E%0A%3Csvg%20viewbox%3D%220%200%2010%2010%22%20style%3D%22display%3A%20block%22%3E%0A%20%3Crect%20fill%3Dblue%20width%3D100%25%20height%3D100%25%20%2F%3E%0A%3C%2Fsvg%3E%0A%3C%2Fdiv%3E
which is actually reasonable, and could probably be obtained by returning nscoord_MAX as the pref ISize, but I think this code is incorrect.

I'll try to write a testcase showing the bad results later.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: one example of incorrect incremental layout

If you remove the assignments to svg.style.fontWeight, the incremental layout bug goes away.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(of course, that testcase ends up showing buggy behavior in only one browser profile...)

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: one example of incorrect incremental layout

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that this nearly led the spec authors to specify ... it's not clear what ... but it might not have been implementable:
https://github.com/w3c/csswg-drafts/issues/951

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1340715 - Make SVG intrinsic sizing not stateful. r=dholbert,jfkthame,jwatt

This corrects bug 1162418 to make intrinsic size not depend on an
arbitrary container. We fall back to the default replaced object size to
avoid regressing bug 1162418.

This is unlike for images, but we've had compat reports to this regard,
and this is what Chromium does effectively, so seems somewhat
reasonable.

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1340715 - Change SVG max-content size for webcompat.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76c1fb5130dc
Change SVG max-content size for webcompat. r=emilio

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Steal the bug from emilio ;)

Comment 10

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/76c1fb5130dc

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/35755 for changes under testing/web-platform/tests

Comment 15

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 17

[Ksenia Berezina [:ksenia]]

Hello Ting-Yu, wonder if it's possible to uplift this patch to ESR?
(coming from https://github.com/webcompat/web-bugs/issues/114165 where a user is still facing the issue on esr)

Comment 18

[Robert Longson [:longsonr]]

That doesn't sound like a good idea given the regression of bug 1801581

Comment 19

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I agree with Robert. This bug has two regressions -- bug 1790167 and bug 1801581. I don't feel confident to uplift my patch to ESR.

Comment 20

[Ksenia Berezina [:ksenia]]

Good point, thanks!