Closed Bug 1385745 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Web Painting, defect, P1)

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: Oriol, Assigned: lochang)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(5 files, 10 obsolete files)

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
Attached 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: nobody → lochang
Status: NEW → ASSIGNED
Keywords: regression
Priority: -- → P1
Attachment #8892037 - Attachment is obsolete: true
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 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-
By the way, I said inline boxes but the problem also happens with fragmented block boxes (e.g. in multicol).
Attachment #8892788 - Attachment is obsolete: true
Attachment #8892790 - Attachment is obsolete: true
Attachment #8892791 - Attachment is obsolete: true
Attachment #8892797 - Attachment is obsolete: true
Attachment #8892798 - Attachment is obsolete: true
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 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 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
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 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-
(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.
Attachment #8892796 - Attachment is obsolete: true
Attachment #8892789 - Attachment is obsolete: true
Attachment #8892469 - Attachment is obsolete: true
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-
Blocks: 1389068
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 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 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+
Attachment #8892468 - Flags: review?(cam)
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
Attachment #8896076 - Flags: feedback?(cku) → feedback+
Blocks: 1388985
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 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
(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.
Attachment #8896075 - Attachment is obsolete: true
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)
Blocks: 1390399
(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
Blocks: 1390399
(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 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 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+
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?
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?
(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
(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)
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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
You need to log in before you can comment on or make changes to this bug.