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

RESOLVED FIXED in mozilla32

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {css3, dev-doc-complete, testcase})

Trunk
mozilla32
css3, dev-doc-complete, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(19 attachments, 25 obsolete attachments)

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
mats
: 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
mats
: 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
mats
: 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
(Assignee)

Description

7 years ago
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?
(Assignee)

Comment 2

7 years ago
Created attachment 492224 [details]
Testcase #1 (inset)
(Assignee)

Comment 3

7 years ago
Created attachment 492225 [details]
screenshot #1
(Assignee)

Comment 4

7 years ago
Created attachment 492226 [details]
screenshot #2
(Assignee)

Comment 5

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

Comment 6

7 years ago
dev-doc-info
Note that we implement an old version of box-decoration-break as -moz-background-inline-policy.
(Assignee)

Comment 7

7 years ago
Created attachment 492374 [details]
Testcase #2 (inset)
Attachment #492224 - Attachment is obsolete: true
(Assignee)

Comment 8

7 years ago
Created attachment 492376 [details]
screenshot #3 (suggested rendering)

This is the correct rendering for the testcase.  In the earlier screenshots
the blurring was affected by the skipped side.
Assignee: nobody → matspal
(Assignee)

Comment 9

7 years ago
Created attachment 492797 [details]
Testcase #3 (outset)
(Assignee)

Comment 10

7 years ago
Created attachment 492798 [details]
Screenshot #4 (suggested rendering for outset)
(Assignee)

Comment 11

7 years ago
Created attachment 494286 [details]
Testcase #4 (inset)
(Assignee)

Comment 12

7 years ago
Created attachment 494288 [details]
Testcase #5 (outset)
Attachment #492374 - Attachment is obsolete: true
Attachment #492797 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
Created attachment 494289 [details]
screenshot #5 (suggested rendering for inset)
Attachment #492376 - Attachment is obsolete: true
Attachment #492798 - Attachment is obsolete: true
(Assignee)

Comment 14

7 years ago
Created attachment 494290 [details]
screenshot #6 (suggested rendering for outset)
Attachment #492225 - Attachment is obsolete: true
Attachment #492226 - Attachment is obsolete: true
(Assignee)

Comment 15

7 years ago
Created attachment 494291 [details] [diff] [review]
Part 1 v1, style system support

This part style system support for the 'box-decoration-break' CSS property
with values 'split' (initial) and 'clone'.
(Assignee)

Comment 16

7 years ago
Created attachment 494293 [details] [diff] [review]
Part 2 v1, add box-shadow rendering for 'split'

This part makes the current box-shadow rendering be that of 'clone'
and implements the new rendering for 'split' (see screenshots).
(Assignee)

Comment 17

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

Comment 19

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

Comment 22

7 years ago
Created attachment 496030 [details] [diff] [review]
Part 3 v1, replace -moz-background-inline-policy

Switch from -moz-background-inline-policy to box-decoration-break and
remove uses of -moz-background-inline-policy:bounding-box
(Assignee)

Comment 23

7 years ago
Created attachment 496031 [details] [diff] [review]
Part 4 v1, remove -moz-background-inline-policy

Remove '-moz-background-inline-policy' property, 'bounding-box' and
'each-box' keywords ('continuous' is still in use by 'speak-numeral').
Blocks: 631373
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

Comment 24

6 years ago
What happened with this? Neither box-decoration-break, nor -moz-box-decoration-break are in 4.2pre1

Comment 25

6 years ago
Please update the patches to reflect the latest code.

Updated

4 years ago
Blocks: 913153

Comment 26

4 years ago
Please rebase your patches then ask for review so Firefox can get this implemented.
Flags: needinfo?(matspal)
(Assignee)

Comment 27

4 years ago
Created attachment 8363601 [details] [diff] [review]
part 1. Implement box-decoration-break in the style system.
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)
(Assignee)

Comment 28

4 years ago
Created attachment 8363602 [details] [diff] [review]
part 2. Implement box-decoration-break layout for border/paddding/margin/box-shadow
Attachment #8363602 - Flags: review?(cam)
(Assignee)

Comment 29

4 years ago
Created attachment 8363604 [details] [diff] [review]
part 3. Implement box-decoration-break layout for backgrounds.
Attachment #8363604 - Flags: review?(cam)
(Assignee)

Comment 30

4 years ago
Created attachment 8363605 [details] [diff] [review]
part 4. Remove remaining vestiges of -moz-background-inline-policy.
Attachment #8363605 - Flags: review?(cam)
(Assignee)

Comment 31

4 years ago
Created attachment 8363606 [details] [diff] [review]
part 5. Reftests for box-decoration-break.

https://tbpl.mozilla.org/?tree=Try&rev=827eab9c6f4c
https://tbpl.mozilla.org/?tree=Try&rev=833a6ff1912e
Attachment #8363606 - Flags: review?(cam)
Keywords: dev-doc-needed
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.
Created attachment 8364041 [details]
simple test

So for this test case...
Created attachment 8364042 [details]
simple test (heycam's expected rendering)

...I would expect it to render like this.

Can you explain from the spec how you get your slicing behaviour?
(Assignee)

Comment 35

4 years ago
I think you're right.  I must have misunderstood that part somehow, but now that I read it
it seems quite clear.
(Assignee)

Comment 36

3 years ago
Created attachment 8365688 [details] [diff] [review]
part 1. Implement box-decoration-break in the style system.
Attachment #8363601 - Attachment is obsolete: true
Attachment #8363601 - Flags: review?(cam)
Attachment #8365688 - Flags: review?(cam)
(Assignee)

Comment 37

3 years ago
Created attachment 8365690 [details] [diff] [review]
part 2. Add more detailed GetBorderRadii() method
Attachment #8363602 - Attachment is obsolete: true
Attachment #8363602 - Flags: review?(cam)
Attachment #8365690 - Flags: review?(cam)
(Assignee)

Comment 38

3 years ago
Created attachment 8365691 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.
Attachment #8363604 - Attachment is obsolete: true
Attachment #8363604 - Flags: review?(cam)
Attachment #8365691 - Flags: review?(cam)
(Assignee)

Comment 39

3 years ago
Created attachment 8365692 [details] [diff] [review]
part 4. Implement box-decoration-break layout for backgrounds.
Attachment #8365692 - Flags: review?(cam)
(Assignee)

Comment 40

3 years ago
Created attachment 8365693 [details] [diff] [review]
part 5. Remove remaining vestiges of -moz-background-inline-policy.
Attachment #8363605 - Attachment is obsolete: true
Attachment #8363605 - Flags: review?(cam)
Attachment #8365693 - Flags: review?(cam)
(Assignee)

Comment 41

3 years ago
Created attachment 8365695 [details] [diff] [review]
part 6. Reftests for box-decoration-break.

https://tbpl.mozilla.org/?tree=Try&rev=2f2a0d6734d0
Attachment #8363606 - Attachment is obsolete: true
Attachment #8363606 - Flags: review?(cam)
Attachment #8365695 - Flags: review?(cam)
(Assignee)

Comment 42

3 years ago
Created attachment 8365698 [details]
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.

Comment 43

3 years ago
dev-doc-info
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.
(Assignee)

Comment 44

3 years ago
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.
(Assignee)

Comment 45

3 years ago
Created attachment 8369701 [details] [diff] [review]
part 5b.  Make sure we Save/Restore the gfx context when using Clip.

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)
(Assignee)

Comment 49

3 years ago
(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)
(Assignee)

Comment 51

3 years ago
(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)
(Assignee)

Comment 53

3 years ago
Created attachment 8376817 [details] [diff] [review]
part 2. Add more detailed GetBorderRadii() method

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+
(Assignee)

Comment 54

3 years ago
Created attachment 8376819 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

(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)
(Assignee)

Comment 55

3 years ago
Created attachment 8376820 [details] [diff] [review]
part 7. Change mode-lines and indentation to 2-space indent.

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)
(Assignee)

Comment 56

3 years ago
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?
(Assignee)

Comment 64

3 years ago
(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)
(Assignee)

Comment 65

3 years ago
(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.
(Assignee)

Comment 68

3 years ago
Created attachment 8397142 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

(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)
(Assignee)

Comment 69

3 years ago
Created attachment 8397152 [details] [diff] [review]
part 4. Implement box-decoration-break layout for backgrounds.

(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)
(Assignee)

Comment 70

3 years ago
Created attachment 8397153 [details] [diff] [review]
part 5. Remove remaining vestiges of -moz-background-inline-policy.

(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)
(Assignee)

Updated

3 years ago
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
(Assignee)

Comment 71

3 years ago
Created attachment 8397157 [details] [diff] [review]
part 6. Reftests for box-decoration-break.

(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)
(Assignee)

Comment 72

3 years ago
Created attachment 8397160 [details] [diff] [review]
part 7. Change mode-lines and indentation to 2-space indent.

(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+
(Assignee)

Comment 73

3 years ago
Created attachment 8397166 [details] [diff] [review]
part 7b. Some additional manual changes to property_database.js.

Some additional manual changes to property_database.js.
Attachment #8397166 - Flags: review?(cam)
(Assignee)

Comment 74

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=81a16a27b40b
https://tbpl.mozilla.org/?tree=Try&rev=0ae9dc4a3912

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 77

3 years ago
Created attachment 8406519 [details]
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.
(Assignee)

Comment 78

3 years ago
Created attachment 8406520 [details] [diff] [review]
part 3. Implement box-decoration-break layout for border/paddding/margin/box-shadow.

(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+
(Assignee)

Comment 79

3 years ago
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.
(Assignee)

Comment 81

3 years ago
landing
https://hg.mozilla.org/integration/mozilla-inbound/rev/8576639616bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff492cb2c882
https://hg.mozilla.org/integration/mozilla-inbound/rev/072817c3a101
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef9b2500f319
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ccf9662d8b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/aadc6d10a36b
https://hg.mozilla.org/integration/mozilla-inbound/rev/518d271f541b
https://hg.mozilla.org/integration/mozilla-inbound/rev/28734b1d45d5
Flags: in-testsuite+
(Assignee)

Comment 82

3 years ago
Created attachment 8408230 [details] [diff] [review]
fix b2g build error
(Assignee)

Comment 83

3 years ago
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
(Assignee)

Comment 84

3 years ago
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? :-)
(Assignee)

Comment 85

3 years ago
Backed out for build failure on B2G Windows.
https://hg.mozilla.org/integration/mozilla-inbound/rev/70862f5149c6
(Assignee)

Comment 86

3 years ago
Created attachment 8408642 [details] [diff] [review]
part 8, build fix for Windows

I tried various tricks to make the Windows compiler accept an inlined
method here but without success. :(
Attachment #8408642 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 88

3 years ago
landing
https://hg.mozilla.org/integration/mozilla-inbound/rev/d872cad1a4b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/88209c35a186
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b5cdec48e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b786b54a4f85
https://hg.mozilla.org/integration/mozilla-inbound/rev/1668b5e299ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/454b9a8616a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5002946cf6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3cf8e3cccf6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ff93dab24e
(Assignee)

Updated

3 years ago
Blocks: 998535

Comment 89

3 years ago
landing
https://hg.mozilla.org/mozilla-central/rev/d872cad1a4b5
https://hg.mozilla.org/mozilla-central/rev/88209c35a186
https://hg.mozilla.org/mozilla-central/rev/b6b5cdec48e8
https://hg.mozilla.org/mozilla-central/rev/b786b54a4f85
https://hg.mozilla.org/mozilla-central/rev/1668b5e299ab
https://hg.mozilla.org/mozilla-central/rev/454b9a8616a9
https://hg.mozilla.org/mozilla-central/rev/f5002946cf6b
https://hg.mozilla.org/mozilla-central/rev/f3cf8e3cccf6
https://hg.mozilla.org/mozilla-central/rev/c2ff93dab24e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31

Updated

3 years ago
Depends on: 998792
(Assignee)

Comment 90

3 years ago
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 → ---
(Assignee)

Comment 91

3 years ago
Created attachment 8409675 [details] [diff] [review]
part 9, bug fix for regression bug 998792

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)
(Assignee)

Comment 92

3 years ago
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+
(Assignee)

Comment 94

3 years ago
Renamed the method to Restore and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b94efbd721c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d19bd97e60c
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbe3aa72cdb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1035f42d8280
https://hg.mozilla.org/integration/mozilla-inbound/rev/54ea939d2a1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/247f77ef92c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffedc1800830
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6040f41e340
https://hg.mozilla.org/integration/mozilla-inbound/rev/48070863848c
https://hg.mozilla.org/integration/mozilla-inbound/rev/06da59d29a48
Target Milestone: mozilla31 → mozilla32
https://hg.mozilla.org/mozilla-central/rev/b94efbd721c2
https://hg.mozilla.org/mozilla-central/rev/3d19bd97e60c
https://hg.mozilla.org/mozilla-central/rev/fdbe3aa72cdb
https://hg.mozilla.org/mozilla-central/rev/1035f42d8280
https://hg.mozilla.org/mozilla-central/rev/54ea939d2a1f
https://hg.mozilla.org/mozilla-central/rev/247f77ef92c3
https://hg.mozilla.org/mozilla-central/rev/ffedc1800830
https://hg.mozilla.org/mozilla-central/rev/a6040f41e340
https://hg.mozilla.org/mozilla-central/rev/48070863848c
https://hg.mozilla.org/mozilla-central/rev/06da59d29a48
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
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)
(Assignee)

Updated

3 years ago
Blocks: 1006326
(Assignee)

Comment 97

3 years ago
(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)
(Assignee)

Updated

3 years ago
Blocks: 1025669

Updated

3 years ago
No longer blocks: 913153
(Assignee)

Updated

3 years ago
Depends on: 1031444
(Assignee)

Comment 99

3 years ago
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)
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1061094

Updated

3 years ago
Depends on: 1067088
(Assignee)

Updated

2 years ago
Depends on: 1178575

Updated

2 years ago
Depends on: 1235152
You need to log in before you can comment on or make changes to this bug.