Only first line is visible in inline element with filter and box-decoration-break:clone

RESOLVED FIXED in Firefox 56

Status

()

Core
Layout: View Rendering
P1
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: Oriol, Assigned: louis)

Tracking

(Blocks: 2 bugs, {regression})

56 Branch
mozilla57
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 fixed, firefox57 fixed)

Details

MozReview Requests

()

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

Attachments

(5 attachments, 10 obsolete attachments)

331 bytes, text/html
Details
59 bytes, text/x-review-board-request
cjku
: review+
heycam
: review+
Details | Review
654 bytes, text/html
Details
884 bytes, text/html
Details
59 bytes, text/x-review-board-request
cjku
: review+
Details | Review
(Reporter)

Description

4 months ago
Created attachment 8891851 [details]
testcase.htm

1. Let an inline element be broken among multiple lines
2. Style it with some filter and box-decoration-break:clone

Result: only the first line is visible.

Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cf34b8de856895e7d61b004dc54d2d4f3c4edea3&tochange=8b127b0d655a89da9972b5ee8dd22552c945f9b4
(Assignee)

Updated

4 months ago
Assignee: nobody → lochang
Status: NEW → ASSIGNED
status-firefox55: --- → unaffected
status-firefox56: --- → affected
Keywords: regression
Priority: -- → P1
(Assignee)

Comment 1

4 months ago
Created attachment 8892037 [details] [diff] [review]
[WIP] Bug 1385745 - Only first line is visible in inline element with filter and box-decoration-break:clone

WIP patch for fixing the test case.
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d87d53d14481337c88c8e70ea1b0d71e64f7180
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8892037 - Attachment is obsolete: true
(Assignee)

Comment 4

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93ac1f7f1c748ac02f7f670ed95db9030ae6a3fc

Comment 5

4 months ago
mozreview-review
Comment on attachment 8892468 [details]
Bug 1385745 Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame.

https://reviewboard.mozilla.org/r/163448/#review168806

::: layout/svg/nsSVGUtils.cpp:1305
(Diff revision 1)
>  {
>    if (aFrame &&
>        aUnits->GetAnimValue() == SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
> -    gfxRect bbox = GetBBox(aFrame);
> +    bool shouldUseUnion = true;
> +    const nsStyleBorder* style = aFrame->StyleBorder();
> +    if (aFrame->StyleSVGReset()->HasClipPath() &&

How about mask? Do we only care about clip-path here?

::: layout/svg/nsSVGUtils.cpp:1308
(Diff revision 1)
> -    gfxRect bbox = GetBBox(aFrame);
> +    bool shouldUseUnion = true;
> +    const nsStyleBorder* style = aFrame->StyleBorder();
> +    if (aFrame->StyleSVGReset()->HasClipPath() &&
> +        style->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone) {
> +      shouldUseUnion = false;
> +    }

CSS image-mask does not use this funciton(nsSVGUtils::AdjustMatrixForUnits).
If we have both css image-mask and filter applied on an element, does this solution work?
Attachment #8892468 - Flags: review?(cku) → review-

Comment 6

4 months ago
mozreview-review
Comment on attachment 8892469 [details]
Bug 1385745 Part 2 - Make css clip-path with basic-shape respect box-decoration-break.

https://reviewboard.mozilla.org/r/163450/#review168812

Please add more test cases, and I suggest you working on test case first, and then go back to Part 1.
1. css image-mask + filter
2. SVG mask + filter
Attachment #8892469 - Flags: review?(cku) → review-
(Reporter)

Comment 7

4 months ago
By the way, I said inline boxes but the problem also happens with fragmented block boxes (e.g. in multicol).
(Assignee)

Comment 8

4 months ago
Created attachment 8892788 [details]
box-decoration-break-with-clippath-url.html
(Assignee)

Comment 9

4 months ago
Created attachment 8892789 [details]
box-decoration-break-with-clippath-basic-shape.html
(Assignee)

Comment 10

4 months ago
Created attachment 8892790 [details]
box-decoration-break-with-svg-mask.html
(Assignee)

Comment 11

4 months ago
Created attachment 8892791 [details]
box-decoration-break-with-css-mask.html
(Assignee)

Comment 12

4 months ago
Created attachment 8892793 [details]
box-decoration-break-with-filter.html
(Assignee)

Comment 13

4 months ago
Created attachment 8892794 [details]
box-decoration-break-with-clippath-url-and-filter.html
(Assignee)

Comment 14

4 months ago
Created attachment 8892796 [details]
box-decoration-break-with-clippath-basic-shape-and-filter.html
(Assignee)

Comment 15

4 months ago
Created attachment 8892797 [details]
box-decoration-break-with-svg-mask-and-filter.html
(Assignee)

Comment 16

4 months ago
Created attachment 8892798 [details]
box-decoration-break-with-css-mask-and-filter.html
status-firefox57: --- → affected
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f85be12b7fc3dd534aa56c36c18a38d2919511c8
(Assignee)

Updated

4 months ago
Attachment #8892788 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8892790 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8892791 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8892797 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8892798 - Attachment is obsolete: true
(Assignee)

Comment 20

4 months ago
After offline discussion, I would like to first fix the filter regression and make clip-path with basic-shape respect box-decoration-break in this bug. Then fix css mask in bug 1319406 and svg mask in bug 1388622.
(Assignee)

Comment 21

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60c6bda033fb84b3c95dad9dc3382df786e19594

Comment 22

4 months ago
mozreview-review
Comment on attachment 8892468 [details]
Bug 1385745 Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame.

https://reviewboard.mozilla.org/r/163448/#review171648

::: layout/svg/nsSVGIntegrationUtils.cpp:217
(Diff revision 2)
>                     presContext->AppUnitsToFloatCSSPixels(r.height));
>  }
>  
>  gfxRect
> -nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(nsIFrame* aNonSVGFrame)
> +nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(nsIFrame* aNonSVGFrame,
> +                                                bool aShouldUseUnion)

Rename aShouldUseUnion as aUnionContinuations

::: layout/svg/nsSVGIntegrationUtils.cpp:238
(Diff revision 2)
>                                           GetOffsetToBoundingBox(firstFrame),
>                                           false);
> +  } else {
> +    r = GetPreEffectsVisualOverflow(firstFrame, aNonSVGFrame,
> +                                    GetOffsetToBoundingBox(firstFrame));
>    }

In GetPreEffectsVisualOverflow, please add an assertion:

MOZ_ASSERT(aFirstContinuation == nsLayoutUtils::FirstContinuationOrIBSplitSibling(aCurrentFrame))

::: layout/svg/nsSVGUtils.h:418
(Diff revision 2)
>     *   element's "user space".  A matrix can optionally be pass to specify a
>     *   transform from aFrame's user space to the bounds space of interest
>     *   (typically this will be the ancestor nsSVGOuterSVGFrame, but it could be
>     *   to any other coordinate space).
>     */
>    static gfxRect GetBBox(nsIFrame *aFrame,

Instead of add a new parameter, you can add a new enum value:
BBoxFlags::eInclueAllContinuationsForNoneSVGElement

Please add comment to explain why we need this new value.

::: layout/svg/nsSVGUtils.cpp:1307
(Diff revision 2)
>        aUnits->GetAnimValue() == SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
> -    gfxRect bbox = GetBBox(aFrame);
> +    bool shouldUseUnion = true;
> +    const nsStyleBorder* style = aFrame->StyleBorder();
> +    if (style->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone) {
> +      shouldUseUnion = false;
> +    }

bool unionContinuations = (style->mBoxDecorationBreak == StyleBoxDecorationBreak::Slice);
Attachment #8892468 - Flags: review?(cku) → review-

Comment 23

4 months ago
mozreview-review
Comment on attachment 8892468 [details]
Bug 1385745 Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame.

https://reviewboard.mozilla.org/r/163448/#review172044

One more thing, the commit message should explain what you fix and why instead of copy the summary of the bug
(Assignee)

Updated

4 months ago
Attachment #8892469 - Attachment description: Bug 1385745 Part 2 - Make SVG clip-path with basic-shape respect box-decoration-break. → Bug 1385745 Part 2 - Make css clip-path with basic-shape respect box-decoration-break.

Comment 24

4 months ago
mozreview-review
Comment on attachment 8892469 [details]
Bug 1385745 Part 2 - Make css clip-path with basic-shape respect box-decoration-break.

https://reviewboard.mozilla.org/r/163450/#review172056

The thing that you want to fix in this patch is not relative to the regression of bug 1319407. Please file another issue and put this patch in that new one.

And, I don't think ComputeHTMLReferenceRect is the right place to fix rendering incorrectnees of basic-shape on multiple continuation frame

::: layout/base/nsLayoutUtils.cpp:9556
(Diff revision 2)
>      case StyleGeometryBox::NoBox:
>      case StyleGeometryBox::BorderBox:
>      case StyleGeometryBox::FillBox:
>      case StyleGeometryBox::StrokeBox:
>      case StyleGeometryBox::ViewBox:
> +      if (style->HasClipPath() || style->HasMask()) {

We should also take care of other cases.
Attachment #8892469 - Flags: review?(cku) → review-
(Assignee)

Comment 25

4 months ago
(In reply to C.J. Ku[:cjku](UTC+8) from comment #24)
> Comment on attachment 8892469 [details]
> Bug 1385745 Part 2 - Make css clip-path with basic-shape respect
> box-decoration-break.
> 
> https://reviewboard.mozilla.org/r/163450/#review172056
> 
> The thing that you want to fix in this patch is not relative to the
> regression of bug 1319407. Please file another issue and put this patch in
> that new one.

I filed bug 1388985 to fix this issue.
(Assignee)

Updated

4 months ago
Attachment #8892796 - Attachment is obsolete: true
(Assignee)

Updated

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

Updated

4 months ago
Attachment #8892469 - Attachment is obsolete: true

Comment 27

4 months ago
mozreview-review
Comment on attachment 8892468 [details]
Bug 1385745 Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame.

https://reviewboard.mozilla.org/r/163448/#review172156

almost done

::: layout/svg/nsSVGIntegrationUtils.h:87
(Diff revision 3)
>     * areas, relative to the top-left of the union of all aNonSVGFrame's
>     * continuations' border box rects.
>     */
>    static gfxRect
> -  GetSVGBBoxForNonSVGFrame(nsIFrame* aNonSVGFrame);
> +  GetSVGBBoxForNonSVGFrame(nsIFrame* aNonSVGFrame, bool aUnionContinuations);
>  

Write comment for aUnionContinuations

::: layout/svg/nsSVGIntegrationUtils.cpp:146
(Diff revision 3)
>  GetPreEffectsVisualOverflow(nsIFrame* aFirstContinuation,
>                              nsIFrame* aCurrentFrame,
>                              const nsPoint& aFirstContinuationToUserSpace)
>  {
> +  MOZ_ASSERT(aFirstContinuation ==
> +             nsLayoutUtils::FirstContinuationOrIBSplitSibling(aCurrentFrame));

Please move this change into another patch

::: layout/svg/nsSVGIntegrationUtils.cpp:240
(Diff revision 3)
>                                           GetOffsetToBoundingBox(firstFrame),
>                                           false);
> +  } else {
> +    r = GetPreEffectsVisualOverflow(firstFrame, aNonSVGFrame,
> +                                    GetOffsetToBoundingBox(firstFrame));
>    }

nsRect r = (aUnionContinuations)
 ? GetPreEffectsVisualOverflowUnion(firstFrame, nullptr, nsRect(), GetOffsetToBoundingBox(firstFrame), false)
 : GetPreEffectsVisualOverflow(firstFrame, aNonSVGFrame, GetOffsetToBoundingBox(firstFrame));

::: layout/svg/nsSVGUtils.h:401
(Diff revision 3)
>      // element's bounds to be returned instead.
>      eUseFrameBoundsForOuterSVG = 1 << 6,
>      // https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect
>      eForGetClientRects         = 1 << 7,
> +    // The flag is used to determine whether we should get only current frame
> +    // or all continuation frames for nonSVGElement.

If the given frame is an HTML element, only include the region of the given frame, instead of all continuations of it, while computing bbox if this flag is set.

::: layout/svg/nsSVGUtils.cpp:1103
(Diff revision 3)
>    if (!svg ||
>        (isOuterSVG && (aFlags & eUseFrameBoundsForOuterSVG))) {
>      // An HTML element or an SVG outer frame.
>      MOZ_ASSERT(!hasSVGLayout);
> -    return nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(aFrame);
> +    return nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(
> +      aFrame, !(aFlags & eIncludeOnlyCurrentFrameForNonSVGElement));

Add a local variable for better readibility:
bool onlyCurrentFrame = aFlags & eIncludeOnlyCurrentFrameForNonSVGElement;
return nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(
aFrame, !onlyCurrentFrame);

::: layout/svg/nsSVGUtils.cpp:1307
(Diff revision 3)
>    if (aFrame &&
>        aUnits->GetAnimValue() == SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
> -    gfxRect bbox = GetBBox(aFrame);
> +    const nsStyleBorder* style = aFrame->StyleBorder();
> +    uint32_t aFlags = (style->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone) ?
> +                      eBBoxIncludeFillGeometry | eIncludeOnlyCurrentFrameForNonSVGElement :
> +                      eBBoxIncludeFillGeometry;

You dont't need style since it is used only once.
If you declare this variable to shorten the next statement, you can move "(style->mXXX) ?" to the next line

uint32_t aFlags =
  (aFrame->StyleBorder()->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone)
    ? eBBoxIncludeFillGeometry | eIncludeOnlyCurrentFrameForNonSVGElement
    : eBBoxIncludeFillGeometry;
    
If you really want to keep it, please rename it from  "style" to "borderStyle"
Attachment #8892468 - Flags: review?(cku) → review-

Updated

4 months ago
Blocks: 1389068
(Assignee)

Comment 28

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db9486fa652a37bb0ffb5254c0bfb7e12c934561
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a38f62e429415f4384f6d6ab08d250d22cd4702b
(Assignee)

Comment 36

4 months ago
Comment on attachment 8896076 [details]
Bug 1385745 Part 2 - Add test cases for box-decoration-break.

It seems the patches could fix the regression. But test cases in part 3 have some slightly different in few pixels. I am still looking into it.
Besides, I could not think of a better testcase for filter. The test case box-decoration-break-02.html could only test that box-decoration-break:clone doesn't break filter effects.
Attachment #8896076 - Flags: feedback?(cku)

Comment 37

4 months ago
mozreview-review
Comment on attachment 8896075 [details]
Bug 1385745 Part 2 - Check if aFirstContinuation is the first frame of current frame.

https://reviewboard.mozilla.org/r/167360/#review172626

Since you are using reviewboard, you don't have to append r?=review at the end of commit message. Reviewboard will do it for you when commit to autoland repo
Attachment #8896075 - Flags: review?(cku) → review+

Comment 38

4 months ago
mozreview-review
Comment on attachment 8892468 [details]
Bug 1385745 Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame.

https://reviewboard.mozilla.org/r/163448/#review172630

::: commit-message-80535:2
(Diff revision 5)
> +Bug 1385745 Part 1 - Add a flag to determine whether use current frame or all continuation frames. r?cjku
> +

Bug 1385745 Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame.

::: layout/svg/nsSVGIntegrationUtils.h:84
(Diff revision 5)
>     * about positioning, and to resolve various lengths against. This method
>     * provides the "bbox" for non-SVG frames. The bbox returned is in CSS px
> -   * units, and is the union of all aNonSVGFrame's continuations' overflow
> -   * areas, relative to the top-left of the union of all aNonSVGFrame's
> -   * continuations' border box rects.
> +   * units, and aUnionContinuations decide whether bbox is the area of current
> +   * frame or the union of all aNonSVGFrame's continuations' overflow areas,
> +   * relative to the top-left of the union of all aNonSVGFrame's continuations'
> +   * border box rects.

and aUnionContinuations decide whether bbox contains  the area of current frame only or the union of all aNonSVGFrame's continuations' overflow areas, relative to the top-left of the union of all aNonSVGFrame's continuations'

::: layout/svg/nsSVGUtils.cpp:1305
(Diff revision 5)
>                                   nsSVGEnum *aUnits,
>                                   nsIFrame *aFrame)
>  {
>    if (aFrame &&
>        aUnits->GetAnimValue() == SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
> -    gfxRect bbox = GetBBox(aFrame);
> +    uint32_t aFlags =

uint32_t flags =
Attachment #8892468 - Flags: review?(cku) → review+

Updated

4 months ago
Attachment #8892468 - Flags: review?(cam)

Comment 39

4 months ago
mozreview-review
Comment on attachment 8896076 [details]
Bug 1385745 Part 2 - Add test cases for box-decoration-break.

https://reviewboard.mozilla.org/r/167362/#review172632

Please move these test cases to svg/svg-integration

Updated

4 months ago
Attachment #8896076 - Flags: feedback?(cku) → feedback+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 43

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a662a2fe04f7f8d13c50b20fca1156d4a9fac84a
(Assignee)

Updated

4 months ago
Blocks: 1388985
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 46

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be01797345bae00bdeedd196b4e4b26912de04d

Comment 47

4 months ago
mozreview-review
Comment on attachment 8892468 [details]
Bug 1385745 Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame.

https://reviewboard.mozilla.org/r/163448/#review172704

::: layout/svg/nsSVGUtils.cpp:1309
(Diff revision 6)
>        aUnits->GetAnimValue() == SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
> -    gfxRect bbox = GetBBox(aFrame);
> +    uint32_t flags =
> +      (aFrame->StyleBorder()->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone)
> +        ? eBBoxIncludeFillGeometry | eIncludeOnlyCurrentFrameForNonSVGElement
> +        : eBBoxIncludeFillGeometry;
> +    gfxRect bbox = GetBBox(aFrame, flags);

It's might better to read decoration-break prop at the callers of nsSVGUtils::AdjustMatrixForUnits. There is two callers which are nsSVGClipPathFrame::GetClipPathTransform and nsSVGMaskFrame::GetMaskTransform. Depend on the value of mBoxDecorationBreak pass flags to nsSVGUtils::AdjustMatrixForUnits

gfxMatrix
nsSVGUtils::AdjustMatrixForUnits(const gfxMatrix &aMatrix,
                                 nsSVGEnum *aUnits,
                                 nsIFrame *aFrame,
                                 uint32_t aFlags)

Up to you

Comment 48

4 months ago
mozreview-review
Comment on attachment 8896076 [details]
Bug 1385745 Part 2 - Add test cases for box-decoration-break.

https://reviewboard.mozilla.org/r/167362/#review172826

::: layout/reftests/svg/svg-integration/box-decoration-break-02.html:19
(Diff revision 4)
> +    span {
> +      padding: 0em 1em;
> +      margin-left: 10px;
> +      font: 24px sans-serif;
> +      line-height: 2;
> +      filter: opacity(1);

Or filter: blur(0px) if you don't want to add fuzzy-if in .list file
(Assignee)

Comment 49

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee143652de6c583bd249f9c8fbc1a575e64ade0c
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 53

4 months ago
(In reply to C.J. Ku[:cjku](UTC+8) from comment #48)
> Or filter: blur(0px) if you don't want to add fuzzy-if in .list file

It seems we still have the issue using blur(0px) based on try result in comment 49. So I would just add fuzzy for them.
(Assignee)

Comment 54

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ba3f1ee093b89c0e6d6b0dfec5a6c4ec1bff803
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 60

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78d617858f120cac025b1f63531da1064aa9e2c5
(Assignee)

Comment 61

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8176e355876f72aa4ffb3efd8acc067a620e34fc
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8896075 - Attachment is obsolete: true

Comment 64

4 months ago
mozreview-review
Comment on attachment 8896076 [details]
Bug 1385745 Part 2 - Add test cases for box-decoration-break.

https://reviewboard.mozilla.org/r/167362/#review173774

::: layout/reftests/svg/svg-integration/reftest.list:50
(Diff revision 8)
>  
>  == transform-outer-svg-01.xhtml transform-outer-svg-01-ref.xhtml
> +
> +# box-decoration-break tests
> +== box-decoration-break-01.html box-decoration-break-01-ref.html
> +fuzzy(136,2836) == box-decoration-break-02.html box-decoration-break-02-ref.html

This value is too big. Since we did not use opacity effect, I do not expect this fuzzy by using blur effect
Attachment #8896076 - Flags: review?(cku)
(Assignee)

Updated

4 months ago
Blocks: 1390399
(Assignee)

Comment 65

4 months ago
(In reply to C.J. Ku[:cjku](UTC+8) from comment #64)
> This value is too big. Since we did not use opacity effect, I do not expect
> this fuzzy by using blur effect

I will track the fuzzy issue of blur(0px) on bug 1390399.
Here, I will first use opacity(1) and use fuzzy for the tests.
No longer blocks: 1390399
(Assignee)

Updated

4 months ago
Blocks: 1390399
(Assignee)

Comment 66

4 months ago
(In reply to Louis Chang [:louis] from comment #65)
> I will track the fuzzy issue of blur(0px) on bug 1390399.
> Here, I will first use opacity(1) and use fuzzy for the tests.

It seems opacity(1) has the same fuzzy issue on osx and window.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64a7e6948ad6877ea3401d64dd65fce261bae666
Comment on attachment 8892468 [details]
Bug 1385745 Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame.

https://reviewboard.mozilla.org/r/163448/#review173914

r=me with that.  Sorry for the long delay.  I didn't think about the actual behavior changes closely here; CJ's review should be enough for that.

::: layout/svg/nsSVGClipPathFrame.cpp:468
(Diff revision 9)
> -  return nsSVGUtils::AdjustMatrixForUnits(tm, clipPathUnits, aClippedFrame);
> +  uint32_t flags =
> +    (aClippedFrame->StyleBorder()->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone)
> +      ? nsSVGUtils::eBBoxIncludeFillGeometry |
> +        nsSVGUtils::eIncludeOnlyCurrentFrameForNonSVGElement
> +      : nsSVGUtils::eBBoxIncludeFillGeometry;

Nit: I would probably do either:

  uint32_t flags =
    nsSVGUtils::eBBoxIncludeFillGeometry |
    (aClippedFrame->StyleBorder()->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone
     ? nsSVGUtils::eIncludeOnlyCurrentFrameForNonSVGElement
     : 0);

or

  uint32_t flags = nsSVGUtils::eBBoxIncludeFillGeometry;
  if (aClippedFrame->StyleBorder()->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone) {
    flags |= nsSVGUtils::eIncludeOnlyCurrentFrameForNonSVGElement;
  }

to avoid some repetition.

::: layout/svg/nsSVGMaskFrame.cpp:229
(Diff revision 9)
> +  uint32_t flags =
> +    (aMaskedFrame->StyleBorder()->mBoxDecorationBreak == StyleBoxDecorationBreak::Clone)
> +      ? nsSVGUtils::eBBoxIncludeFillGeometry |
> +        nsSVGUtils::eIncludeOnlyCurrentFrameForNonSVGElement
> +      : nsSVGUtils::eBBoxIncludeFillGeometry;

Nit: Here too.

::: layout/svg/nsSVGUtils.h:375
(Diff revision 9)
>    /**
>     * Take the CTM to userspace for an element, and adjust it to a CTM to its
>     * object bounding box space if aUnits is SVG_UNIT_TYPE_OBJECTBOUNDINGBOX.
>     * (I.e. so that [0,0] is at the top left of its bbox, and [1,1] is at the
>     * bottom right of its bbox).
>     *
>     * If the bbox is empty, this will return a singular matrix.
>     */
>    static gfxMatrix AdjustMatrixForUnits(const gfxMatrix &aMatrix,
>                                          nsSVGEnum *aUnits,
> -                                        nsIFrame *aFrame);
> +                                        nsIFrame *aFrame,
> +                                        uint32_t aFlags);

Please mention what values the new aFlags can take, and how they affect the function.

::: layout/svg/nsSVGUtils.cpp:1104
(Diff revision 9)
>        (isOuterSVG && (aFlags & eUseFrameBoundsForOuterSVG))) {
>      // An HTML element or an SVG outer frame.
>      MOZ_ASSERT(!hasSVGLayout);
> -    return nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(aFrame);
> +    bool onlyCurrentFrame = aFlags & eIncludeOnlyCurrentFrameForNonSVGElement;
> +    return nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(
> +      aFrame, !onlyCurrentFrame);

Nit: For boolean-typed arguments where it's not clear, like here, I prefer to write:

return nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(
  aFrame,
  /* aUnionContinuations = */ !onlyCurrentFrame);

::: layout/svg/nsSVGUtils.cpp:1298
(Diff revision 9)
>  gfxMatrix
>  nsSVGUtils::AdjustMatrixForUnits(const gfxMatrix &aMatrix,
>                                   nsSVGEnum *aUnits,
> -                                 nsIFrame *aFrame)
> +                                 nsIFrame *aFrame,
> +                                 uint32_t aFlags)
>  {
>    if (aFrame &&
>        aUnits->GetAnimValue() == SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
> -    gfxRect bbox = GetBBox(aFrame);
> +    gfxRect bbox = GetBBox(aFrame, aFlags);

If it only ever makes sense to pass in some of the BBoxFlags to this function, please MOZ_ASSERT in here that the flags passed in are only those that are allowed.
Attachment #8892468 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 70

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0256ad6550f2da34dcab32e6b49b0a83cb8f8b7f

Comment 71

4 months ago
mozreview-review
Comment on attachment 8896076 [details]
Bug 1385745 Part 2 - Add test cases for box-decoration-break.

https://reviewboard.mozilla.org/r/167362/#review174294

Almost done
Please change format from html to xhtml. To do this, you need to delare svg namepspace in xhtml
Attachment #8896076 - Flags: review?(cku)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 74

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8785f8ac2e32d68da2bd533938309c0a2e7a9185

Comment 75

4 months ago
mozreview-review
Comment on attachment 8896076 [details]
Bug 1385745 Part 2 - Add test cases for box-decoration-break.

https://reviewboard.mozilla.org/r/167362/#review174440

Thank you for working on this
Attachment #8896076 - Flags: review?(cku) → review+
(Assignee)

Comment 76

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbb7c78af61a1256bb03db46be2af800087046de
(Assignee)

Comment 77

4 months ago
Comment on attachment 8892468 [details]
Bug 1385745 Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame.

Approval Request Comment
[Feature/Bug causing the regression]:Bug 1319407
[User impact if declined]:CSS filter property could not work correctly with box-decoration-break.
[Is this code covered by automated tests?]: Yes, reftest included in Part 2 patch.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: All the patches in this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only effect when box-decoration-break is set to clone.
[String changes made/needed]:No.
Attachment #8892468 - Flags: approval-mozilla-beta?
(Assignee)

Comment 78

4 months ago
Comment on attachment 8896076 [details]
Bug 1385745 Part 2 - Add test cases for box-decoration-break.

Approval Request Comment
[Feature/Bug causing the regression]:Bug 1319407
[User impact if declined]:CSS filter property could not work correctly with box-decoration-break.
[Is this code covered by automated tests?]: Yes, reftest included in Part 2 patch.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: All the patches in this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only effect when box-decoration-break is set to clone.
[String changes made/needed]:No.
Attachment #8896076 - Flags: approval-mozilla-beta?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 81

4 months ago
(In reply to Louis Chang [:louis] from comment #76)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bbb7c78af61a1256bb03db46be2af800087046de

Try looks good.
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(lochang)
Keywords: checkin-needed
(Assignee)

Comment 83

4 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #82)
> Autoland can't push this until all pending issues in MozReview are marked as
> resolved.

Done.
Flags: needinfo?(lochang)

Comment 84

4 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ef585ac7c476
Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame. r=cjku,heycam
https://hg.mozilla.org/integration/autoland/rev/ce31b936f783
Part 2 - Add test cases for box-decoration-break. r=cjku
https://hg.mozilla.org/mozilla-central/rev/ef585ac7c476
https://hg.mozilla.org/mozilla-central/rev/ce31b936f783
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
status-firefox-esr52: --- → unaffected
Comment on attachment 8892468 [details]
Bug 1385745 Part 1 - Add BBoxFlags::eIncludeOnlyCurrentFrameForNonSVGElement to determine whether include all continuations while computing bbox of a html frame.

Fix a regression. Beta56+.
Attachment #8892468 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

4 months ago
Attachment #8896076 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 87

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/06894e19fc5b
https://hg.mozilla.org/releases/mozilla-beta/rev/2bd7fc8ce509
status-firefox56: affected → fixed
Flags: in-testsuite+
(In reply to Louis Chang [:louis] from comment #78)
> [Is this code covered by automated tests?]: Yes, reftest included in Part 2
> patch.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Louis's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.