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

RESOLVED FIXED in Firefox 56

Status

()

Core
Layout: View Rendering
P2
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: u459114, Assigned: louis)

Tracking

(Depends on: 4 bugs, Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

2 years ago
Blocks: 1247229
Status: NEW → ASSIGNED
Priority: -- → P3

Updated

2 years ago
No longer blocks: 1224422
Depends on: 1319406
Component: Layout → Layout: View Rendering
(Reporter)

Updated

2 years ago
Blocks: 1324179
(Reporter)

Comment 1

2 years ago
Created attachment 8820199 [details]
box-decoration-break.html
(Reporter)

Comment 2

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

Updated

2 years ago
No longer blocks: 1247229
(Reporter)

Updated

a year ago
Assignee: cku → lochang
(Assignee)

Comment 3

a year ago
Created attachment 8881551 [details] [diff] [review]
WIP patch

A hacking patch that makes svg clip-path works correctly when using box-decoration-break: clone..
Priority: P3 → P2
(Assignee)

Updated

11 months ago
Attachment #8881551 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

11 months ago
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.
Attachment #8887410 - Flags: review?(cku)
(Assignee)

Comment 7

11 months ago
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.
Attachment #8887411 - Flags: review?(cku)
(Reporter)

Comment 8

11 months ago
mozreview-review
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, .....);
Attachment #8887410 - Flags: review?(cku)
(Reporter)

Comment 9

11 months ago
mozreview-review
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
Attachment #8887411 - Flags: review?(cku)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8887411 - Attachment is obsolete: true
(Reporter)

Comment 11

11 months ago
mozreview-review
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.
Attachment #8887410 - Flags: review?(cku)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 15

11 months ago
mozreview-review
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);
Attachment #8887410 - Flags: review?(cku)
(Reporter)

Comment 16

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

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

LGTM
Attachment #8887876 - Flags: review?(cku) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 20

11 months ago
mozreview-review
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.
Attachment #8887410 - Flags: review?(cku)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 23

11 months ago
mozreview-review
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
Attachment #8887410 - Flags: review?(cku) → review+
(Reporter)

Updated

11 months ago
Attachment #8887410 - Flags: review?(cam)

Comment 24

11 months ago
mozreview-review
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.
Attachment #8887410 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 27

11 months ago
Pushed by cku@mozilla.com:
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e89eb57f2e97
https://hg.mozilla.org/mozilla-central/rev/8b127b0d655a
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

11 months ago
Depends on: 1385745
(Assignee)

Updated

11 months ago
Depends on: 1388622
(Assignee)

Updated

11 months ago
Depends on: 1388985
(Assignee)

Updated

10 months ago
Depends on: 1391143
You need to log in before you can comment on or make changes to this bug.