734751 - Make nsSVGOuterSVGFrame a reflow root

Status: RESOLVED DUPLICATE of bug 1550535

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Bug 734079 is implementing a kind of lightweight reflow for SVG to update the bounds of SVG frames asynchronously. To prevent us doing unnecessary work, I want to make nsSVGOuterSVGFrame a reflow root, since there's no need to mark ancestors of nsSVGOuterSVGFrame as being dirty when descendants of nsSVGOuterSVGFrame change. Unfortunately, the simple act of adding NS_FRAME_REFLOW_ROOT to nsSVGOuterSVGFrame breaks things.

What breaks is that changes to the width (or height) _property_ on outer-<svg> no longer have any effect. (Changes to the width (or height) attribute work.)

The problem seems to be that the bit passed to FrameNeedsReflow with the nsSVGOuterSVGFrame is NS_FRAME_HAS_DIRTY_CHILDREN, not NS_FRAME_IS_DIRTY as I'd expect for a 'width' property change. The stack for that is:

  PresShell::FrameNeedsReflow
  nsCSSFrameConstructor::StyleChangeReflow
  nsCSSFrameConstructor::ProcessRestyledFrames
  nsCSSFrameConstructor::RestyleElement
  mozilla::css::RestyleTracker::ProcessOneRestyle
  mozilla::css::RestyleTracker::DoProcessRestyles
  mozilla::css::RestyleTracker::ProcessRestyles

This means that in FrameNeedsReflow FRAME_IS_REFLOW_ROOT(_frame_) evaluates to true for the nsSVGOuterSVGFrame when it's passed in (it would be false if the bit was NS_FRAME_IS_DIRTY) and we end up inside this |if|:

http://hg.mozilla.org/mozilla-central/file/c33438bd5706/layout/base/nsPresShell.cpp#l2756

That means we never set NS_FRAME_HAS_DIRTY_CHILDREN on the nsSVGOuterSVGFrame's ancestors, since we hit the |break| at the end of the |if|, and somehow that means we end up with aReflowState.ComputedWidth() returning the old width in nsSVGOuterSVGFrame::Reflow.

It seems to me that the problem is that we should be getting NS_FRAME_IS_DIRTY passed in to FrameNeedsReflow for a change to the 'width' property on outer-<svg>. The reason we don't is because the StyleChangeReflow call in the stack above is only passed an aHint of nsChangeHint_NeedReflow and nsChangeHint_ClearAncestorIntrinsics, so it decides to pass NS_FRAME_HAS_DIRTY_CHILDREN to FrameNeedsReflow. I'm not sure if those hints are wrong.

Boris, David, do you have any thoughts on this before I potentially spend time investigating/fixing the wrong thing? :)

Comment 1

[Jonathan Watt [:jwatt]]

Of course it would help if I CC'ed Boris and David.

Boris, David, please see the question above.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

Not passing in NS_FRAME_IS_DIRTY is quite purposeful.  See bug 502288 and the comments in the diff there.  We don't want to force reflow of all the descendants, which is what NS_FRAME_IS_DIRTY would do.

The real issue is that NS_FRAME_HAS_DIRTY_CHILDREN is being overloaded here to mean both "the actual thing that needs reflowing is your descendants" and "not NS_FRAME_IS_DIRTY".  Or perhaps that NS_FRAME_IS_DIRTY is being overloaded to mean both "you need reflowing" and "all your descendants need reflowing".  Whichever way you want to look at it.

The right way to fix this is probably to somehow communicate to FrameNeedsReflow that the aFrame being passed in should really not be treated as a reflow root for purposes of this FrameNeedsReflow call.

Would it make any sense to trigger this off of aIntrinsicDirty != eResize?  I suspect that would do the right thing, but is a bit ... non-obvious.  And that change would affect the short-circuit in the existing aIntrinsicDirty != eResize block.  So probably better to communicate what's going on explicitly, either by adding another argument or changing the aBitToAdd argument somehow.

Comment 4

[Jonathan Watt [:jwatt]]

Where is NS_FRAME_IS_DIRTY supposed to be treated as "all your descendants need reflowing"? The only handling of descendants that PresShell::FrameNeedsReflow does is to mark their intrinsics dirty if eStyleChange is passed in. It doesn't seem to do anything for descendants based on NS_FRAME_IS_DIRTY.

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #3)
> The real issue is that NS_FRAME_HAS_DIRTY_CHILDREN is being overloaded here
> to mean both "the actual thing that needs reflowing is your descendants" and
> "not NS_FRAME_IS_DIRTY".  Or perhaps that NS_FRAME_IS_DIRTY is being
> overloaded to mean both "you need reflowing" and "all your descendants need
> reflowing".  Whichever way you want to look at it.

So NS_FRAME_HAS_DIRTY_CHILDREN means "all your descendants need reflowing" and NS_FRAME_IS_DIRTY means "you + all your descendants need reflowing"?

I also don't think I clearly understand the difference between "the frame's intrinsic dimensions need updating" and "the frame needs reflowed". The former would imply the latter, but not vice versa. Is the only case that can cause the latter without causing the former a size change on an ancestor?

Comment 5

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

(In reply to Jonathan Watt [:jwatt] from comment #4)
> So NS_FRAME_HAS_DIRTY_CHILDREN means "all your descendants need reflowing"
> and NS_FRAME_IS_DIRTY means "you + all your descendants need reflowing"?

No.  NS_FRAME_IS_DIRTY means that a frame and all its descendants need to be reflowed.  

NS_FRAME_HAS_DIRTY_CHILDREN means that either:
 (a) at least one of the frame's children has NS_FRAME_IS_DIRTY or NS_FRAME_HAS_DIRTY_CHILDREN
 (b) one of the frame's children has been removed since the last reflow
 (c) a style change has happened that requires the frame to be reflowed but does not require its descendants to be reflowed (e.g., 'height', 'width', 'margin', etc.)

In other words, it means that children can (in many cases, but not all) be skipped during reflow if they don't have either bit set.  But the child list still needs to be walked to update positions after any children that change size or any that have been removed.

> I also don't think I clearly understand the difference between "the frame's
> intrinsic dimensions need updating" and "the frame needs reflowed". The
> former would imply the latter, but not vice versa. Is the only case that can
> cause the latter without causing the former a size change on an ancestor?

Yes, the former does imply the latter.

Examples of things that require a frame to be reflowed but don't require its intrinsic dimensions to be updated are:

 * a change to its 'height', 'width', 'margin', etc. properties (which requires updating intrinsic dimensions on its *ancestors* (generally a small set) but not on itself or its *descendants* (which can be a large set))

 * changes to things like vertical alignment, justification

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to David Baron [:dbaron] from comment #5)
>  (c) a style change has happened that requires the frame to be reflowed but
> does not require its descendants to be reflowed (e.g., 'height', 'width',
> 'margin', etc.)

I don't follow how these example properties do not require descendants to be reflowed. If I have:

  <div style="width: 50px;">
    <div style="width: 50%;"/>
  </div>

doesn't a change to the width of the outer div require the inner div to also be reflowed to re-resolve its percentage width?

> In other words, it means that children can (in many cases, but not all) be
> skipped during reflow if they don't have either bit set.  But the child list
> still needs to be walked to update positions after any children that change
> size or any that have been removed.

By the second sentence, do you simply mean that if a frame's Reflow() calls Reflow() on any of its direct children causing their size or position to change, or if any of its direct children were removed, then the frame's Reflow() method will have to look at all the direct children in order to figure out the size/position of the frame itself? Or do you mean something less straightforward?

> Examples of things that require a frame to be reflowed but don't require its
> intrinsic dimensions to be updated are:
> 
>  * a change to its 'height', 'width', 'margin', etc. properties (which
> requires updating intrinsic dimensions on its *ancestors* (generally a small
> set) but not on itself or its *descendants* (which can be a large set))

I think I misunderstand what's meant by "intrinsic dimensions" - I thought this term meant the dimensions a frame would take up if its parent was infinite in size. So if a div is set to style="width:50px" and is then set to style="width:100px" then its intrinsic dimensions would change from 50px to 100px. If not, then what does it mean?

Also, is "intrinsic dimensions" a replaced element only concept? The comment for nsIFrame::GetIntrinsicSize() hints that it might be, but maybe it's just the method that's replaced element specific.

Comment 7

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

(In reply to Jonathan Watt [:jwatt] from comment #6)
> (In reply to David Baron [:dbaron] from comment #5)
> >  (c) a style change has happened that requires the frame to be reflowed but
> > does not require its descendants to be reflowed (e.g., 'height', 'width',
> > 'margin', etc.)
> 
> I don't follow how these example properties do not require descendants to be
> reflowed. If I have:
> 
>   <div style="width: 50px;">
>     <div style="width: 50%;"/>
>   </div>
> 
> doesn't a change to the width of the outer div require the inner div to also
> be reflowed to re-resolve its percentage width?

Reflow still has code throughout to handle responding to things resizing -- during reflow, if a parent resizes, it will reflow all of its children to update themselves for the new size.  But they, in turn, might choose *not* to reflow all of their children if they don't resize.  (It's important to make resizes fast for cases like window resizing, sidebars or the tab strip appearing/disappearing, etc.)  See nsHTMLReflowState::ShouldReflowAllKids().

> > In other words, it means that children can (in many cases, but not all) be
> > skipped during reflow if they don't have either bit set.  But the child list
> > still needs to be walked to update positions after any children that change
> > size or any that have been removed.
> 
> By the second sentence, do you simply mean that if a frame's Reflow() calls
> Reflow() on any of its direct children causing their size or position to
> change, or if any of its direct children were removed, then the frame's
> Reflow() method will have to look at all the direct children in order to
> figure out the size/position of the frame itself? Or do you mean something
> less straightforward?

I guess I wasn't as precise as I should have been.  If a frame's Reflow method is called, I believe that in general it will look at all the children and recompute the child positions and its own size from them.  But whether it reflows the children depends on whether it is dirty or resizing, or whether the child is dirty or has dirty children.

> > Examples of things that require a frame to be reflowed but don't require its
> > intrinsic dimensions to be updated are:
> > 
> >  * a change to its 'height', 'width', 'margin', etc. properties (which
> > requires updating intrinsic dimensions on its *ancestors* (generally a small
> > set) but not on itself or its *descendants* (which can be a large set))
> 
> I think I misunderstand what's meant by "intrinsic dimensions" - I thought
> this term meant the dimensions a frame would take up if its parent was
> infinite in size. So if a div is set to style="width:50px" and is then set
> to style="width:100px" then its intrinsic dimensions would change from 50px
> to 100px. If not, then what does it mean?

Roughly, yes, except that the intrinsic width of a frame is *not* affected by its own 'width' property.  However, the intrinsic width of its parent *is* affected by the child's 'width' property.  This allows things like 'width: min-content' that otherwise wouldn't make any sense.

(But there are two intrinsic widths:  max-content roughly being what you get by laying each paragraph out all on one line, and min-content being what you get if the width of a paragraph is the width of its longest word.)

> Also, is "intrinsic dimensions" a replaced element only concept? The comment
> for nsIFrame::GetIntrinsicSize() hints that it might be, but maybe it's just
> the method that's replaced element specific.

It's just the method.  Replaced elements, however, also have a concept of an intrinsic ratio.

Comment 8

[Jonathan Watt [:jwatt]]

Thanks for the great help clarifying how reflow and the dirty bits work, bz, dbaron. I've spun a patch out to integrate that great info into documenting comments in bug 78638.

Comment 9

[Jonathan Watt [:jwatt]]

Err, bug 786387, rather.

Comment 10

[Jonathan Watt [:jwatt]]

Coming back to this bug, the whole dirty bit thing seems to be a red herring. Here's what I think the real issue is:

If nsSVGOuterSVGFrame is _not_ a reflow root, then the nsHTMLReflowState that is passed to nsSVGOuterSVGFrame::Reflow is initialized by calling nsSVGOuterSVGFrame::ComputeSize. The stack looks something like:

  nsSVGOuterSVGFrame::ComputeSize
  nsHTMLReflowState::InitConstraints
  nsHTMLReflowState::Init
  nsHTMLReflowState::nsHTMLReflowState
  nsHTMLReflowState::nsHTMLReflowState
  nsBlockReflowContext::ComputeCollapsedTopMargin
  nsBlockFrame::ReflowBlockFrame
  nsBlockFrame::ReflowLine
  nsBlockFrame::ReflowDirtyLines
  nsBlockFrame::Reflow

nsSVGOuterSVGFrame::ComputeSize takes the change to the width/height property into account, so when nsSVGOuterSVGFrame::Reflow is called it computes the new width/height as expected.

However, if nsSVGOuterSVGFrame _is_ a reflow root, then nsSVGOuterSVGFrame::ComputeSize is never called, and instead the nsHTMLReflowState is initialized with the pre-reflow size of the nsSVGOuterSVGFrame here:

http://hg.mozilla.org/mozilla-central/file/271ca35d7645/layout/base/nsPresShell.cpp#l7382

Stack:

  PresShell::DoReflow
  PresShell::ProcessReflowCommands

Not surprising then that the size of the nsSVGOuterSVGFrame doesn't change as it should.

It's not clear to me exactly why this code is doing what it's doing, and therefore what the correct way to fix this is.

Comment 11

[Jonathan Watt [:jwatt]]

bz pointed out on IRC that one of the invariants when starting a reflow at a reflow root is that the root itself isn't changing size, so actually in the case of a width/height change we do not want to start the reflow at the nsSVGOuterSVGFrame since it should resize. In other words, this is still a dirty bit issue - we want to do dirty marking up the nsSVGOuterSVGFrame's parent chain to the next reflow root in this case.

Comment 12

[Jonathan Watt [:jwatt]]

The way of fixing this that would seem to make most sense to me would be to:

 1) rename NS_FRAME_IS_DIRTY to NS_FRAME_IS_DIRTY_TREE, meaning
    "this frame and _all_ its descendants need reflowed"

 2) have a new NS_FRAME_IS_DIRTY that means just "this frame needs reflowed",
    and use that bit for width/height and other changes where appropriate
    instead of misleadingly using NS_FRAME_HAS_DIRTY_CHILDREN

 3) use NS_FRAME_HAS_DIRTY_CHILDREN only for (a) and (b) from dbaron's
    comment 5

Comment 13

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

Ignoring the name changes, it sounds like the substantive change proposed in comment 12 is that items (a) and (b) in comment 5 should be treated differently from item (c).  But it's not clear to me what difference you're proposing, only that there's going to be a difference.

If the difference you're proposing is that we change the |targetFrameDirty| assignment in PresShell::FrameNeedsReflow so that case (c) in comment 5 also leads to targetFrameDirty being true, then I think that sounds great.

But I'm not sure that difference needs to be reflected permanently with another bit that we store in the frame tree; I don't see why we'd need to make any later distinctions between these states.  So I'd rather keep just two bits.

Can you accomplish the different behavior of |targetFrameDirty| with a different parameter to FrameNeedsReflow instead?

I think this problem is basically a regression from https://hg.mozilla.org/mozilla-central/rev/74fb7057f4dd , and I think it's probably observable on other reflow roots as well; we just didn't notice it.

Comment 14

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

(In reply to David Baron [:dbaron] from comment #13)
> I think this problem is basically a regression from
> https://hg.mozilla.org/mozilla-central/rev/74fb7057f4dd , and I think it's
> probably observable on other reflow roots as well; we just didn't notice it.

... and in particular, a regression because it's not compatible with the earlier optimization made in https://hg.mozilla.org/mozilla-central/rev/b87cba34d6c7

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: testcase for textarea

(In reply to David Baron [:dbaron] from comment #13)
> I think this problem is basically a regression from
> https://hg.mozilla.org/mozilla-central/rev/74fb7057f4dd , and I think it's
> probably observable on other reflow roots as well; we just didn't notice it.

The only NS_FRAME_REFLOW_ROOT frame that content seems to have access to is nsTextControlFrame, but this problem doesn't seem to be observable for that frame type.

Comment 16

[Jonathan Watt [:jwatt]]

Wait, it's the scroll frame child that NS_FRAME_REFLOW_ROOT is set on. Not really sure if I have access to set the width of that directly though. Or if there's some otherway to reproduce the regression for textarea.

Comment 17

[Jonathan Watt [:jwatt]]

(In reply to David Baron [:dbaron] from comment #13)
> But I'm not sure that difference needs to be reflected permanently with
> another bit that we store in the frame tree; I don't see why we'd need to
> make any later distinctions between these states.

It just seems undesirable to me that the name NS_FRAME_HAS_DIRTY_CHILDREN should be used when in fact the children are not dirty, but rather the frame itself is dirtied in a way that doesn't necessarily require its children to be reflowed at all.

> So I'd rather keep just two bits.

Okay.

> Can you accomplish the different behavior of |targetFrameDirty| with a
> different parameter to FrameNeedsReflow instead?

I'll give that a go.

Comment 18

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

Yeah, I think we could probably have better names for the bits.  But I think that's separate from the functional changes.