Closed Bug 1169440 Opened 9 years ago Closed 9 years ago

changes to 'width', etc., on reflow roots don't work correctly

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(7 files)

patch 1 - Add bitwise operators to nsChangeHint
3.47 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 2 - Rename style struct MaxDifferenceNeverInherited to DifferenceAlwaysHandledForDescendants
25.82 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 3 - Add nsChangeHint_ReflowChangesSizeOrPosition
26.92 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 4 - Add comment to nsStylePadding::DifferenceAlwaysHandledForDescendants
1.28 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 5 - Adjust hints in CalcDifference methods to emit new nsChangeHint_ClearAncestorIntrinsics hint
2.65 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 6 - Add parameter to nsIFrame::FrameNeedsReflow to control handling of target being a reflow root
5.66 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
patch 7 - Pass parameter determined from style hint to FrameNeedsReflow
1.63 KB, patch
dholbert
: review+
Details | Diff | Splinter Review 
This is a regression from bug 502288, which doesn't play well with the optimization added in bug 378784.

In bug 1159042 comment 3 I wrote:
> OK, looked into the failure of:
>  
> layout/reftests/position-dynamic-changes/horizontal/leftA-widthN-rightA.html
> in that try run, and I think the problem is the following:
> 
> The test case is dynamically changing 'width' on an absolutely positioned
> element with both 'height' and 'width' set, and thus subject to this
> optimization.  Per nsStylePosition::CalcDifference, the change hint for
> 'width' is:  NS_SubtractHint(nsChangeHint_AllReflowHints,
>                                      
> NS_CombineHint(nsChangeHint_ClearDescendantIntrinsics,
>                                                     
> nsChangeHint_NeedDirtyReflow))).
> This is equivalent to nsChangeHint_NeedReflow |
> nsChangeHint_ClearAncestorIntrinsics.  Note that
> nsChangeHint_NeedDirtyReflow is not set.  This means that
> RestyleManager::StyleChangeReflow calls PresShell::FrameNeedsReflow(aFrame,
> nsIPresShell::eTreeChange, NS_FRAME_HAS_DIRTY_CHILDREN).  Because it's
> NS_FRAME_HAS_DIRTY_CHILDREN and not NS_FRAME_IS_DIRTY,
> PresShell::FrameNeedsReflow sets targetFrameDirty to false, which means it
> uses the reflow root optimization in this case, where it's actually not
> valid.
> 
> We need a way to know that these geometry style changes that don't set
> NS_FRAME_IS_DIRTY to still have targetFrameDirty true in FrameNeedsReflow so
> that we don't do the reflow root behavior in that case.
> 
> I believe this is a preexisting bug related to changing 'width' and similar
> properties on reflow roots.

I'm filing this bug to cover this problem, separately from bug 1159042.
Interestingly, I think there are a bunch of style changes where we do set NS_FRAME_IS_DIRTY, but where we can make the reflow-root optimization, e.g., for font-size.  I'm not sure that any of them are important performance-wise, though.

One option is to add a tri-state parameter to FrameNeedsReflow for whether reflow root optimizations are valid for the target -- infer-from-state-bit, valid, and invalid.  We could then add use infer-from-state-bit for everything other than style changes, and have style changes pass valid and invalid directly.

This requires new style change hints to cause that.  The next question is whether those style change hints are interesting in any new ways (e.g., in terms of NS_HintsNotHandledForDescendantsIn and nsChangeHint_Hints_NotHandledForDescendants).  If they are, it might not be worth doing.
At first glance, such a style hint would in fact not be handled for descendants...
I think it would probably behave exactly like nsChangeHint_NeedReflow, including the bit about not being handled for descendants unless NeedDirtyReflow is set.  If that's the case, then it should be easy, but I need to check that.
That sounds correct to me.
This did fix the reftest failures with bug 1159042, so here's a try run with just this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76fba845d6cc
(I think the remaining assertions are from bug 1159042 rather than this bug.)
I'm fed up with having to use the NS_*Hint functions, and I also have
trouble reading/writing some of them since I don't remember the order of
parameters.

(At some point I think we should convert existing callers, but I don't
plan to do that here.)
Attachment #8614447 - Flags: review?(cam)
This was added in bug 897763, but the current name no longer makes sense
in the context of the renaming in 2d8810ba0412 (bug 779968, patch 4).

(I think the old name was also missing a negation -- it meant hints that
were never *non*-inherited.)
Attachment #8614448 - Flags: review?(cam)
Note that most of what is needed was actually taken care of by patch 1's
addition of the new hint to nsChangeHint_AllReflowHints.
Attachment #8614451 - Flags: review?(cam)
Comment on attachment 8614447 [details] [diff] [review]
patch 1 - Add bitwise operators to nsChangeHint

Review of attachment 8614447 [details] [diff] [review]:
-----------------------------------------------------------------

Can we use MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS or won't that work because it needs an enum class?
Attachment #8614448 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #16)
> Can we use MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS or won't that work because
> it needs an enum class?

I think it needs an enum class (I think I tried it when I added the ones to nsRestyleHint lower down in the file).  This also matches the nsRestyleHint ones elsewhere in the file (with the exception of the added MOZ_CONSTEXPR, which I should add to the nsRestyleHint ones).
Comment on attachment 8614451 [details] [diff] [review]
patch 5 - Adjust hints in CalcDifference methods to emit new nsChangeHint_ClearAncestorIntrinsics hint

Review of attachment 8614451 [details] [diff] [review]:
-----------------------------------------------------------------

s/ClearAncestorIntrinsics/ReflowChangesSizeOrPosition/ in the commit message.

Do we need to return this change hint from nsStyleSVG::CalcDifference?  The SVG frame's mRect can be resized and repositioned in response to the style change.  I forget how SVG frame reflow works, so perhaps the absence of nsChangeHint_ClearAncestorIntrinsics means that nsChangeHint_ReflowChangesSizeOrPosition doesn't make sense here.
Attachment #8614447 - Flags: review?(cam) → review+
Attachment #8614449 - Flags: review?(cam) → review+
Comment on attachment 8614453 [details] [diff] [review]
patch 7 - Pass parameter determined from style hint to FrameNeedsReflow

Nit on part 7:

>+++ b/layout/base/RestyleManager.cpp
>@@ -566,23 +566,31 @@ RestyleManager::StyleChangeReflow(nsIFra
>+  nsIPresShell::ReflowRootHandling rootHandling;
>+  if (aHint & nsChangeHint_ReflowChangesSizeOrPosition) {
>+    rootHandling = nsIPresShell::ePositionOrSizeChange;
>+  } else {
>+    rootHandling = nsIPresShell::eNoPositionOrSizeChange;
>+  }
>+
>   // If we're not going to clear any intrinsic sizes on the frames, and
>   // there are no dirty bits to set, then there's nothing to do.
>   if (dirtyType == nsIPresShell::eResize && !dirtyBits)
>     return;
> 
>   do {
>-    mPresContext->PresShell()->FrameNeedsReflow(aFrame, dirtyType, dirtyBits);
>+    mPresContext->PresShell()->FrameNeedsReflow(aFrame, dirtyType, dirtyBits,
>+                                                rootHandling);

The rootHandling setup code is useless work, if we take the early return here -- so it probably should move down below that return.
Attachment #8614453 - Flags: review?(dholbert) → review+
Comment on attachment 8614452 [details] [diff] [review]
patch 6 - Add parameter to nsIFrame::FrameNeedsReflow to control handling of target being a reflow root

>+++ b/layout/base/nsPresShell.cpp
[...]
>@@ -2844,23 +2845,34 @@ PresShell::FrameNeedsReflow(nsIFrame *aF
>-    // Now if subtreeRoot is a reflow root we can cut off this reflow at it if
>-    // the bit being added is NS_FRAME_HAS_DIRTY_CHILDREN.
>-    bool targetFrameDirty = (aBitToAdd == NS_FRAME_IS_DIRTY);
>+    // Determine whether we need to keep looking for the next ancestor
>+    // reflow root if subtreeRoot itself is a reflow root.
>+    bool targetNeedsReflowFromParent;
>+    switch (aRootHandling) {
>+      case ePositionOrSizeChange:
>+        targetNeedsReflowFromParent = true;
>+        break;
[...]
> #define FRAME_IS_REFLOW_ROOT(_f)                   \
>   ((_f->GetStateBits() & NS_FRAME_REFLOW_ROOT) &&  \
>-   (_f != subtreeRoot || !targetFrameDirty))
>+   (_f != subtreeRoot || !targetNeedsReflowFromParent))

Question:  So, I think this makes sense when subtreeRoot is aFrame; but does it still make sense for later iterations of the "do-while" loop that this code is inside?  In those later iterations, subtreeRoot is some other queued-up out-of-flow frame, and I'm not sure why aRootHandling would be meaningful at that point. (since aRootHandling was passed in with aFrame, but now we're looking at some separate frame that's in a different subtree)
(In reply to Daniel Holbert [:dholbert] from comment #20)
> > #define FRAME_IS_REFLOW_ROOT(_f)                   \
> >   ((_f->GetStateBits() & NS_FRAME_REFLOW_ROOT) &&  \
> >-   (_f != subtreeRoot || !targetFrameDirty))
> >+   (_f != subtreeRoot || !targetNeedsReflowFromParent))
> 
> Question:  So, I think this makes sense when subtreeRoot is aFrame; but does
> it still make sense for later iterations of the "do-while" loop that this
> code is inside?  In those later iterations, subtreeRoot is some other
> queued-up out-of-flow frame, and I'm not sure why aRootHandling would be
> meaningful at that point. (since aRootHandling was passed in with aFrame,
> but now we're looking at some separate frame that's in a different subtree)

Well, aRootHandling is really associated with the type of change being made, i.e., what CSS properties are changing.  So I think it is sensible.  (For example, if font-size changes, and a descendant of the element on which it changed is an absolutely positioned reflow root whose containing block is an ancestor of the element on which it changed, the targetNeedsReflowFromParent applies just as well to that abs pos element as it does to aFrame.)

I should probably fix the comments documenting ReflowRootHandling to explain that it also applies to out-of-flows processed due to eStyleChange, though.
(In reply to Cameron McCormack (:heycam) from comment #18)
> Do we need to return this change hint from nsStyleSVG::CalcDifference?  The
> SVG frame's mRect can be resized and repositioned in response to the style
> change.  I forget how SVG frame reflow works, so perhaps the absence of
> nsChangeHint_ClearAncestorIntrinsics means that
> nsChangeHint_ReflowChangesSizeOrPosition doesn't make sense here.

For which property changes?

All of the changes I see in nsStyleSVG::CalcDifference comment about how nsChangeHint_NeedDirtyReflow should be removed -- and in the old way of doing things, that would have implicitly removed this, since I'm effectively separating this out of NeedDirtyReflow.  And none of them felt like things that would change the position or size of the frame, although I might me misjudging (and I'm not actually sure what the rules for sizing/positioning SVG frames are).
Comment on attachment 8614452 [details] [diff] [review]
patch 6 - Add parameter to nsIFrame::FrameNeedsReflow to control handling of target being a reflow root

OK, comment 21 makes sense -- thanks. r=me on part 6.
Attachment #8614452 - Flags: review?(dholbert) → review+
(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #22)
> For which property changes?

Any of them for which we currently return a reflow hint currently.  IIRC the mRect of SVG frames covers all of the painted region of the element, and an SVG container frame's rect covers all of its children.  But I think you should check with jwatt to be sure.
Flags: needinfo?(jwatt)
Comment on attachment 8614450 [details] [diff] [review]
patch 4 - Add comment to nsStylePadding::DifferenceAlwaysHandledForDescendants

Review of attachment 8614450 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleStruct.h
@@ +655,5 @@
>      return NS_SubtractHint(NS_STYLE_HINT_REFLOW,
>                             nsChangeHint_ClearDescendantIntrinsics);
>    }
>    static nsChangeHint DifferenceAlwaysHandledForDescendants() {
>      // CalcDifference can return nsChangeHint_ClearAncestorIntrinsics as an

"an" -> "a" while you're here.

@@ +659,5 @@
>      // CalcDifference can return nsChangeHint_ClearAncestorIntrinsics as an
>      // hint not handled for descendants.
> +    // In theory, we could return the other reflow bits that are
> +    // sometimes not handled for descendants, but it won't ever cause us
> +    // to skip computation.

This is because when we do return nsChangeHint_NeedReflow, which is always handled for descendants, then we'll have included nsChangeHint_ClearAncestorIntrinsics as a not-handled for descendants bit, yes?  Maybe make this clearer by making the comment something like:

  // CalcDifference can return nsChangeHint_ClearAncestorIntrinsics as a
  // hint not handled for descendants.  But while the other reflow bits can
  // be returned as not handled for descendants, they will always be
  // returned in conjunction with nsChangeHint_ClearAncestorIntrinsics, so
  // we don't strictly need to return any change hints here for
  // nsRuleNode::CalcStyleDifference's purposes.
Attachment #8614450 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #24)
> (In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from
> comment #22)
> > For which property changes?
> 
> Any of them for which we currently return a reflow hint currently.  IIRC the
> mRect of SVG frames covers all of the painted region of the element, and an
> SVG container frame's rect covers all of its children.  But I think you
> should check with jwatt to be sure.

nsSVGPathGeometryFrame::ReflowSVG (SVG paths and other geometry) include fill, stroke and markers in mRect. FinishAndStoreOverflow then accounts for filters etc. by adding in effects to the overflow bounds.

nsSVGDisplayContainerFrame::ReflowSVG (SVG containers for the most part) does not set mRect (so general SVG containers don't have a size). It only unions the overflow rects of its children.

SVG containers that establish some sort of clipping rect (such as <svg> (nsSVGInnerSVGFrame::ReflowSVG)) set mRect to that rect in order that FinishAndStoreOverflow will restrict its overflow rects to avoid over invalidation.
Flags: needinfo?(jwatt)
Thanks.  So it sounds like we indeed don't need nsChangeHint_ReflowChangesSizeOrPosition here.
Comment on attachment 8614451 [details] [diff] [review]
patch 5 - Adjust hints in CalcDifference methods to emit new nsChangeHint_ClearAncestorIntrinsics hint

r=me with the comment 18 c/p error fixed.
Attachment #8614451 - Flags: review?(cam) → review+
We'll automatically get test coverage for this when bug 1159042 lands, although perhaps I should look into writing something to test it more directly...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: