Closed Bug 1249913 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mats, Assigned: mats)

Details

(Keywords: testcase)

Attachments

(3 files)

Attached file Testcase
A frame dump shows that the overflow container's frame height is zero though,
as it should be.
Assignee: nobody → mats
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.
Attachment #8722815 - Flags: review?(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?
Flags: needinfo?(mats)
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.)
Attachment #8722815 - Flags: review?(dholbert) → review+
> Otherwise, I think we mint incorrectly catch continuations of split abspos frames here

(sorry -- s/mint/might/)
https://hg.mozilla.org/mozilla-central/rev/1607ef86164f
https://hg.mozilla.org/mozilla-central/rev/395e9c857e90
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(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.
Flags: needinfo?(mats)
You need to log in before you can comment on or make changes to this bug.