Closed Bug 1550535 Opened 5 months ago Closed 5 months ago

Mark <svg> elements (nsSVGOuterSVGFrame) as reflow roots

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [qf:p2:responsiveness])

Attachments

(1 file)

In bug 1550511, there's an interactive SVG graph which changes its markup in response to cursor movements, in a way that requires a reflow -- and right now, it's triggering expensive "measuring" reflows in the rest of the page.

One way to ameliorate this would be to make the outer SVG frame into a dynamic reflow root. I'm pretty sure that outer-SVG-frames' sizing is not impacted by their descendants at all, so they should be safe to make into a dynamic reflow root (or perhaps even a "true" reflow root, as long as we're sure to mark them as needing to be reflowed by their parent whenever their width/height properties change).

I expect this will get us a pretty nice perf win on pages that have interactive SVG charts intermixed with other content.

Summary: Make <svg> elements dynamic reflow roots → Mark <svg> elements (nsSVGOuterSVGFrame) as dynamic reflow roots

Yeah, this seems reasonable now that we can do overflow updates on reflow roots (bug 1159042 patch 2).

But why dynamic? Why shouldn't they just be regular reflow roots?

Yeah, I think regular reflow roots would probably work (per comment 0 perhaps even a "true" reflow root).

--> removing "dynamic" from title.

Summary: Mark <svg> elements (nsSVGOuterSVGFrame) as dynamic reflow roots → Mark <svg> elements (nsSVGOuterSVGFrame) as reflow roots

The Try run looks good. Posted patch for review.

now that we can do overflow updates on reflow roots (bug 1159042 patch 2).

BTW, I tried to come up with an SVG-specific test for this, but I couldn't manage to create any overflowing content outside of an SVG element. Here's what I tested (with bbb/ccc attempting to be transformed to create overflow, but they just get clipped at the content-edge, i.e. at the black border):

<!DOCTYPE html>
<body>
  <svg style="border: 1px solid black; width: 100px">
    <foreignObject width="1000px" height="1000px">
      <div xmnls="http://www.w3.org/1999/xhtml"
           style="transform:translatex(50px)">aaa</div>
      <div xmnls="http://www.w3.org/1999/xhtml"
           style="transform:translatex(90px)">bbb</div>
      <div xmnls="http://www.w3.org/1999/xhtml"
           style="transform:translatex(-10px)">ccc</div>
    </foreignObject>
  </svg>
</body>
Assignee: nobody → dholbert
Status: NEW → ASSIGNED

(In reply to Daniel Holbert [:dholbert] from comment #5)

BTW, I tried to come up with an SVG-specific test for this, but I couldn't manage to create any overflowing content outside of an SVG element.

Indeed, nsSVGOuterSVGFrame::Reflow just forces the overflow areas to match its border-rect:


  // With regards to visual overflow, we always clip root-<svg> (see our
  // BuildDisplayList method) regardless of the value of the 'overflow'
  // property since that is per-spec, even for the initial 'visible' value. For
  // that reason there's no point in adding descendant visual overflow to our
  // own when this frame is for a root-<svg>. [...SNIP...]
  [...]
  aDesiredSize.SetOverflowAreasToDesiredBounds();

https://searchfox.org/mozilla-central/rev/8308eb7ea14318f53b55f3289c2bb9b712265318/layout/svg/nsSVGOuterSVGFrame.cpp#474-478,493

So even before we had the special reflow-root overflow-area-updating code, we would probably have been fine to mark outer SVG frames as reflow roots.

Whiteboard: [qf?] → [qf:p2:responsiveness]

I'll land this after the code freeze + merge have passed. (i.e. early next week)

Flags: needinfo?(dholbert)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/09ef16b1c51f
Mark outer svg frames as reflow roots. r=dbaron
Flags: needinfo?(dholbert)
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1556511
Duplicate of this bug: 734751
You need to log in before you can comment on or make changes to this bug.