Closed Bug 613659 Opened 14 years ago Closed 10 years ago

implement box-decoration-break: Left/right part of a box-shadow should only be drawn on the first/last continuation of an inline box

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

(Depends on 1 open bug, Blocks 1 open bug, Regressed 1 open bug)

Details

(Keywords: css3, dev-doc-complete, testcase)

Attachments

(19 files, 25 obsolete files)

4.20 KB, text/html
Details
4.04 KB, text/html
Details
16.76 KB, image/png
Details
28.91 KB, image/png
Details
356 bytes, text/html
Details
1.42 KB, image/png
Details
13.64 KB, patch
heycam
: review+
Details | Diff | Splinter Review
121.29 KB, image/png
Details
9.64 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
47.20 KB, patch
heycam
: review+
jrmuizel
: review+
Details | Diff | Splinter Review
18.40 KB, patch
heycam
: review+
Details | Diff | Splinter Review
38.89 KB, patch
heycam
: review+
Details | Diff | Splinter Review
404.11 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
2.61 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.33 KB, text/html
Details
50.57 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
957 bytes, patch
Details | Diff | Splinter Review
2.28 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
6.69 KB, patch
roc
: review+
Details | Diff | Splinter Review
Left/right part of a box-shadow should only be drawn on the first/last continuation of an inline box.

Follow-up from bug 611574.

The 'box-shadow' spec [1] says "If an element has multiple boxes, all of them get drop shadows, but shadows are only drawn where borders would also be drawn; see ‘box-decoration-break’.".

The 'box-decoration-break' spec [2] defines 'slice' as the initial value and
for that value "No box-shadow is drawn at the broken edge"

[1] http://www.w3.org/TR/css3-background/#the-box-shadow
[2] http://www.w3.org/TR/css3-background/#the-box-decoration-break
I'm not quite sure what that means. We have to slice up the shadow, or what?
Attached file Testcase #1 (inset) (obsolete) —
Attached image screenshot #1 (obsolete) —
Attached image screenshot #2 (obsolete) —
(In reply to comment #1)
> I'm not quite sure what that means. We have to slice up the shadow, or what?

Yes, I think "6.1. The ‘box-decoration-break’ property" is quite clear.
Our current rendering of box shadows is what 'clone' should do.
I've attached a couple of screenshots of possible renderings for 'slice'.
Note that we implement an old version of box-decoration-break as -moz-background-inline-policy.
Attached file Testcase #2 (inset) (obsolete) —
Attachment #492224 - Attachment is obsolete: true
Attached image screenshot #3 (suggested rendering) (obsolete) —
This is the correct rendering for the testcase.  In the earlier screenshots
the blurring was affected by the skipped side.
Assignee: nobody → matspal
Attached file Testcase #3 (outset) (obsolete) —
Attached file Testcase #4 (inset)
Attached file Testcase #5 (outset)
Attachment #492374 - Attachment is obsolete: true
Attachment #492797 - Attachment is obsolete: true
Attachment #492376 - Attachment is obsolete: true
Attachment #492798 - Attachment is obsolete: true
Attachment #492225 - Attachment is obsolete: true
Attachment #492226 - Attachment is obsolete: true
Attached patch Part 1 v1, style system support (obsolete) — Splinter Review
This part style system support for the 'box-decoration-break' CSS property
with values 'split' (initial) and 'clone'.
This part makes the current box-shadow rendering be that of 'clone'
and implements the new rendering for 'split' (see screenshots).
Fwiw, all the major UAs [1] does the 'clone' rendering for box-shadow
as the default, contrary to what the spec says.
http://www.w3.org/TR/css3-background/#the-box-shadow
http://www.w3.org/TR/css3-background/#the-box-decoration-break

[1] Opera 11 beta 1, IE9 beta, nightly builds of Chrome and Safari,
    and also Firefox so far
(In reply to comment #17)
> Fwiw, all the major UAs [1] does the 'clone' rendering for box-shadow
> as the default, contrary to what the spec says.
> http://www.w3.org/TR/css3-background/#the-box-shadow
> http://www.w3.org/TR/css3-background/#the-box-decoration-break
> 
> [1] Opera 11 beta 1, IE9 beta, nightly builds of Chrome and Safari,
>     and also Firefox so far

I don't think we need to worry about compat here; box-shadow is pretty new and probably rarely used on inlines.

However, we should be removing -moz-background-inline-policy and changing the code that looks at it to look at box-decoration-break.  (See comment 6.)  That's probably best done as additional patches on top (in the reverse order:  change the code to look at the new property, and then remove the old one.)
Ok, so 'continuous' corresponds to 'split' and 'each-box' to 'clone',
but what should I do with 'bounding-box'?
https://developer.mozilla.org/en/CSS/-moz-background-inline-policy
Create a -moz-bounding-box value for box-decoration-break, I guess.
I think just remove the code for it, actually.
Switch from -moz-background-inline-policy to box-decoration-break and
remove uses of -moz-background-inline-policy:bounding-box
Remove '-moz-background-inline-policy' property, 'bounding-box' and
'each-box' keywords ('continuous' is still in use by 'speak-numeral').
Summary: Left/right part of a box-shadow should only be drawn on the first/last continuation of an inline box → implement box-decoration-break: Left/right part of a box-shadow should only be drawn on the first/last continuation of an inline box
What happened with this? Neither box-decoration-break, nor -moz-box-decoration-break are in 4.2pre1
Please update the patches to reflect the latest code.
Blocks: css3test
Please rebase your patches then ask for review so Firefox can get this implemented.
Flags: needinfo?(matspal)
Attachment #494291 - Attachment is obsolete: true
Attachment #494293 - Attachment is obsolete: true
Attachment #496030 - Attachment is obsolete: true
Attachment #496031 - Attachment is obsolete: true
Attachment #8363601 - Flags: review?(cam)
Flags: needinfo?(matspal)
Mats, I think some of your renderings aren't what I'd expect from reading the spec text.  In http://dev.w3.org/csswg/css-break/#break-decoration it says that for 'slice':

  The effect is as though the element were rendered with no breaks present, and then sliced by the
  breaks afterward.
Attached file simple test
So for this test case...
...I would expect it to render like this.

Can you explain from the spec how you get your slicing behaviour?
I think you're right.  I must have misunderstood that part somehow, but now that I read it
it seems quite clear.
Attachment #8363601 - Attachment is obsolete: true
Attachment #8363601 - Flags: review?(cam)
Attachment #8365688 - Flags: review?(cam)
Attachment #8363602 - Attachment is obsolete: true
Attachment #8363602 - Flags: review?(cam)
Attachment #8365690 - Flags: review?(cam)
Attachment #8363604 - Attachment is obsolete: true
Attachment #8363604 - Flags: review?(cam)
Attachment #8365691 - Flags: review?(cam)
Attachment #8363605 - Attachment is obsolete: true
Attachment #8363605 - Flags: review?(cam)
Attachment #8365693 - Flags: review?(cam)
https://tbpl.mozilla.org/?tree=Try&rev=2f2a0d6734d0
Attachment #8363606 - Attachment is obsolete: true
Attachment #8363606 - Flags: review?(cam)
Attachment #8365695 - Flags: review?(cam)
Attached image screenshot
So, just to clarify, I have now changed our border/background/box-shadow
rendering from the old "skip sides" method to the "visual clipping"
algorithm that is specified by the spec.  Here's a screenshot to illustrate.
Note the curved borders on the middle continuations in the bottom four
tests.  And the sliced inline "iM" in the upper right.
FWIW, how box-decoration-break interacts with some other properties was discussed in today's CSS WG meeting:

[2014-01-27 10:23:06 -0800] <TabAtkins> krit: clip-path/clip/mask same as background wrt box-decoration-break
[2014-01-27 10:23:19 -0800] <TabAtkins> krit: mask-box same behavior as border-image wrt box-decoration-break
[2014-01-27 10:23:52 -0800] <TabAtkins> fantasai: So b-d-b controls masking/clipping as well as backgrounds.
[2014-01-27 10:24:18 -0800] <TabAtkins> smfr: Are there any cases where you'd want to use different b-d-b behaviors between the properties?
[2014-01-27 10:24:21 -0800] <TabAtkins> krit: I think not.
[2014-01-27 10:24:45 -0800] <TabAtkins> RESOLVED: clipping/masking properties respond to box-decoration-break as proposed above.
Cameron, I've found a minor bug in part 4 which I'm working on...
I misread this line:
http://hg.mozilla.org/mozilla-central/annotate/01a67765dbc7/layout/base/nsCSSRendering.cpp#l2492
as actually Save()-ing the gfx context at that point, but it's not, so my
added Clip() later in the same method lingered on and cause a reftest
failure on Android.

It's a minor tweak to get it right and shouldn't affect the review much.
This goes on top of part 5.  I'll fold it before landing but I figured
it might be easier to review it as a separate patch.  There's only one
use of Reset() in the tree and that API doesn't really fit this code.
It makes more sense to have a EnsureSaved() method that you can call
to do the "delayed Save" in code paths that need it.
Attachment #8369701 - Flags: review?(cam)
Comment on attachment 8365688 [details] [diff] [review]
part 1. Implement box-decoration-break in the style system.

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

::: layout/style/test/property_database.js
@@ +4892,5 @@
> +		initial_values: [ "slice" ],
> +		other_values: [ "clone" ],
> +		invalid_values: [ "auto",  "none",  "1px" ]
> +	};
> +}

Use two spaces instead of tabs.
Attachment #8365688 - Flags: review?(cam) → review+
Comment on attachment 8365690 [details] [diff] [review]
part 2. Add more detailed GetBorderRadii() method

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +4155,2 @@
>  {
> +  if (!mOuter->nsContainerFrame::GetBorderRadii(aRadii, aSkipSides, aFrameSize, aBorderArea))

Wrap to fit 80 columns.
Attachment #8365690 - Flags: review?(cam) → review+
Comment on attachment 8365691 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

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

Removing r? as there are some questions I have below.

::: layout/base/nsCSSRendering.cpp
@@ +60,5 @@
>  
>  static int gFrameTreeLockCount = 0;
>  
> +static void
> +ApplySkipSides(int aSkipSides, nsMargin* aMargin)

To be consistent with nsIFrame::ApplySkipSides, make it take (nsMargin& aMargin, int aSkipSides)?

@@ +79,5 @@
> +                      bool* aHaveRoundedCorners)
> +{
> +  nsRect borderArea = aBorderArea;
> +  nscoord radii[8];
> +  if (aForFrame->StyleBorder()->mBoxDecorationBreak == NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

Wrap line to avoid going over 80 columns.

@@ +84,5 @@
> +    nsLayoutUtils::JoinBoxesForSlice(borderArea, aForFrame);
> +  }
> +  nsSize sz = borderArea.Size();
> +  nsSize frameSize = aForFrame->GetSize();
> +  if (&aBorder == aForFrame->StyleBorder() &&

I know this is in the old code, but can you explain why we're comparing the style struct here, and why we do different things?

@@ +85,5 @@
> +  }
> +  nsSize sz = borderArea.Size();
> +  nsSize frameSize = aForFrame->GetSize();
> +  if (&aBorder == aForFrame->StyleBorder() &&
> +      frameSize == aBorderArea.Size()) {

Use sz instead of aBorderArea.Size() since we just got it above.

@@ +92,5 @@
> +    *aHaveRoundedCorners =
> +      nsIFrame::ComputeBorderRadii(aBorder.mBorderRadius, frameSize, sz, 0, radii);
> +  }
> +  if (*aHaveRoundedCorners) {
> +    auto a2d = aForFrame->PresContext()->AppUnitsPerDevPixel();

"a2d" sounds like it's the number to multiply by to convert an app unit value to a dev pixel, but it's really the other way around.

@@ +95,5 @@
> +  if (*aHaveRoundedCorners) {
> +    auto a2d = aForFrame->PresContext()->AppUnitsPerDevPixel();
> +    nsCSSRendering::ComputePixelRadii(radii, a2d, aBgRadii);
> +  } else {
> +    return aBorderArea;

Why do we return the un-joined border area if there was no border radius?  Is this function only called when working out how to draw the radii, and nothing else (like background slicing)?

@@ +97,5 @@
> +    nsCSSRendering::ComputePixelRadii(radii, a2d, aBgRadii);
> +  } else {
> +    return aBorderArea;
> +  }
> +  return borderArea;

Move this into the |if (*aHaveRoundedCorners)| block, then move the |return aBorderArea| outside the else.

@@ +538,5 @@
> +  // Compute the outermost boundary of the area that might be painted.
> +  // Same coordinate space as aBorderArea & aBGClipRect.
> +  gfxCornerSizes bgRadii;
> +  bool haveRoundedCorners;
> +  nsRect borderArea = ::GetBorderAreaAndRadii(aForFrame, aStyleBorder, aBorderArea,

Wrap line to avoid going over 80 columns, and no need for the "::".

@@ +541,5 @@
> +  bool haveRoundedCorners;
> +  nsRect borderArea = ::GetBorderAreaAndRadii(aForFrame, aStyleBorder, aBorderArea,
> +                                              &bgRadii, &haveRoundedCorners);
> +  if (!haveRoundedCorners) {
> +    ::ApplySkipSides(aSkipSides, &border);

Why do we do this now only if we don't have border radius?

@@ +551,5 @@
>  
>    // Get our conversion values
>    nscoord twipsPerPixel = aPresContext->DevPixelsToAppUnits(1);
>  
>    // convert outer and inner rects

Presumably a comment left over from a while ago.  (Did the inner rect used to be the content box or something?)  Anyway, please update the comment.  Also can you give oRect a better name?

@@ +594,5 @@
>  
> +  //SF ("bgRadii: %f %f %f %f\n", bgRadii[0], bgRadii[1], bgRadii[2], bgRadii[3]);
> +
> +  if (haveRoundedCorners &&
> +      aForFrame->StyleBorder()->mBoxDecorationBreak == NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

wrap line

@@ +1073,5 @@
> +    hasBorderRadius = true; // we'll update this below
> +  }
> +
> +  nsRect frameRect =
> +    nativeTheme ? aForFrame->GetVisualOverflowRectRelativeToSelf() + aFrameArea.TopLeft() : aFrameArea;

Wrap this line while you're moving it (and the one below it).

@@ +1086,5 @@
>      nscoord twipsRadii[8];
>      NS_ASSERTION(aFrameArea.Size() == aForFrame->VisualBorderRectRelativeToSelf().Size(),
>                   "unexpected size");
> +    nsSize sz = frameRect.Size();
> +    hasBorderRadius = aForFrame->GetBorderRadii(twipsRadii, 0, sz, sz);

Can you explain why we don't do like the if statement in GetBorderAreaAndRadii and call ComputeBorderRadii instead of GetBorderRadii sometimes?

@@ +1291,5 @@
> +  NS_ASSERTION(aForFrame->GetType() == nsGkAtoms::fieldSetFrame ||
> +               aFrameArea.Size() == aForFrame->GetSize(), "unexpected size");
> +
> +  nsRect frameRect = aFrameArea;
> +  if (aForFrame->StyleBorder()->mBoxDecorationBreak == NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

wrap line

::: layout/base/nsCSSRenderingBorders.cpp
@@ -119,5 @@
>                                           const gfxFloat* aBorderWidths,
>                                           gfxCornerSizes& aBorderRadii,
>                                           const nscolor* aBorderColors,
>                                           nsBorderColors* const* aCompositeColors,
> -                                         int aSkipSides,

Would have preferred a separate patch to remove the currently unused aSkipSides, but don't worry about it now.

::: layout/base/nsDisplayList.cpp
@@ -2673,5 @@
>  
>    PROFILER_LABEL("nsDisplayBoxShadowOuter", "Paint");
>    for (uint32_t i = 0; i < rects.Length(); ++i) {
>      aCtx->PushState();
> -    aCtx->IntersectClip(rects[i]);

I guess this is no longer necessary since we now clip in PaintBoxShadowOuter?

::: layout/base/nsLayoutUtils.cpp
@@ +4051,5 @@
>    return contentBottom;
>  }
>  
> +/* static */ void
> +nsLayoutUtils::JoinBoxesForSlice(nsRect& aFrameArea, nsIFrame* aFrame)

Wrap to avoid going over 80 columns in a few different places in this function.

::: layout/generic/nsContainerFrame.cpp
@@ +868,5 @@
> +      clonePBM = startPBM;
> +    }
> +  }
> +
> +  nscoord endPBM = 

trailing white space

@@ +873,5 @@
> +    // clamp negative calc() to 0
> +    std::max(GetCoord(stylePadding->mPadding.Get(endSide), 0), 0) +
> +    styleBorder->GetComputedBorderWidth(endSide) +
> +    GetCoord(styleMargin->mMargin.Get(endSide), 0);
> +  if (MOZ_UNLIKELY(!sliceBreak)) {

Make this MOZ_LIKELY(sliceBreak) to match the earlier one (and to remove the double negative)?

@@ +883,5 @@
>  
>    nsContainerFrame *lastInFlow;
>    for (nsContainerFrame *nif = this; nif;
>         nif = static_cast<nsContainerFrame*>(nif->GetNextInFlow())) {
> +    if (aData->currentLine == 0) {

As I'm not super familiar with this code: where does aData->currentLine get reset to zero when we reach a next in flow that is on the next line?

Also why don't we add endPBM on to each of these continuations in the box-decoration-break:clone case?

Can you add some (or add to the existing) comments in this function explaining how we want to include the PBM on each fragment when in clone mode.

@@ +914,5 @@
>    // last line.
>    // We reached the last-in-flow, but it might have a next bidi
>    // continuation, in which case that continuation should handle
>    // the endSide border.
> +  if (MOZ_LIKELY(!lastInFlow->GetNextContinuation() && sliceBreak)) {

Should the |!lastInFlow->GetNextContinuation()| bit really be in the MOZ_LIKELY?

::: layout/generic/nsImageFrame.cpp
@@ +1778,5 @@
>  int
>  nsImageFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
>  {
> +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {

Make this |LIKELY ... == NS_STYLE_BOX_DECORATION_BREAK_SLICE| to match the previous MOZ_LIKELY calls in this patch.  Same for the remaining MOZ_UNLIKELYs.

::: layout/generic/nsInlineFrame.cpp
@@ +116,5 @@
>      !IsMarginZero(margin->mMargin.GetLeft());
>    if (haveLeft || haveRight) {
> +    if ((GetStateBits() & NS_FRAME_IS_SPECIAL) &&
> +        StyleBorder()->mBoxDecorationBreak ==
> +          NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

Can you explain this bit?  Does this just make sure in clone mode we take into account the PBM that would be at the start/end of an inline that was split by a block?  A comment would be good.

@@ +496,5 @@
>    // Don't offset by our start borderpadding if we have a prev continuation or
>    // if we're in a part of an {ib} split other than the first one.
> +  if ((!GetPrevContinuation() && !FrameIsNonFirstInIBSplit()) ||
> +      MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {

Please update the comment to state how box-decoration-break affects the "Don't offset by ...".

@@ +660,5 @@
>    // continuation or if we're in a part of an {ib} split other than the first
>    // one.
> +  if ((!GetPrevContinuation() && !FrameIsNonFirstInIBSplit()) ||
> +      MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {

Again update the comment please.

@@ +675,5 @@
>     */
> +  if ((NS_FRAME_IS_COMPLETE(aStatus) && !LastInFlow()->GetNextContinuation() &&
> +       !FrameIsNonLastInIBSplit()) ||
> +      MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {

Given you check for NS_STYLE_BOX_DECORATION_BREAK_CLONE a few times in this function, maybe store it in a local variable to avoid cluttering up these conditions.

::: layout/generic/nsLineLayout.cpp
@@ +1074,5 @@
>    // the start margin for later continuations anyway.
> +  if ((pfd->mFrame->GetPrevContinuation() ||
> +       pfd->mFrame->FrameIsNonFirstInIBSplit()) &&
> +      aReflowState.mStyleBorder->mBoxDecorationBreak ==
> +        NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

Please update the comment.

@@ +1157,5 @@
>           pfd->mFrame->LastInFlow()->GetNextContinuation() ||
>           pfd->mFrame->FrameIsNonLastInIBSplit())
> +        && !pfd->GetFlag(PFD_ISLETTERFRAME)
> +        && pfd->mFrame->StyleBorder()->mBoxDecorationBreak ==
> +             NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

Update the comment above.
Attachment #8365691 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #46)
> ::: layout/style/test/property_database.js
> Use two spaces instead of tabs.

This file actually requests TAB-indentation for some reason:
http://mxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js#1
Should I change that and re-indent the entire file?
Flags: needinfo?(cam)
Yes, but in a separate, whitespace-only (plus modeline) changeset.
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #48)
> ::: layout/base/nsCSSRendering.cpp
> > +  if (&aBorder == aForFrame->StyleBorder() &&
> 
> I know this is in the old code, but can you explain why we're comparing the
> style struct here, and why we do different things?

I don't know, it's pretty cryptic to me too.
It looks like dbaron added this in bug 459144, maybe he knows?
http://hg.mozilla.org/mozilla-central/rev/a118b42abad1
Flags: needinfo?(dbaron)
So that we don't use the nsIFrame method in cases where the caller is passing in an alternative nsStyleBorder*, which we do for internal table parts and for painting the canvas background.  (Note that bug 921341 is likely to change that code soon as well.)

(It would probably make more sense to refactor that so all the callers that don't want to override pass nullptr.)
Flags: needinfo?(dbaron)
I just moved the aSkipSides param for GetBorderRadii like so:

  virtual bool GetBorderRadii(const nsSize& aFrameSize,
                              const nsSize& aBorderArea,
                              int aSkipSides,
                              nscoord aRadii[8]) const;

so that it takes those params in the same order as
the existing ComputeBorderRadii.

No need for another review, unless you wish.
Attachment #8365690 - Attachment is obsolete: true
Attachment #8376817 - Flags: review+
(In reply to Cameron McCormack (:heycam) from comment #48)
> > +static void
> > +ApplySkipSides(int aSkipSides, nsMargin* aMargin)
> 
> To be consistent with nsIFrame::ApplySkipSides, make it take (nsMargin&
> aMargin, int aSkipSides)?

I think the convention in Layout code is to use T* for [in-]out-params and
usually they go last.  Maybe you meant nsIFrame::GetSkipSides that have
a aSkipSides param? -- I've changed that instead so that the params are
now in the same order as nsIFrame::ComputeBorderRadii, so the out-param
aRadii comes last. (in part 2)

nsIFrame::ApplySkipSides has this signature:
  void ApplySkipSides(nsMargin& aMargin,
                      const nsHTMLReflowState* aReflowState = nullptr) const;
I suppose we should change that to nsMargin* as well.
That seems like a separate bug though.

> Wrap line to avoid going over 80 columns.

I thought the consensus on the recent code style thread was that we
could use 99 if it would hurt readability to wrap it to 80.
Perhaps it was wishful thinking on my part... :-)

That said, I have wrapped the lines you requested.

> > +  nsSize sz = borderArea.Size();
> > +  nsSize frameSize = aForFrame->GetSize();
> > +  if (&aBorder == aForFrame->StyleBorder() &&
> > +      frameSize == aBorderArea.Size()) {
> 
> Use sz instead of aBorderArea.Size() since we just got it above.

Can't do that since borderArea != aBorderArea at that point.
So I take that as evidence that the JoinBoxesForSlice signature is not
clear enough that it modifies the nsRect& it takes.  Mea culpa, I should
have thought of the rule I just mentioned above.  So I've made it take
an nsRect* instead.

> "a2d" sounds like it's the number to multiply by to convert an app unit
> value to a dev pixel, but it's really the other way around.

Indeed, I made it "d2a" instead.

> @@ +95,5 @@
> > +  if (*aHaveRoundedCorners) {
> > +    auto a2d = aForFrame->PresContext()->AppUnitsPerDevPixel();
> > +    nsCSSRendering::ComputePixelRadii(radii, a2d, aBgRadii);
> > +  } else {
> > +    return aBorderArea;
> 
> Why do we return the un-joined border area if there was no border radius?

I figured I should keep the existing ApplySkipSides drawing path in that
(common) case, since it should be faster than drawing the whole area with
a clip.


> Move this into the |if (*aHaveRoundedCorners)| block, then move the |return
> aBorderArea| outside the else.

Fixed.

> > +  nsRect borderArea = ::GetBorderAreaAndRadii(
> 
> Wrap line to avoid going over 80 columns, and no need for the "::".

I use "::" here as a hint to the reader that this is not a method call
but rather a global function, and usually a "static" in the same file.
Is that not a useful hint?

> > +  if (!haveRoundedCorners) {
> > +    ::ApplySkipSides(aSkipSides, &border);
> 
> Why do we do this now only if we don't have border radius?

See earlier answer.

> Presumably a comment left over from a while ago.  (Did the inner rect used
> to be the content box or something?)  Anyway, please update the comment. 
> Also can you give oRect a better name?

I cleaned up the code a bit here, and renamed oRect -> borderAreaPx.

> ::: layout/base/nsDisplayList.cpp
> > -    aCtx->IntersectClip(rects[i]);
> 
> I guess this is no longer necessary since we now clip in PaintBoxShadowOuter?

Correct.


> ::: layout/generic/nsContainerFrame.cpp
> > +  nscoord endPBM = 
> 
> trailing white space

Fixed.

> > +  if (MOZ_UNLIKELY(!sliceBreak)) {
> 
> Make this MOZ_LIKELY(sliceBreak) to match the earlier one (and to remove the
> double negative)?

Can't do that since it would change the logic.  MOZ_UNLIKELY is just a hint
to the compiler, it doesn't affect the expression.

> > +    if (aData->currentLine == 0) {
> 
> As I'm not super familiar with this code: where does aData->currentLine get
> reset to zero when we reach a next in flow that is on the next line?

grep -r 'ForceBreak(' layout/
should give you a good overview I think.

> Also why don't we add endPBM on to each of these continuations in the
> box-decoration-break:clone case?

I do that actually, that's what 'clonePBM' is about, its value is
startPBM + endPBM for box-decoration-break:clone, and it's added to
each line.  For :slice it's zero.

> Can you add some (or add to the existing) comments in this function
> explaining how we want to include the PBM on each fragment when in clone
> mode.

I added:
  // For box-decoration-break:clone we setup clonePBM = startPBM + endPBM and
  // add that to each line.  For box-decoration-break:slice clonePBM is zero.

> > +  if (MOZ_LIKELY(!lastInFlow->GetNextContinuation() && sliceBreak)) {
> 
> Should the |!lastInFlow->GetNextContinuation()| bit really be in the
> MOZ_LIKELY?

Yes, it's very unlikely that the last-in-flow have a next-continuation.
It only happens in some rare bidi-cases AFAIK.


> ::: layout/generic/nsImageFrame.cpp
> >  nsImageFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
> >  {
> > +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> > +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {
> 
> Make this |LIKELY ... == NS_STYLE_BOX_DECORATION_BREAK_SLICE| to match the
> previous MOZ_LIKELY calls in this patch.  Same for the remaining
> MOZ_UNLIKELYs.

Again, that would change the logic.  I think the code is clearer as written,
i.e. with the CLONE case as an early return.

> ::: layout/generic/nsInlineFrame.cpp
> >    if (haveLeft || haveRight) {
> > +    if ((GetStateBits() & NS_FRAME_IS_SPECIAL) &&
> > +        StyleBorder()->mBoxDecorationBreak ==
> > +          NS_STYLE_BOX_DECORATION_BREAK_SLICE) {
> 
> Can you explain this bit?  Does this just make sure in clone mode we take
> into account the PBM that would be at the start/end of an inline that was
> split by a block?  A comment would be good.

This is the IsSelfEmpty() method.  Since "haveLeft || haveRight" is true
and all continuations have that border/padding/margin in the b-d-b:clone case
it follows that this frame isn't empty, so we should skip this if-block and
just return false.  I've added a code comment to explain it.

> Please update the comment to state how box-decoration-break affects the
> "Don't offset by ...".

Fixed.

> Again update the comment please.

Fixed.

> Given you check for NS_STYLE_BOX_DECORATION_BREAK_CLONE a few times in this
> function, maybe store it in a local variable to avoid cluttering up these
> conditions.

Fixed.

> ::: layout/generic/nsLineLayout.cpp
> Please update the comment.

Fixed.

> Update the comment above.

Fixed.
Attachment #8365691 - Attachment is obsolete: true
Attachment #8376819 - Flags: review?(cam)
Note: there is one TAB in one of the test values for 'stroke-dasharray'.
I kept that since I think it's part of the test.
Attachment #8376820 - Flags: review?(cam)
Test builds if you need them:
https://tbpl.mozilla.org/?tree=Try&rev=865a7c888639
(In reply to Mats Palmgren (:mats) from comment #54)
> (In reply to Cameron McCormack (:heycam) from comment #48)
> > > +static void
> > > +ApplySkipSides(int aSkipSides, nsMargin* aMargin)
> > 
> > To be consistent with nsIFrame::ApplySkipSides, make it take (nsMargin&
> > aMargin, int aSkipSides)?
> 
> I think the convention in Layout code is to use T* for [in-]out-params and
> usually they go last.  Maybe you meant nsIFrame::GetSkipSides that have
> a aSkipSides param? -- I've changed that instead so that the params are
> now in the same order as nsIFrame::ComputeBorderRadii, so the out-param
> aRadii comes last. (in part 2)
> 
> nsIFrame::ApplySkipSides has this signature:
>   void ApplySkipSides(nsMargin& aMargin,
>                       const nsHTMLReflowState* aReflowState = nullptr) const;
> I suppose we should change that to nsMargin* as well.
> That seems like a separate bug though.

I did mean nsIFrame::ApplySkipSides I think, as it has its nsMargin out-param first.  But you are right, better to have the out-params last.  (The optional argument in nsIFrame::ApplySkipSides makes it hard to move the out-param to the end I guess.)

> > Wrap line to avoid going over 80 columns.
> 
> I thought the consensus on the recent code style thread was that we
> could use 99 if it would hurt readability to wrap it to 80.
> Perhaps it was wishful thinking on my part... :-)
> 
> That said, I have wrapped the lines you requested.

It is my wish too, still waiting for consensus / style guide changes. :-)

> > > +  nsSize sz = borderArea.Size();
> > > +  nsSize frameSize = aForFrame->GetSize();
> > > +  if (&aBorder == aForFrame->StyleBorder() &&
> > > +      frameSize == aBorderArea.Size()) {
> > 
> > Use sz instead of aBorderArea.Size() since we just got it above.
> 
> Can't do that since borderArea != aBorderArea at that point.
> So I take that as evidence that the JoinBoxesForSlice signature is not
> clear enough that it modifies the nsRect& it takes.  Mea culpa, I should
> have thought of the rule I just mentioned above.  So I've made it take
> an nsRect* instead.

Ah I didn't notice that while reading GetBorderAreaAndRadii.  (Generally I am happy with reference arguments for things which must not be null, but you are right that making it a pointer probably would have made it more obvious that it changes the argument.)

> > @@ +95,5 @@
> > > +  if (*aHaveRoundedCorners) {
> > > +    auto a2d = aForFrame->PresContext()->AppUnitsPerDevPixel();
> > > +    nsCSSRendering::ComputePixelRadii(radii, a2d, aBgRadii);
> > > +  } else {
> > > +    return aBorderArea;
> > 
> > Why do we return the un-joined border area if there was no border radius?
> 
> I figured I should keep the existing ApplySkipSides drawing path in that
> (common) case, since it should be faster than drawing the whole area with
> a clip.

I am just confused why we might want to return different border area values.  If you change the |return aBorderArea;| to |return borderArea;|, will the painted effect be the same?  I can't see anything that applies a clip or not based on the specific area that is returned.  In PaintBorderWithBorderStyle, it  looks at haveRoundedCorners , which we know will be false.

> > > +  nsRect borderArea = ::GetBorderAreaAndRadii(
> > 
> > Wrap line to avoid going over 80 columns, and no need for the "::".
> 
> I use "::" here as a hint to the reader that this is not a method call
> but rather a global function, and usually a "static" in the same file.
> Is that not a useful hint?

I guess I have never used it as a hint, but only when required to disambiguate it from a method call.  If I wanted to know what GetBorderAreaAndRadii was I'd search and notice that it was a static function.  But feel free to keep it if you feel it is a useful hint.

> > > +  if (!haveRoundedCorners) {
> > > +    ::ApplySkipSides(aSkipSides, &border);
> > 
> > Why do we do this now only if we don't have border radius?
> 
> See earlier answer.

Sorry which earlier answer was that?  Is this just an optimization?

> > > +  if (MOZ_UNLIKELY(!sliceBreak)) {
> > 
> > Make this MOZ_LIKELY(sliceBreak) to match the earlier one (and to remove the
> > double negative)?
> 
> Can't do that since it would change the logic.  MOZ_UNLIKELY is just a hint
> to the compiler, it doesn't affect the expression.

My bad.

> > > +    if (aData->currentLine == 0) {
> > 
> > As I'm not super familiar with this code: where does aData->currentLine get
> > reset to zero when we reach a next in flow that is on the next line?
> 
> grep -r 'ForceBreak(' layout/
> should give you a good overview I think.

Got it.

> > Also why don't we add endPBM on to each of these continuations in the
> > box-decoration-break:clone case?
> 
> I do that actually, that's what 'clonePBM' is about, its value is
> startPBM + endPBM for box-decoration-break:clone, and it's added to
> each line.  For :slice it's zero.

Ah I see.

> > Can you add some (or add to the existing) comments in this function
> > explaining how we want to include the PBM on each fragment when in clone
> > mode.
> 
> I added:
>   // For box-decoration-break:clone we setup clonePBM = startPBM + endPBM and
>   // add that to each line.  For box-decoration-break:slice clonePBM is zero.

Great. :-)

> > > +  if (MOZ_LIKELY(!lastInFlow->GetNextContinuation() && sliceBreak)) {
> > 
> > Should the |!lastInFlow->GetNextContinuation()| bit really be in the
> > MOZ_LIKELY?
> 
> Yes, it's very unlikely that the last-in-flow have a next-continuation.
> It only happens in some rare bidi-cases AFAIK.

OK.

> > ::: layout/generic/nsImageFrame.cpp
> > >  nsImageFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
> > >  {
> > > +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> > > +                     NS_STYLE_BOX_DECORATION_BREAK_CLONE)) {
> > 
> > Make this |LIKELY ... == NS_STYLE_BOX_DECORATION_BREAK_SLICE| to match the
> > previous MOZ_LIKELY calls in this patch.  Same for the remaining
> > MOZ_UNLIKELYs.
> 
> Again, that would change the logic.  I think the code is clearer as written,
> i.e. with the CLONE case as an early return.

OK.

> > ::: layout/generic/nsInlineFrame.cpp
> > >    if (haveLeft || haveRight) {
> > > +    if ((GetStateBits() & NS_FRAME_IS_SPECIAL) &&
> > > +        StyleBorder()->mBoxDecorationBreak ==
> > > +          NS_STYLE_BOX_DECORATION_BREAK_SLICE) {
> > 
> > Can you explain this bit?  Does this just make sure in clone mode we take
> > into account the PBM that would be at the start/end of an inline that was
> > split by a block?  A comment would be good.
> 
> This is the IsSelfEmpty() method.  Since "haveLeft || haveRight" is true
> and all continuations have that border/padding/margin in the b-d-b:clone case
> it follows that this frame isn't empty, so we should skip this if-block and
> just return false.  I've added a code comment to explain it.

OK makes sense.
Comment on attachment 8376819 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

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

Where did the pref definition in modules/libpref/src/init/all.js go?
Comment on attachment 8365692 [details] [diff] [review]
part 4. Implement box-decoration-break layout for backgrounds.

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

::: layout/base/nsCSSRendering.cpp
@@ +2623,5 @@
>            SetupBackgroundClip(clipState, ctx, haveRoundedCorners,
>                                appUnitsPerPixel, &autoSR);
>            clipSet = true;
> +          if (haveRoundedCorners &&
> +              aForFrame->StyleBorder()->mBoxDecorationBreak == NS_STYLE_BOX_DECORATION_BREAK_SLICE) {

wrap line

@@ +2627,5 @@
> +              aForFrame->StyleBorder()->mBoxDecorationBreak == NS_STYLE_BOX_DECORATION_BREAK_SLICE) {
> +            // We're drawing the background for the joined continuation boxes
> +            // so we need to clip that to the slice that we want for this frame.
> +            ctx->NewPath();
> +            ctx->Rectangle(nsLayoutUtils::RectToGfxRect(aBorderArea, appUnitsPerPixel));

wrap line

::: layout/reftests/bugs/368020-3-ref.html
@@ +4,5 @@
>  <title>Testcase, bug 368020</title>
>  </head>
>  <body style="line-height: 3; width: 500px; height: 250px;">
>  
> +    <span style="background: url(repeatable-diagonal-gradient.png); background-clip: border-box; background-clip: border; background-origin: padding-box; background-origin: padding; margin: 7px 4px 2px 18px; border: 6px transparent solid; border-width: 2px 10px 15px 2px;">

May as well get rid of the incorrect 'background-origin: padding'.

::: layout/reftests/bugs/368020-3.html
@@ +4,5 @@
>  <title>Testcase, bug 368020</title>
>  </head>
>  <body style="line-height: 3; width: 500px; height: 250px;">
>  
> +    <span style="background: url(repeatable-diagonal-gradient.png); background-clip: padding-box; background-clip: padding; background-origin: content-box; background-origin: content; border: medium transparent solid; border-width: 7px 4px 2px 18px; padding: 2px 2% 3% 2px;">

And here.

::: layout/reftests/bugs/368020-5-ref.html
@@ +4,5 @@
>  <title>Testcase, bug 368020</title>
>  </head>
>  <body style="line-height: 3; width: 500px; height: 250px;">
>  
> +    <span style="background: url(repeatable-diagonal-gradient.png); background-clip: border-box; background-clip: border; background-origin: padding-box; background-origin: padding; margin: 7px 4px 2px 18px; border: 6px transparent solid; border-width: 2px 10px 15px 2px; box-decoration-break: clone;">

And here.

::: layout/reftests/bugs/368020-5.html
@@ +4,5 @@
>  <title>Testcase, bug 368020</title>
>  </head>
>  <body style="line-height: 3; width: 500px; height: 250px;">
>  
> +    <span style="background: url(repeatable-diagonal-gradient.png); background-clip: padding-box; background-clip: padding; background-origin: content-box; background-origin: content; border: medium transparent solid; border-width: 7px 4px 2px 18px; padding: 2px 10px 15px 2px; box-decoration-break: clone;">

And here.
Attachment #8365692 - Flags: review?(cam) → review+
Comment on attachment 8365693 [details] [diff] [review]
part 5. Remove remaining vestiges of -moz-background-inline-policy.

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

::: layout/base/nsDisplayList.cpp
@@ -2037,5 @@
> -  // this could easily lead to O(N^2) behavior inside InlineBackgroundData,
> -  // which expects frames to be sent to it in content order, not reverse
> -  // content order which we'll produce here.
> -  // Of course, if there's only one frame in the flow, it doesn't matter.
> -  if (mBackgroundStyle->mBackgroundInlinePolicy == NS_STYLE_BG_INLINE_POLICY_EACH_BOX ||

Why don't we replace this with something like |mBorderStyle->mBoxDecorationBreak == NS_STYLE_NS_STYLE_BOX_DECORATION_BREAK_CLONE|?

::: layout/style/nsCSSKeywordList.h
@@ -187,5 @@
>  CSS_KEY(border-box, border_box)
>  CSS_KEY(both, both)
>  CSS_KEY(bottom, bottom)
>  CSS_KEY(bottom-outside, bottom_outside)
> -CSS_KEY(bounding-box, bounding_box)

This keyword will return soon in bug 945187. :-)
Comment on attachment 8369701 [details] [diff] [review]
part 5b.  Make sure we Save/Restore the gfx context when using Clip.

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

r=me for the nsCSSRendering.cpp bits.

::: gfx/thebes/gfxContext.h
@@ +831,5 @@
> +  void EnsureSaved(gfxContext *aContext) {
> +    MOZ_ASSERT(!mContext || mContext == aContext, "wrong context");
> +    if (!mContext) {
> +        mContext = aContext;
> +        mContext->Save();

A gfx peer should review this bit.  (Sad face at "c-basic-offset: 4" modeline and the preponderance of 2ch indents in half the file.)

::: layout/base/nsCSSRendering.cpp
@@ +1698,5 @@
>      aCtx->Clip();
>    }
>  
>    if (aHaveRoundedCorners) {
> +    aAutoSR->EnsureSaved(aCtx); // XXXmats move this before NewPath?

Looks good to move.
Attachment #8369701 - Flags: review?(cam) → review+
Comment on attachment 8365695 [details] [diff] [review]
part 6. Reftests for box-decoration-break.

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

Do we already have tests for the default box-decoration-break mode with inset/outset box shadows?  If not we should have some.

::: layout/reftests/css-break/box-decoration-break-1-ref.html
@@ +8,5 @@
> +  <title>CSS Test: Testing box-decoration-break:clone</title>
> +  <link rel="author" title="Mats Palmgren" href="https://bugzilla.mozilla.org/show_bug.cgi?id=613659">
> +  <meta charset="utf-8">
> +<style>
> +span,button {  line-height:4em; }

Remove this declaration?

@@ +11,5 @@
> +<style>
> +span,button {  line-height:4em; }
> +
> +span {
> +  line-height:4em; 

Trailing space.

::: layout/reftests/css-break/box-decoration-break-1.html
@@ +14,5 @@
> +
> +span {
> +  box-decoration-break:clone;
> +
> +  line-height:4em; 

Trailing space.

@@ +31,5 @@
> +.o10 {
> +  border-radius: 17px;
> +  margin-left:0;
> +}
> +.o10x {

I do not understand these class names at all. :-)

::: layout/reftests/css-break/box-decoration-break-with-inset-box-shadow-1-ref.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +<style>
> +span,button {  border:3px dashed pink; margin:0 1em; line-height:4em; }

Remove ",button".

@@ +5,5 @@
> +span,button {  border:3px dashed pink; margin:0 1em; line-height:4em; }
> +
> +span {
> +  font-family:monospace;
> +  padding:1em 1em; 

Trailing space.

::: layout/reftests/css-break/box-decoration-break-with-inset-box-shadow-1.html
@@ +1,3 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>

Add some metadata/title in here like in the other tests.

@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +<style>
> +span,button {  border:3px dashed pink; margin:0 1em; line-height:4em; box-decoration-break:clone; }

Remove ",button".

@@ +5,5 @@
> +span,button {  border:3px dashed pink; margin:0 1em; line-height:4em; box-decoration-break:clone; }
> +
> +span {
> +  font-family:monospace;
> +  padding:1em 1em; 

Trailing space.

::: layout/reftests/css-break/box-decoration-break-with-outset-box-shadow-1-ref.html
@@ +5,5 @@
> +span {  border:3px dashed pink; line-height:80px;}
> +
> +span {
> +  font-family:monospace;
> +  padding:1em 1em; 

Trailing space.

::: layout/reftests/css-break/box-decoration-break-with-outset-box-shadow-1.html
@@ +5,5 @@
> +span {  border:3px dashed pink; line-height:80px; box-decoration-break:clone; }
> +
> +span {
> +  font-family:monospace;
> +  padding:1em 1em; 

Trailing space.

::: layout/reftests/css-break/reftest.list
@@ +1,1 @@
> +pref(layout.css.box-decoration-break.enabled,true) == box-decoration-break-1.html box-decoration-break-1-ref.html

You can use:

  default-preferences pref(layout.css.box-decoration-break.enabled,true)

to avoid repeating it on each line in the file if you want.

@@ +1,3 @@
> +pref(layout.css.box-decoration-break.enabled,true) == box-decoration-break-1.html box-decoration-break-1-ref.html
> +
> +skip-if(Android||B2G) fuzzy(1,20) pref(layout.css.box-decoration-break.enabled,true) == box-decoration-break-with-inset-box-shadow-1.html box-decoration-break-with-inset-box-shadow-1-ref.html

Do you need these skip-ifs if you have them in the parent reftest.list?  Or vice versa maybe, if box-decoration-break-1.html works fine on these platforms.

::: layout/reftests/reftest.list
@@ +54,5 @@
>  # blending/
>  skip-if(B2G||Android) include css-blending/reftest.list
>  
> +# Tests for the css-break spec
> +skip-if(Android||B2G) include css-break/reftest.list

OOC, what doesn't work on these platforms?
Attachment #8365695 - Flags: review?(cam) → review+
Comment on attachment 8376820 [details] [diff] [review]
part 7. Change mode-lines and indentation to 2-space indent.

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

If you're changing the modeline to a 2 space indent, do you want to reindent the body of the file that mostly uses 4?
(In reply to Cameron McCormack (:heycam) from comment #57)
> If you change the |return aBorderArea;| to |return borderArea;|, will the
> painted effect be the same?  

No, it draws borders to far when the frame has continuations.

> I can't see anything that applies a clip or not
> based on the specific area that is returned.  In PaintBorderWithBorderStyle,
> it  looks at haveRoundedCorners , which we know will be false.

Right, we will either do this:
  if (!haveRoundedCorners) {
    ::ApplySkipSides(aSkipSides, &border);
  }

Or, a bit later:
  if (haveRoundedCorners && aForFrame->StyleBorder()->mBoxDecorationBreak ==
                              NS_STYLE_BOX_DECORATION_BREAK_SLICE) {
    // We're drawing borders around the joined continuation boxes so we need
    // to clip that to the slice that we want for this frame.
    aRenderingContext.IntersectClip(aBorderArea);
  }

So there are two paths.  Either we draw borders using the larger
"JoinBoxesForSlice" area, and apply a clip to get the part for the
frame at hand.  Or, we use the area of the current frame and do
ApplySkipSides to exclude the sides that don't apply for this frame.

But you're right that the code is a bit hard to follow...  I'll fix
that somehow.  Would be clearer if GetBorderAreaAndRadii always
returned 'borderArea' and then separate the paths in
PaintBorderWithBorderStyle? something like this:

  if (borderArea.IsEqualEdges(aBorderArea)) {
    ::ApplySkipSides(aSkipSides, &border);
  } else {
    aRenderingContext.IntersectClip(aBorderArea);
  }

and also rename 'borderArea' to 'joinedBorderArea' and add some
code comments?

(I may have to give up sharing GetBorderAreaAndRadii for backgrounds
though, I haven't checked yet.)
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #58)
> Where did the pref definition in modules/libpref/src/init/all.js go?

Not sure what you mean - it's in part 1, as always.
(In reply to Mats Palmgren (:mats) from comment #65)
> Not sure what you mean - it's in part 1, as always.

Sorry, I was looking at the interdiff between the old/new part 3 patch and saw it disappear from there and forgot that I already saw it in part 1.
Flags: needinfo?(cam)
(In reply to Mats Palmgren (:mats) from comment #64)
> So there are two paths.  Either we draw borders using the larger
> "JoinBoxesForSlice" area, and apply a clip to get the part for the
> frame at hand.  Or, we use the area of the current frame and do
> ApplySkipSides to exclude the sides that don't apply for this frame.

Why do we call ApplySkipSides in the !haveRoundedCorners case?

> But you're right that the code is a bit hard to follow...  I'll fix
> that somehow.  Would be clearer if GetBorderAreaAndRadii always
> returned 'borderArea' and then separate the paths in
> PaintBorderWithBorderStyle? something like this:
> 
>   if (borderArea.IsEqualEdges(aBorderArea)) {
>     ::ApplySkipSides(aSkipSides, &border);
>   } else {
>     aRenderingContext.IntersectClip(aBorderArea);
>   }
> 
> and also rename 'borderArea' to 'joinedBorderArea' and add some
> code comments?

I think that would be clearer, although it would be good if we didn't actually have to do a comparison of the border edge positions and just got a boolean out of GetBorderAreaAndRadii that we could use instead.
(In reply to Cameron McCormack (:heycam) from comment #67)
> Why do we call ApplySkipSides in the !haveRoundedCorners case?

Using ApplySkipSides and the frame's are (not the joined area) was
just an optimization since continuations shouldn't affect other
continuations' borders unless they are rounded.  But I now think
that's not entirely correct since it affects how dashed/dotted
borders start, and maybe other things.
So I removed that bit, and added comments in PaintBorderWithStyleBorder
for the different b-d-b:slice cases for drawing borders.  Drawing
non-rounded borders using the joined area showed a bug in the way
I calculated the area for inlines (BIDI, ib-split).  Fortunately there
is code in InlineBackgroundData::GetContinuousRect that I could reuse.
Note that the joined area for backgrounds is NOT the same as for
borders, but it's easy to "rotate" the area to get the left-border
continuation as the origin instead.
Attachment #8376819 - Attachment is obsolete: true
Attachment #8376819 - Flags: review?(cam)
Attachment #8397142 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #59)
> part 4. Implement box-decoration-break layout for backgrounds.

Fixed all the nits.  And some minor tweaks.

Please ignore the gfx/thebes/gfxContext.h bit -
I'll ask a GFX peer to review that bit.
Attachment #8365692 - Attachment is obsolete: true
Attachment #8369701 - Attachment is obsolete: true
Attachment #8397152 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #60)
> part 5. Remove remaining vestiges of -moz-background-inline-policy.
> ::: layout/base/nsDisplayList.cpp
> @@ -2037,5 @@
> > -  // this could easily lead to O(N^2) behavior inside InlineBackgroundData,
> > -  // which expects frames to be sent to it in content order, not reverse
> > -  // content order which we'll produce here.
> > -  // Of course, if there's only one frame in the flow, it doesn't matter.
> > -  if (mBackgroundStyle->mBackgroundInlinePolicy == NS_STYLE_BG_INLINE_POLICY_EACH_BOX ||
> 
> Why don't we replace this with something like
> |mBorderStyle->mBoxDecorationBreak ==
> NS_STYLE_NS_STYLE_BOX_DECORATION_BREAK_CLONE|?

Yeah, good point, did so.

> ::: layout/style/nsCSSKeywordList.h
> > -CSS_KEY(bounding-box, bounding_box)
> 
> This keyword will return soon in bug 945187. :-)

OK, I'll still remove it here though so that when it's re-added the line
will point to the right changeset/bug.
Attachment #8365693 - Attachment is obsolete: true
Attachment #8365693 - Flags: review?(cam)
Attachment #8397153 - Flags: review?(cam)
Attachment #8397153 - Attachment description: Remove remaining vestiges of -moz-background-inline-policy. → part 5. Remove remaining vestiges of -moz-background-inline-policy.
Attachment #8397153 - Attachment is patch: true
(In reply to Cameron McCormack (:heycam) from comment #62)
> part 6. Reftests for box-decoration-break.
> 
> Do we already have tests for the default box-decoration-break mode with
> inset/outset box shadows?  If not we should have some.

I'm pretty sure we don't, since the current box shadow rendering is more
clone-like than slice, and I didn't have to update any box shadow tests.
I'll try to write some.

I fixed the other nits on the test files.

> ::: layout/reftests/css-break/reftest.list
> Do you need these skip-ifs if you have them in the parent reftest.list?  Or
> vice versa maybe, if box-decoration-break-1.html works fine on these
> platforms.

Yeah, I'm confused about that as well.  I would have thought that
"skip-if(B2G||Android)" on the 'include' would skip these tests on those
platforms but apparently not since I had to add it here too.
Anyway...

> OOC, what doesn't work on these platforms?

IIRC, there were some fuzzy errors in the shadow pixels that I concluded
must be some GFX platform weirdness.  But now that I tried it again the
tests passed!  So, let's try having them enabled for the initial landing,
we can disable them later in case they have intermittent failures.
Attachment #8365695 - Attachment is obsolete: true
Attachment #8397157 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #63)
> part 7. Change mode-lines and indentation to 2-space indent.
> If you're changing the modeline to a 2 space indent, do you want to reindent
> the body of the file that mostly uses 4?

Fixed.  (This part was scripted.)
Attachment #8376820 - Attachment is obsolete: true
Attachment #8376820 - Flags: review?(cam)
Attachment #8397160 - Flags: review+
Some additional manual changes to property_database.js.
Attachment #8397166 - Flags: review?(cam)
Status: NEW → ASSIGNED
Blocks: 988653
Comment on attachment 8397142 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

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

::: layout/base/nsCSSRendering.cpp
@@ +98,4 @@
>    }
>  
> +  /**
> +   * Return a continuous rect for (an inline) aFrame relative the continuation

"relative to"

@@ +158,5 @@
>      return nsRect(-x, 0, mUnbrokenWidth, mFrame->GetSize().height);
>    }
>  
> +  /**
> +   * Return a continuous rect for (an inline) aFrame relative the continuation

"relative to"

@@ +170,5 @@
> +  {
> +    // Calling GetContinuousRect(aFrame) here may lead to Reset/Init which
> +    // resets our mLeftBorderData so we save it ...
> +    BorderLeftData saved(mLeftBorderData);
> +    nsRect borderArea = GetContinuousRect(aFrame);

I find it a bit confusing that we have aBorderArea and borderArea, but one of them is the big continuous rectangle, and the other is for the single frame aFrame.  Can you change the name of one of them?

@@ +179,5 @@
> +        mLeftBorderData.SetX(GetContinuousRect(mLeftBorderData.mFrame).x);
> +      }
> +    } else {
> +      // ... and restore it when possible.
> +      mLeftBorderData.SetX(saved.mX);

We know saved.mIsValid is true here, so you could just assign to mX instead of calling SetX.

@@ +182,5 @@
> +      // ... and restore it when possible.
> +      mLeftBorderData.SetX(saved.mX);
> +    }
> +    if (borderArea.x > mLeftBorderData.mX) {
> +      borderArea.x = -(mUnbrokenWidth + borderArea.x - aBorderArea.width);

I'm a bit slow here; can you explain this, maybe with a small example?  (I'm not sure "rotate" in the function's comment is the right word to describe what's happening here?)

@@ +209,5 @@
>  protected:
> +  struct BorderLeftData {
> +    nsIFrame* mFrame;
> +    nscoord   mX;
> +    bool      mIsValid;

Can you please document these -- at least mFrame and mX?

@@ +220,5 @@
> +  nsRect         mBoundingBox;
> +  nscoord        mContinuationPoint;
> +  nscoord        mUnbrokenWidth;
> +  nscoord        mLineContinuationPoint;
> +  BorderLeftData mLeftBorderData;

Change the type to LeftBorderData or the member to mBorderLeftData.

@@ +487,5 @@
> +  return borderArea;
> +}
> +
> +/**
> + * Extend aBorderArea relative aFrame's origin to calculate a hypothetical

s/relative aFrame's origin/, which is relative to aFrame's origin,/

@@ +631,5 @@
>    }
>  
> +  // Compute the outermost boundary of the area that might be painted.
> +  // Same coordinate space as aBorderArea & aBGClipRect.
> +  nsRect borderArea =

Did you want to rename this to joinedBorderArea per comment 67?

@@ +643,5 @@
> +  // start drawing
> +  gfxContext* ctx = aRenderingContext.ThebesContext();
> +  ctx->Save();
> +
> +  // we can assume that we're already clipped to aDirtyRect -- I think? (!?)

As we've had this comment/code for five years I think this must be true all the time. :-)  Feel free to remove it, especially since you mention clipping in the if statements below.

@@ +692,5 @@
>    }
>  
>    SF(" borderStyles: %d %d %d %d\n", borderStyles[0], borderStyles[1], borderStyles[2], borderStyles[3]);
> +  //SF ("bgRadii: %f %f %f %f\n", bgRadii[0], bgRadii[1], bgRadii[2], bgRadii[3]);
> +#if 0

Put a blank line before this one.

@@ +1158,5 @@
> +    aForFrame->GetVisualOverflowRectRelativeToSelf() + aFrameArea.TopLeft() :
> +    aFrameArea;
> +  frameRect = ::BoxDecorationRectForBorder(aForFrame, frameRect);
> +
> +  // Get any border radius, since box-shadow must also have rounded corners if the frame does.

Mind the 80 column limit.

::: layout/generic/nsFrame.cpp
@@ +946,5 @@
>  
>  int
>  nsIFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
>  {
> +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==

Unsure whether it's worth using a MOZ_UNLIKELY for a perfectly valid use of box-decoration-break.

::: layout/generic/nsInlineFrame.cpp
@@ +677,3 @@
>     */
> +  if ((NS_FRAME_IS_COMPLETE(aStatus) && !LastInFlow()->GetNextContinuation() &&
> +       !FrameIsNonLastInIBSplit()) || boxDecorationBreakClone) {

I think it might be clearer with the original line breaks after each ANDed term.

::: layout/generic/nsSplittableFrame.cpp
@@ +252,5 @@
>    if (IS_TRUE_OVERFLOW_CONTAINER(this)) {
>      return LOGICAL_SIDES_B_BOTH;
>    }
>  
> +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==

Maybe put this at the top of the function, for symmetry with the other GetLogicalSkipSides changes in this patch?  I'm assuming the order doesn't matter here.
Attachment #8397142 - Flags: review?(cam) → review+
Attachment #8397152 - Flags: review?(cam) → review+
Attachment #8397153 - Flags: review?(cam) → review+
Attachment #8397157 - Flags: review?(cam) → review+
Comment on attachment 8397166 [details] [diff] [review]
part 7b. Some additional manual changes to property_database.js.

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

::: layout/style/test/property_database.js
@@ +3677,5 @@
>      inherited: true,
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "none", "context-value" ],
> +    // This TAB is intentional to test parsing --------------v
> +    other_values: [ "5px,3px,2px", "5px 3px 2px", "  5px ,3px	, 2px ", "1px", "5%", "3em" ],

Just use \t in the string rather than point out a maybe-invisible tab character.
Attachment #8397166 - Flags: review?(cam) → review+
Attached file Testcase #8
(In reply to Cameron McCormack (:heycam) from comment #75)
> I'm a bit slow here; can you explain this, maybe with a small example?  (I'm
> not sure "rotate" in the function's comment is the right word to describe
> what's happening here?)

In the attached example, the GetContinuousRect rect is in the order
1,2,3 (background order).  For border rendering we want x-coordintes
as if the order was 1,2,3 (top), 3,2,1 (bottom).
The code that does that:

    if (joinedBorderArea.x > mLeftBorderData.mX) {
      joinedBorderArea.x =
        -(mUnbrokenWidth + joinedBorderArea.x - aBorderArea.width);
    } else {
      joinedBorderArea.x -= mLeftBorderData.mX;
    }

So, given the x-offset of "1" relative aFrame (joinedBorderArea.x) and
the x-offset of "1" relative the left-border-frame (mLeftBorderData.mX)
in the GetContinuousRect rect, this calculates the x-offset relative
the left-border-frame instead of "1" and reorders the frames that are
left of the left-border (1 and 2 in the RTL case at the bottom).

For a typical LTR case (top) the order and offsets do not change.
mLeftBorderData.mX is zero and we fall into the else-part since
the 1 has negative relative offset to 2 and 3.

For RTL (bottom), the left-border-frame has the most negative
x-offset (to 1) so the if-condition is true for 1 and 2.
For 3 we hit the else-part and x becomes zero.

Yeah, "rotate" isn't the right word.  I'm not sure what is.
(In reply to Cameron McCormack (:heycam) from comment #75)
> Comment on attachment 8397142 [details] [diff] [review]
> part 3. Implement box-decoration-break layout for
> border/paddding/margin/box-shadow.
> > + * Extend aBorderArea relative aFrame's origin to calculate a hypothetical
> 
> s/relative aFrame's origin/, which is relative to aFrame's origin,/

Done.  I also changed "Extend" to "Inflate" to make it clearer that
the result has the same origin, it just extends outward.

> > +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> 
> Unsure whether it's worth using a MOZ_UNLIKELY for a perfectly valid use of
> box-decoration-break.

I suspect that b-d-b:clone will be very rarely used, so a compiler hint
seems worth it.

> ::: layout/generic/nsSplittableFrame.cpp
> >    if (IS_TRUE_OVERFLOW_CONTAINER(this)) {
> >      return LOGICAL_SIDES_B_BOTH;
> >    }
> >  
> > +  if (MOZ_UNLIKELY(StyleBorder()->mBoxDecorationBreak ==
> 
> Maybe put this at the top of the function, for symmetry with the other
> GetLogicalSkipSides changes in this patch?  I'm assuming the order doesn't
> matter here.

The order does matter.  We shouldn't render anything for a true overflow
container itself, just its children, so b-d-b:clone doesn't apply.
(I think s/LOGICAL_SIDES_B_BOTH/LOGICAL_SIDES_ALL/ and a comment would
make that clearer. I'll file that as a separate bug.)
(layout/reftests/pagination/border-breaking-002-cols.xhtml has an
example if you want test this with "return 0" there)

I fixed the other nits as requested.
Attachment #8397142 - Attachment is obsolete: true
Attachment #8406520 - Flags: review+
Comment on attachment 8397152 [details] [diff] [review]
part 4. Implement box-decoration-break layout for backgrounds.

Jeff, can you review this small change to gfxContextAutoSaveRestore
in gfx/thebes/gfxContext.h please? (ignore the rest of the patch)

The change makes it easier to do a Save() only when needed, typically
when we get into a branch that needs to add a clip. The Reset() method
became unused in the process so I removed it.
Attachment #8397152 - Flags: review?(jmuizelaar)
Attachment #8397152 - Flags: review?(jmuizelaar) → review+
(In reply to Mats Palmgren (:mats) from comment #77)
> In the attached example, the GetContinuousRect rect is in the order
> 1,2,3 (background order).  For border rendering we want x-coordintes
> as if the order was 1,2,3 (top), 3,2,1 (bottom).

Thanks, I understand now.  Is it possible to describe this in a comment somehow, that we're doing this to just reverse the positions of each frame its container is RTL?

> Yeah, "rotate" isn't the right word.  I'm not sure what is.

Is it really "reverse"?  I'm OK with leaving it as "rotate" if there's no better word, especially if you add the comment, as it should be clearer then.
Looks like unified builds hid this problem on other platforms,
I wonder why it didn't show up on Try though?
https://hg.mozilla.org/integration/mozilla-inbound/rev/21043f348d65
Oh, "B2G Desktop Windows Opt" is the one B2G platform I didn't test.
I'm guessing this is a relatively new platform so it's missing in my Try template patch.
What are the odds? :-)
Backed out for build failure on B2G Windows.
https://hg.mozilla.org/integration/mozilla-inbound/rev/70862f5149c6
I tried various tricks to make the Windows compiler accept an inlined
method here but without success. :(
Attachment #8408642 - Flags: review?(roc)
Attachment #8408642 - Flags: review?(cam)
Comment on attachment 8408642 [details] [diff] [review]
part 8, build fix for Windows

[stealing review on part 8 per IRC]

r=me
Attachment #8408642 - Flags: review?(roc)
Attachment #8408642 - Flags: review?(cam)
Attachment #8408642 - Flags: review+
Blocks: 998535
Depends on: 998792
Backed out on m-c:
remote:   https://hg.mozilla.org/mozilla-central/rev/4bc69199b7e1
remote:   https://hg.mozilla.org/mozilla-central/rev/28dc0100f9b0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry to bother you again, I need a method to explicitly Restore the
gfxContext if any were Saved.  (I somehow got the crazy idea that
~gfxContextAutoSaveRestore would run when assigning 'autoSR')

(this is to fix bug 998792)

https://tbpl.mozilla.org/?tree=Try&rev=adc91681d837
https://tbpl.mozilla.org/?tree=Try&rev=5066da0cbb86
Attachment #8409675 - Flags: review?(jmuizelaar)
Comment on attachment 8409675 [details] [diff] [review]
part 9, bug fix for regression bug 998792

roc, this is a trivial 7-line patch to fix a bug I introduced in the
earlier set of patches.  Can you review it please?

It adds a new method to gfxContextAutoSaveRestore to call Restore()
on any saved mContext and then null it out.
Attachment #8409675 - Flags: review?(jmuizelaar) → review?(roc)
Comment on attachment 8409675 [details] [diff] [review]
part 9, bug fix for regression bug 998792

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

r+ either way

::: gfx/thebes/gfxContext.h
@@ +819,5 @@
>      mContext->Save();
>    }
>  
>    ~gfxContextAutoSaveRestore() {
> +    Clear();

Why don't we call this Restore()?
Attachment #8409675 - Flags: review?(roc) → review+
Is there a bug on turning on the pref?

(It seems less than ideal to remove -moz-background-inline-policy without enabling box-decoration-break, although I suppose it's unlikely to be used much.)
Flags: needinfo?(matspal)
Blocks: 1006326
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #96)
> Is there a bug on turning on the pref?

I filed bug 1006326 on enabling it by default in Nightly.

> (It seems less than ideal to remove -moz-background-inline-policy without
> enabling box-decoration-break, although I suppose it's unlikely to be used
> much.)

Yeah, the code is rather entangled so it seems hard maintaining two code paths.
Flags: needinfo?(matspal)
So, if I understand correctly, as box-decoration-break is behind a pref that is off by default even in Nightly, the only user facing feature here is to remove -moz-background-inline-policy.

Mats, am I correct?
Flags: needinfo?(mats)
Blocks: 1025669
No longer blocks: css3test
Depends on: 1031444
This is now preffed on by default for Firefox 32 (bug 1006326) and I've updated the MDN pages.
https://developer.mozilla.org/en-US/docs/Web/CSS/box-decoration-break
https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-background-inline-policy
Flags: needinfo?(mats)
Depends on: 1061094
Depends on: 1067088
Depends on: 1178575
Depends on: 1235152
Regressions: 1543105
You need to log in before you can comment on or make changes to this bug.