1319407 - clip-path reference-box geometry computing should respect box-decoration-break

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[u459114]

According to CSS Fragmentation Module Level 3:
https://drafts.csswg.org/css-break/#break-decoration

When a break (page/column/region/line) splits a box, the box-decoration-break property controls
* how the background positioning area [CSS3BG] (and mask positioning area [CSS-MASKING-1], shape reference box [CSS-SHAPES-1], etc.) is derived from or duplicated across the box fragments and how the element’s background is drawn within them. 

We do not actually handle box-decoration-break: slice among nsDisplayMask/ nsCSSClipPathInstance::ComputeSVGReferenceRect()/ nsCSSClipPathInstance::ComputeHTMLReferenceRect()

Comment 1

[u459114]

Attachment: box-decoration-break.html

Comment 2

[u459114]

clip-path + box-decoration-break work not correctly on FF, Chrome and Edge. So I think we need to fix it, but this bug should not be a blocker of basic-shape shipping.

Comment 3

[Louis Chang [:louis]]

Attachment: WIP patch

A hacking patch that makes svg clip-path works correctly when using box-decoration-break: clone..

Comment 4

[Louis Chang [:louis]]

Attachment: Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

Review commit: https://reviewboard.mozilla.org/r/158272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158272/

Comment 5

[Louis Chang [:louis]]

Attachment: Bug 1319407 - Add reftest for clip-path when box-decoration-break is clone.

Review commit: https://reviewboard.mozilla.org/r/158274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158274/

Comment 6

[Louis Chang [:louis]]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

Hi CJ,

Could you review the patch for me? Thanks.

In the patch, I make it not to merge the continuation frames when
box-decoration-break is clone. So it will call nsDisplayMask::Paint for each frame. Then we can apply clip-path to each frame respectively.

I think it works for both path: PaintMask and PaintAsLayer. The reftest of using PaintAsLayer is in the other attachment. However, I only test PaintMask on local with hack patch.

Comment 7

[Louis Chang [:louis]]

Comment on attachment 8887411 [details]
Bug 1319407 - Add reftest for clip-path when box-decoration-break is clone.

reftest for clip-path with box-decoration-break is clone.

Comment 8

[u459114]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

https://reviewboard.mozilla.org/r/158272/#review163922

::: layout/painting/nsDisplayList.cpp:8464
(Diff revision 1)
>  
> +  // items with StyleBoxDecorationBreak::Clone should not be merged into a
> +  // compositing group
> +  if (mFrame->StyleBorder()->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone)
> +    return false;
> +

Put return false in curly brackets

::: layout/svg/nsSVGIntegrationUtils.cpp:222
(Diff revision 1)
> +                                                nullptr,
> +                                                nsRect(),
> +                                                false);
> +    collector.AddBox(aNonSVGFrame);
> +    r = collector.GetResult() + GetOffsetToBoundingBox(firstFrame);
> +  } else {

Add
MOZ_ASSERT(aNonSVGFrame->StyleBorder()->mBoxDecorationBreak == StyleBoxDecorationBreak::Slice);

::: layout/svg/nsSVGIntegrationUtils.cpp:225
(Diff revision 1)
> +    collector.AddBox(aNonSVGFrame);
> +    r = collector.GetResult() + GetOffsetToBoundingBox(firstFrame);
> +  } else {
> +    r = GetPreEffectsVisualOverflowUnion(firstFrame, nullptr, nsRect(),
> -                                              GetOffsetToBoundingBox(firstFrame),
> +                                         GetOffsetToBoundingBox(firstFrame),
> -                                              false);
> +                                         false);

Can you add a new parameter "lastFrame" as the 2nd parameter of GetPreEffectsVisualOverflowUnion.

Set lastFrame as firstFrame if mBoxDecorationBreak == StyleBoxDecorationBreak::Clone; otherwise, set lastFrame as nullptr.

Inside GetPreEffectsVisualOverflowUnion, iterate frames from firstFrame to lastFrame, and iterate to the last coninuation one if lastFrame is nullptr.

Then, the code here will look like:
nsIFrame* lastFrame = aNonSVGFrame->StyleBorder()->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone ? firstFrame : nullptr
r = GetPreEffectsVisualOverflowUnion(firstFrame, lastFrame, nullptr, .....);

Comment 9

[u459114]

Comment on attachment 8887411 [details]
Bug 1319407 - Add reftest for clip-path when box-decoration-break is clone.

https://reviewboard.mozilla.org/r/158274/#review163938

::: layout/reftests/svg/box-decoration-break.html:9
(Diff revision 1)
> +-->
> +
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>Test of box-decoration-break</title>

Test of box-decoration-break with clip-path

::: layout/reftests/svg/box-decoration-break.html:15
(Diff revision 1)
> +  <style>
> +    .clone {
> +      -ms-box-decoration-break: clone;
> +      -webkit-box-decoration-break: clone;
> +      -o-box-decoration-break: clone;
> +      box-decoration-break: clone;

keep only canonical one -
box-decoration-break: clone;

::: layout/reftests/svg/box-decoration-break.html:19
(Diff revision 1)
> +      -o-box-decoration-break: clone;
> +      box-decoration-break: clone;
> +    }
> +    span {
> +      background: linear-gradient(to bottom right, yellow, green);
> +      box-shadow: 8px 8px 10px 0px deeppink, -5px -5px 5px 0px blue, 5px 5px 15px 0px yellow;

box-shadow is not relative to this test, please remove it. and the same for border-xxx

::: layout/reftests/svg/box-decoration-break.html:34
(Diff revision 1)
> +      background: grey;
> +    }
> +  </style>
> +</head>
> +<body>
> +    <div>

2 space indent

Comment 10

[Louis Chang [:louis]]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158272/diff/1-2/

Comment 11

[u459114]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

https://reviewboard.mozilla.org/r/158272/#review164042

::: layout/svg/nsSVGIntegrationUtils.cpp:139
(Diff revision 2)
>    // Compute union of all overflow areas relative to aFirstContinuation:
> -  nsLayoutUtils::GetAllInFlowBoxes(aFirstContinuation, &collector);
> +  nsLayoutUtils::GetInFlowBoxes(aFirstContinuation, aCurrentFrame, &collector);
>    // Return the result in user space:
>    return collector.GetResult() + aFirstContinuationToUserSpace;
>  }
>  

Leave GetPreEffectsVisualOverflowUnion as it is.
Add a new function

GetPreEffectsVisualOverflow(
  nsIFrame* aCurrentFrame,
  const nsRect& aCurrentFramePreEffectsOverflow,
  const nsPoint& aFirstContinuationToUserSpace,
  bool aInReflow)
  
Only take geometry of aCurrentFrame into account.

Comment 12

[Louis Chang [:louis]]

Attachment: Bug 1319407 - Add reftest for box-decoration-break with clip-path.

Review commit: https://reviewboard.mozilla.org/r/158806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158806/

Comment 13

[Louis Chang [:louis]]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158272/diff/2-3/

Comment 14

[Louis Chang [:louis]]

Comment on attachment 8887876 [details]
Bug 1319407 - Add reftest for box-decoration-break with clip-path.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158806/diff/1-2/

Comment 15

[u459114]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

https://reviewboard.mozilla.org/r/158272/#review164460

::: layout/base/nsLayoutUtils.cpp:3935
(Diff revision 3)
>  
>  void
> +nsLayoutUtils::GetInFlowBoxes(nsIFrame* aCurrentFrame,
> +                              BoxCallback* aCallback)
> +{
> +  AddBoxesForFrame(aCurrentFrame, aCallback);

How about export AddBoxesForFrame from nsLayoutUtils and remove this one?

::: layout/painting/nsDisplayList.cpp:8460
(Diff revision 3)
>  bool nsDisplayMask::TryMerge(nsDisplayItem* aItem)
>  {
>    if (aItem->GetType() != TYPE_MASK)
>      return false;
>  
> +  // items with StyleBoxDecorationBreak::Clone should not be merged into a

// Do not merge items with StyleBoxDecorationBreak::Clone prop value.

::: layout/svg/nsSVGIntegrationUtils.cpp:145
(Diff revision 3)
> +static nsRect
> +GetPreEffectsVisualOverflow(nsIFrame* aFirstContinuation,
> +                            nsIFrame* aCurrentFrame,
> +                            const nsRect& aCurrentFramePreEffectsOverflow,
> +                            const nsPoint& aFirstContinuationToUserSpace,
> +                            bool aInReflow)

Remove aCurrentFramePreEffectsOverflow and aInReflow

::: layout/svg/nsSVGIntegrationUtils.cpp:150
(Diff revision 3)
> +                            bool aInReflow)
> +{
> +  PreEffectsVisualOverflowCollector collector(aFirstContinuation,
> +                                              nullptr,
> +                                              aCurrentFramePreEffectsOverflow,
> +                                              aInReflow);

collector(aFirstContinuation, nullptr, nsRect(),
          false);

::: layout/svg/nsSVGIntegrationUtils.cpp:152
(Diff revision 3)
> +  PreEffectsVisualOverflowCollector collector(aFirstContinuation,
> +                                              nullptr,
> +                                              aCurrentFramePreEffectsOverflow,
> +                                              aInReflow);
> +  // Compute overflow areas of current frame relative to aFirstContinuation:
> +  nsLayoutUtils::GetInFlowBoxes(aCurrentFrame, &collector);

nsLayoutUtils::AddBoxesForFrame(aCurrentFrame, &collector);

Comment 16

[u459114]

Comment on attachment 8887876 [details]
Bug 1319407 - Add reftest for box-decoration-break with clip-path.

https://reviewboard.mozilla.org/r/158806/#review164468

LGTM

Comment 17

[Louis Chang [:louis]]

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

Comment 18

[Louis Chang [:louis]]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158272/diff/3-4/

Comment 19

[Louis Chang [:louis]]

Comment on attachment 8887876 [details]
Bug 1319407 - Add reftest for box-decoration-break with clip-path.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158806/diff/2-3/

Comment 20

[u459114]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

https://reviewboard.mozilla.org/r/158272/#review164490

I think, Bug 1319406 should also be fixed by this patch. Please make sure it is. r+ after I get the answer and try result is green

::: layout/base/nsLayoutUtils.cpp:3908
(Diff revision 4)
>    return false;
>  }
>  
> -static void
> -AddBoxesForFrame(nsIFrame* aFrame,
> +void
> +nsLayoutUtils::AddBoxesForFrame(nsIFrame* aFrame,
>                   nsLayoutUtils::BoxCallback* aCallback)

indent is not correct

::: layout/svg/nsSVGIntegrationUtils.cpp:232
(Diff revision 4)
>      nsLayoutUtils::FirstContinuationOrIBSplitSibling(aNonSVGFrame);
>    // 'r' is in "user space":
> -  nsRect r = GetPreEffectsVisualOverflowUnion(firstFrame, nullptr, nsRect(),
> +  nsRect r;
> +  if (aNonSVGFrame->StyleBorder()->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone) {
> +    r = GetPreEffectsVisualOverflow(
> +      firstFrame, aNonSVGFrame, GetOffsetToBoundingBox(firstFrame));

indent is not correct. put firstFrame and aNonSVGFrame right after GetPreEffectsVisualOverflow, and put GetOffsetToBoundingBox(firstFrame) in the next line.

Comment 21

[Louis Chang [:louis]]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158272/diff/4-5/

Comment 22

[Louis Chang [:louis]]

Comment on attachment 8887876 [details]
Bug 1319407 - Add reftest for box-decoration-break with clip-path.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158806/diff/3-4/

Comment 23

[u459114]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

https://reviewboard.mozilla.org/r/158272/#review164534

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

https://reviewboard.mozilla.org/r/158272/#review165558

::: layout/base/nsLayoutUtils.h:1141
(Diff revision 5)
>    public:
>      BoxCallback() : mIncludeCaptionBoxForTable(true) {}
>      virtual void AddBox(nsIFrame* aFrame) = 0;
>      bool mIncludeCaptionBoxForTable;
>    };
> +  static void AddBoxesForFrame(nsIFrame* aFrame, BoxCallback* aCallback);

Maybe add a comment here saying what AddBoxesForFrame does, now that it's a public method?

::: layout/painting/nsDisplayList.cpp:8465
(Diff revision 5)
>  bool nsDisplayMask::TryMerge(nsDisplayItem* aItem)
>  {
>    if (aItem->GetType() != TYPE_MASK)
>      return false;
>  
> +  // Do not merge items with StyleBoxDecorationBreak::Clone prop value

Nit: The comment as it's written basically says the same thing as the code, so I think it's not too useful.  Maybe something like this, instead, which says why we don't do this?

  // Do not merge items for box-decoration-break:clone elements,
  // since each box should have its own mask in that case.

Comment 25

[Louis Chang [:louis]]

Comment on attachment 8887410 [details]
Bug 1319407 - Apply clip-path to each frame when box-decoration-break is clone.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158272/diff/5-6/

Comment 26

[Louis Chang [:louis]]

Comment on attachment 8887876 [details]
Bug 1319407 - Add reftest for box-decoration-break with clip-path.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158806/diff/4-5/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e89eb57f2e97
Apply clip-path to each frame when box-decoration-break is clone. r=cjku,heycam
https://hg.mozilla.org/integration/autoland/rev/8b127b0d655a
Add reftest for box-decoration-break with clip-path. r=cjku

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e89eb57f2e97
https://hg.mozilla.org/mozilla-central/rev/8b127b0d655a