Closed
Bug 1385745
Opened 7 years ago
Closed 7 years ago
Only first line is visible in inline element with filter and box-decoration-break:clone
Categories
(Core :: Web Painting, defect, P1)
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+
gchang
:
approval-mozilla-beta+
|
Details |
654 bytes,
text/html
|
Details | |
884 bytes,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
u459114
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
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•7 years ago
|
Assignee: nobody → lochang
Status: NEW → ASSIGNED
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
Keywords: regression
Priority: -- → P1
Assignee | ||
Comment 1•7 years ago
|
||
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•7 years ago
|
Attachment #8892037 -
Attachment is obsolete: true
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93ac1f7f1c748ac02f7f670ed95db9030ae6a3fc
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-
Reporter | ||
Comment 7•7 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•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
Assignee | ||
Comment 11•7 years ago
|
||
Assignee | ||
Comment 12•7 years ago
|
||
Assignee | ||
Comment 13•7 years ago
|
||
Assignee | ||
Comment 14•7 years ago
|
||
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Comment 16•7 years ago
|
||
Updated•7 years ago
|
status-firefox57:
--- → affected
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f85be12b7fc3dd534aa56c36c18a38d2919511c8
Assignee | ||
Updated•7 years ago
|
Attachment #8892788 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8892790 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8892791 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8892797 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8892798 -
Attachment is obsolete: true
Assignee | ||
Comment 20•7 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.
Assignee | ||
Comment 21•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60c6bda033fb84b3c95dad9dc3382df786e19594
Comment 22•7 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•7 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•7 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•7 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•7 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•7 years ago
|
Attachment #8892796 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8892789 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8892469 -
Attachment is obsolete: true
Comment 27•7 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-
Assignee | ||
Comment 28•7 years 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•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a38f62e429415f4384f6d6ab08d250d22cd4702b
Assignee | ||
Comment 36•7 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•7 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•7 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+
Attachment #8892468 -
Flags: review?(cam)
Comment 39•7 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
Attachment #8896076 -
Flags: feedback?(cku) → feedback+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a662a2fe04f7f8d13c50b20fca1156d4a9fac84a
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 46•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be01797345bae00bdeedd196b4e4b26912de04d
Comment 47•7 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•7 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
Assignee | ||
Comment 49•7 years 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•7 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.
Assignee | ||
Comment 54•7 years 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•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78d617858f120cac025b1f63531da1064aa9e2c5
Assignee | ||
Comment 61•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8176e355876f72aa4ffb3efd8acc067a620e34fc
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8896075 -
Attachment is obsolete: true
Comment 64•7 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 | ||
Comment 65•7 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 | ||
Comment 66•7 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•7 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) |
Assignee | ||
Comment 70•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0256ad6550f2da34dcab32e6b49b0a83cb8f8b7f
Comment 71•7 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) |
Assignee | ||
Comment 74•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8785f8ac2e32d68da2bd533938309c0a2e7a9185
Comment 75•7 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 76•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbb7c78af61a1256bb03db46be2af800087046de
Assignee | ||
Comment 77•7 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•7 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•7 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
Comment 82•7 years ago
|
||
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(lochang)
Keywords: checkin-needed
Assignee | ||
Comment 83•7 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•7 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
Comment 85•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ef585ac7c476 https://hg.mozilla.org/mozilla-central/rev/ce31b936f783
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 86•7 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. Fix a regression. Beta56+.
Attachment #8892468 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8896076 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 87•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/06894e19fc5b https://hg.mozilla.org/releases/mozilla-beta/rev/2bd7fc8ce509
Flags: in-testsuite+
Comment 88•7 years ago
|
||
(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-
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•