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

RESOLVED FIXED in Firefox 47

Status

()

Core
Layout: Block and Inline
--
minor
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({testcase})

unspecified
mozilla47
testcase
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
Created attachment 8721694 [details]
Testcase

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

Updated

2 years ago
Assignee: nobody → mats
Comment hidden (obsolete)
(Assignee)

Comment 2

2 years ago
Created attachment 8722815 [details] [diff] [review]
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.
Attachment #8722815 - Flags: review?(dholbert)
(Assignee)

Comment 3

2 years ago
Created attachment 8722816 [details] [diff] [review]
Reftests for 'box-decoration-break' with child overflow.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f18d450f032
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/)

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1607ef86164f
https://hg.mozilla.org/mozilla-central/rev/395e9c857e90
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 9

2 years ago
(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.