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

RESOLVED FIXED in Firefox 34, Firefox OS v2.1

Status

()

Core
SVG
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment)

This is to fix the bug described in bug 997101 comment 64.
Created attachment 8484432 [details] [diff] [review]
patch
Attachment #8484432 - Flags: review?(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>
 }
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc20221b0f6

As well as addressing the review comments I added a regression test before pushing.
Depends on: 425954
Depends on: 755187
Depends on: 771520
https://hg.mozilla.org/mozilla-central/rev/4cc20221b0f6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Attachment #8484432 - Flags: approval-mozilla-aurora?
Comment on attachment 8484432 [details] [diff] [review]
patch

Approving for Aurora so that we can take the perf fix for bug 997101.
Attachment #8484432 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/42be358fe96f
status-b2g-v2.1: affected → fixed
status-firefox34: --- → fixed
status-firefox35: --- → fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.