Closed Bug 1063073 Opened 6 years ago Closed 6 years ago

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


(Core :: SVG, defect)

Not set



Tracking Status
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed


(Reporter: jwatt, Assigned: jwatt)




(1 file)

This is to fix the bug described in bug 997101 comment 64.
Attached patch patchSplinter Review
Attachment #8484432 - Flags: review?(dholbert)
Comment on attachment 8484432 [details] [diff] [review]

>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>
Attachment #8484432 - Flags: review?(dholbert) → review+
Might this fix a bunch of existing intermittent-orange bugs?  I seem to recall some related to SVG sizing.
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.

As well as addressing the review comments I added a regression test before pushing.
Depends on: 425954
Depends on: 755187
Depends on: 771520
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment on attachment 8484432 [details] [diff] [review]

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
Attachment #8484432 - Flags: approval-mozilla-aurora?
Comment on attachment 8484432 [details] [diff] [review]

Approving for Aurora so that we can take the perf fix for bug 997101.
Attachment #8484432 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.