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

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: Oriol, Assigned: lochang)

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

Attachments

(5 attachments, 10 obsolete attachments)

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

Description

2 years ago
Posted file 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

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

Updated

2 years ago
Attachment #8892037 - Attachment is obsolete: true

Comment 5

2 years 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

2 years 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

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

Comment 8

2 years ago
(Assignee)

Comment 10

2 years ago
(Assignee)

Comment 11

2 years ago
status-firefox57: --- → affected
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8892788 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8892790 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8892791 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8892797 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8892798 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years 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.

Comment 22

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Attachment #8892796 - Attachment is obsolete: true
(Assignee)

Updated

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

Updated

2 years ago
Attachment #8892469 - Attachment is obsolete: true

Comment 27

2 years 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

2 years ago
Blocks: 1389068
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 36

2 years 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

2 years 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

2 years 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

2 years ago
Attachment #8892468 - Flags: review?(cam)

Comment 39

2 years 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

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

Updated

2 years ago
Blocks: 1388985
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 47

2 years 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

2 years 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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 53

2 years 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.
Comment hidden (mozreview-request)
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)

Updated

2 years ago
Attachment #8896075 - Attachment is obsolete: true

Comment 64

2 years 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

2 years ago
Blocks: 1390399
(Assignee)

Comment 65

2 years 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

2 years ago
Blocks: 1390399
(Assignee)

Comment 66

2 years 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 67

2 years 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/#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)

Comment 71

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

Comment 75

2 years 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 77

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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: 2 years 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+
Attachment #8896076 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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-
Component: Layout: View Rendering → Layout: Web Painting
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.