1249913 - box-decoration-break:clone on block overflow container makes it draw a border

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase

A frame dump shows that the overflow container's frame height is zero though,
as it should be.

Comment 1

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce3112bb19cb

Comment 2

[Mats Palmgren (inactive)]

Attachment: True overflow containers shouldn't have borders or background.

It's possible to avoid creating border/background display items for true
overflow containers in nsFrame::DisplayBorderBackgroundOutline, but that
would add a IS_TRUE_OVERFLOW_CONTAINER check on a hot path.  Overflow
containers are very rare so creating a redundant display item for those
seems like the better option for performance.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Reftests for 'box-decoration-break' with child overflow.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f18d450f032

Comment 4

[Daniel Holbert [:dholbert]]

In a debug build on my machine, we pass the reftest even without the fix.

Is there something missing from the reftest, to make it trigger this bug (incorrect border-drawing)  in builds without the fix?

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8722815 [details] [diff] [review]
True overflow containers shouldn't have borders or background.

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

r=me, possibly using a macro if appropriate as noted below.

::: layout/base/nsCSSRendering.cpp
@@ +755,5 @@
>    } else {
>      MOZ_ASSERT(joinedBorderArea.IsEqualEdges(aBorderArea),
>                 "Should use aBorderArea for box-decoration-break:clone");
> +    MOZ_ASSERT(aForFrame->GetSkipSides().IsEmpty() ||
> +               aForFrame->HasAnyStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER),

As noted below for nsFrame, I think this might want to be an "IS_TRUE_OVERFLOW_CONTAINER(aForFrame)" check, rather than a direct state-bit check.

::: layout/generic/nsFrame.cpp
@@ +1013,5 @@
>  nsIFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
>  {
>    if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE) &&
> +      !(GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER)) {

Should this bit-check be replaced with...
  IS_TRUE_OVERFLOW_CONTAINER(this)
?

Otherwise, I think we mint incorrectly catch continuations of split abspos frames here.  (Also: we use this macro in nsSplittableFrame::GetLogicalSkipSides(), and it seems like we should be consistent about fancier macro vs. simple state-bit-check between GetSkipSides-related code.)

Comment 6

[Daniel Holbert [:dholbert]]

> Otherwise, I think we mint incorrectly catch continuations of split abspos frames here

(sorry -- s/mint/might/)

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1607ef86164f
https://hg.mozilla.org/integration/mozilla-inbound/rev/395e9c857e90

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1607ef86164f
https://hg.mozilla.org/mozilla-central/rev/395e9c857e90

Comment 9

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] (mostly AFK March 3rd - 6th) from comment #5)
> > +    MOZ_ASSERT(aForFrame->GetSkipSides().IsEmpty() ||
> > +               aForFrame->HasAnyStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER),
> 
> As noted below for nsFrame, I think this might want to be an
> "IS_TRUE_OVERFLOW_CONTAINER(aForFrame)" check

Yes, I made it so, since that stricter check that should still hold.

> >  nsIFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
> >  {
> >    if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> > +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE) &&
> > +      !(GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER)) {
> 
> Should this bit-check be replaced with...
>   IS_TRUE_OVERFLOW_CONTAINER(this)
> ?

It could, but I just don't think it's worth the extra cycles to check
on this hot path.  The early return here is just an optimization in
itself, so falling through here will still return the correct result.