1063073 - Make sure embedding elements that rely on an embedded SVG's intrinsic dimensions are resized if the SVG is late in loading

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

This is to fix the bug described in bug 997101 comment 64.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8484432 [details] [diff] [review]
patch

>diff --git a/layout/svg/nsSVGOuterSVGFrame.cpp b/layout/svg/nsSVGOuterSVGFrame.cpp
>+// helper
>+static inline bool
>+DependsOnIntrinsicSize(const nsIFrame* aEmbeddingFrame)
>+{
>+  const nsStylePosition *pos = aEmbeddingFrame->StylePosition();
>+  const nsStyleCoord &width = pos->mWidth;
>+  const nsStyleCoord &height = pos->mHeight;
>+
>+  // XXX it would be nice to know if the size of aEmbeddingFrame's containing
>+  // block depends on aEmbeddingFrame, then we'd know if we can return false
>+  // for eStyleUnit_Percent too.
>+  return !width.ConvertsToLength() ||
>+  !height.ConvertsToLength();

"!height" needs more indentation there. (It at least needs 2 spaces, or it can match the place you're moving it from, which had it aligned under "!width".)

> void
> nsSVGOuterSVGFrame::Init(nsIContent*       aContent,
>                          nsContainerFrame* aParent,
>                          nsIFrame*         aPrevInFlow)
> {
[...]
>+      nsIFrame* embeddingFrame;
>+      if (IsRootOfReplacedElementSubDoc(&embeddingFrame) && embeddingFrame) {
>+        if (DependsOnIntrinsicSize(embeddingFrame)) {
>+          // If this document loads after the embedding element has had its
>+          // reflow we need to schedule another reflow so that it will use our
>+          // intrinsic dimensions:
>+          embeddingFrame->PresContext()->PresShell()->
>+            FrameNeedsReflow(embeddingFrame, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY);
>+        }
>+      }

IIUC, we don't need this if embeddingFrame is already marked as dirty, right?

So, maybe wrap this in:
 if (NS_UNLIKELY(!embeddingFrame->HasAllStateBits(NS_FRAME_IS_DIRTY)) {
   // Looks like this document is loading after the embedding element has had
   // its first reflow, so we need to [...]
   <make call to FrameNeedsReflow>
 }

Comment 3

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

Might this fix a bunch of existing intermittent-orange bugs?  I seem to recall some related to SVG sizing.

Comment 4

[Jonathan Watt [:jwatt]]

There were a bunch that were filed as a result of the patch for bug 997101 landing, but those all went away when bug 997101 was backed out as a result. I don't think there are any existing orange bugs that this will fix.

Comment 5

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc20221b0f6

As well as addressing the review comments I added a regression test before pushing.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cc20221b0f6

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8484432 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: NA
[Describe test coverage new/current, TBPL]: Landed on m-c for a while
[Risks and why]: Little, this patch only add one additional SVG reflow when the SVG is loaded in a different timing.
[String/UUID change made/needed]: NA

This patch is need to uplift bug 997101

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8484432 [details] [diff] [review]
patch

Approving for Aurora so that we can take the perf fix for bug 997101.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/42be358fe96f