Closed
Bug 1311270
Opened 7 years ago
Closed 6 years ago
Implement rendering of mask-clip/mask-origin: fill-box|stroke-box|view-box|no-clip values
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: ethlin, Assigned: u459114)
References
(Blocks 2 open bugs)
Details
Attachments
(9 files, 4 obsolete files)
58 bytes,
text/x-review-board-request
|
heycam
:
review+
TYLin
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
TYLin
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
TYLin
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
TYLin
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
Per Bug 1303623 comment 4, create another bug for rendering issues of mask-clip/mask-origin in nsCSSRendering + nsDisplayMask + nsSVGIntergrationUtils.
Blocks: mask-image
Updated•6 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
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) |
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 15•6 years ago
|
||
Update WIP patches. Several things that still unfinished 1. mask-origin: fill-box/ stroke-box/ view-box/ margin-box 2. Test case for all these new mask property values.
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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8813093 -
Attachment is obsolete: true
Attachment #8813798 -
Attachment is obsolete: true
Attachment #8813801 -
Attachment is obsolete: true
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8813092 [details] Bug 1311270 - Part 1. Move geomery box computing from nsCSSClipPathInstance to nsLayoutUtils. https://reviewboard.mozilla.org/r/94614/#review95612 I assume ComputeSVGReferenceRect(), ComputeHTMLReferenceRect(), and the logic inside ComputeGeometryBox() are simply copied and pasted to nsLayoutUtils. r=TYLin ::: layout/base/nsLayoutUtils.cpp:9253 (Diff revision 3) > + return r; > +} > + > +/* static */ nsRect > +nsLayoutUtils::ComputeGeometryBox(nsIFrame* aFrame, > + StyleClipPathGeometryBox geometryBox) Nit: Please fix this indentation.
Attachment #8813092 -
Flags: review?(tlin) → review+
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 32•6 years ago
|
||
mozreview-review |
Comment on attachment 8813092 [details] Bug 1311270 - Part 1. Move geomery box computing from nsCSSClipPathInstance to nsLayoutUtils. https://reviewboard.mozilla.org/r/94614/#review95616 ::: layout/base/nsLayoutUtils.h:2870 (Diff revision 3) > * next line exists, null otherwise. > */ > static bool IsInvisibleBreak(nsINode* aNode, nsIFrame** aNextLineFrame = nullptr); > > + static nsRect ComputeGeometryBox(nsIFrame* aFrame, > + StyleClipPathGeometryBox geometryBox); Nit: aGeometryBox. ::: layout/base/nsLayoutUtils.cpp:9144 (Diff revision 3) > return lineNonEmpty; > } > + > +static nsRect > +ComputeSVGReferenceRect(nsIFrame* aFrame, > + StyleClipPathGeometryBox geometryBox) Nit: Again, aGeometryBox. ::: layout/base/nsLayoutUtils.cpp:9219 (Diff revision 3) > + return r; > +} > + > +static nsRect > +ComputeHTMLReferenceRect(nsIFrame* aFrame, > + StyleClipPathGeometryBox geometryBox) Nit: aGeometryBox. ::: layout/base/nsLayoutUtils.cpp:9253 (Diff revision 3) > + return r; > +} > + > +/* static */ nsRect > +nsLayoutUtils::ComputeGeometryBox(nsIFrame* aFrame, > + StyleClipPathGeometryBox geometryBox) Nit: Also aGeometryBox.
Comment 33•6 years ago
|
||
mozreview-review |
Comment on attachment 8813094 [details] Bug 1311270 - Part 2. Rename StyleClipPathGeometryBox to StyleGeometryBox and extend contained values. https://reviewboard.mozilla.org/r/94618/#review95624 r- because I feel it'll be better not to remove margin-box support for clip-path in this patch. ::: layout/base/nsLayoutUtils.cpp (Diff revision 3) > - case StyleClipPathGeometryBox::NoBox: > - case StyleClipPathGeometryBox::Border: > - case StyleClipPathGeometryBox::Content: > - case StyleClipPathGeometryBox::Padding: > - case StyleClipPathGeometryBox::Margin: > + case StyleGeometryBox::NoBox: > + case StyleGeometryBox::Border: > + case StyleGeometryBox::Content: > + case StyleGeometryBox::Padding: > + case StyleGeometryBox::Fill: { > - case StyleClipPathGeometryBox::Fill: { margin-box is required by clip-path. We should keep this. ::: layout/base/nsLayoutUtils.cpp:9231 (Diff revision 3) > - case StyleClipPathGeometryBox::Margin: > - r = aFrame->GetMarginRectRelativeToSelf(); > - break; > + case StyleGeometryBox::NoBox: > + case StyleGeometryBox::Border: > + case StyleGeometryBox::Fill: I think StyleGeometryBox::Margin is still needed. ::: layout/style/nsStyleConsts.h:143 (Diff revision 3) > -enum class StyleClipPathGeometryBox : uint8_t { > - NoBox, > +// background-origin, mask-clip and mask-origin. > +enum class StyleGeometryBox : uint8_t { > Content, > Padding, > Border, > - Margin, > + Margin, // clip-path reference-box only. Better to mention Bug 1260094 here, and say something like "Although margin-box is reqired by mask-origin and mask-clip, we don't implement that due to the lack support of other browsers. ::: layout/style/nsStyleConsts.h:149 (Diff revision 3) > - Fill, > - Stroke, > - View, > + Fill, // mask-clip, mask-origin and clip-path reference-box only. > + Stroke, // mask-clip, mask-origin and clip-path reference-box only. > + View, // mask-clip, mask-origin and clip-path reference-box only. > + NoClip, // mask-clip only. > + Text, // background-clip only. > + NoBox, // Depend on which kind of element this style value applied on, Nit: s/Depend/Depending/ ::: layout/style/nsStyleConsts.h:151 (Diff revision 3) > - View, > + View, // mask-clip, mask-origin and clip-path reference-box only. > + NoClip, // mask-clip only. > + Text, // background-clip only. > + NoBox, // Depend on which kind of element this style value applied on, > + // the initial value of a reference-box can be different. > + // For a HTML element, the initial value of reference-box is Nit: s/a HTML/an HTML ::: layout/style/nsStyleConsts.h:151 (Diff revision 3) > + // For a HTML element, the initial value of reference-box is > + // border-box; for a SVG element, the initial value is fill-box. Nit: s/a HTML/an HTML/, and s/a SVG/an SVG/
Attachment #8813094 -
Flags: review?(tlin) → review-
Comment 34•6 years ago
|
||
mozreview-review |
Comment on attachment 8813095 [details] Bug 1311270 - Part 3. Replace {bg|mask}-origin/{bg|mask}-clip constants by StyleGeometryBox. https://reviewboard.mozilla.org/r/94620/#review95632 r=TYLin with the inline comments addresssed. ::: layout/style/nsComputedDOMStyle.h (Diff revision 3) > > already_AddRefed<CSSValue> GetCSSShadowArray(nsCSSShadowArray* aArray, > const nscolor& aDefaultColor, > bool aIsBoxShadow); > > - already_AddRefed<CSSValue> GetBackgroundList( Any reason to convert the two methods GetBackgroundList() and GetRoCSSValueList() to *non-static* function in cpp? They are both marked /\*static\*/ which it is untrue. I'll prefer them remain as methods. ::: layout/style/nsComputedDOMStyle.cpp:1805 (Diff revision 3) > > return val.forget(); > } > > + > +/*static*/ template <typename T> Nit: We usually don't have a space between template and <> ::: layout/style/nsStyleStruct.h:802 (Diff revision 3) > // or an ImageValue.) > mozilla::Position mPosition; // [reset] > Size mSize; // [reset] > - uint8_t mClip; // [reset] See nsStyleConsts.h > + StyleGeometryBox mClip; // [reset] See nsStyleConsts.h > MOZ_INIT_OUTSIDE_CTOR > - uint8_t mOrigin; // [reset] See nsStyleConsts.h > + StyleGeometryBox mOrigin; // [reset] See nsStyleConsts.h Nit: Please align StyleGeometryBox, and the comment.
Attachment #8813095 -
Flags: review?(tlin) → review+
Assignee | ||
Comment 35•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8813095 [details] Bug 1311270 - Part 3. Replace {bg|mask}-origin/{bg|mask}-clip constants by StyleGeometryBox. https://reviewboard.mozilla.org/r/94620/#review95632 > Nit: Please align StyleGeometryBox, and the comment. MOZ_INIT_OUTSIDE_CTOR StyleGeometryBox mOrigin These two lines is broken from one line, so this identation is needed.
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8814304 [details] Bug 1311270 - Part 9. Test cases for new mask-clip/mask-origin values. https://reviewboard.mozilla.org/r/95564/#review95646 ::: layout/reftests/w3c-css/submitted/masking/mask-origin-3-ref.html:6 (Diff revision 2) > +<!DOCTYPE html> > +<html> > + <head> > + <meta charset="utf-8"> > + <title>CSS Masking: mask-origin: mask positioning area</title> > + <link rel="author" title="Astley Chen" href="mailto:aschen@mozilla.com"> Please correct the author name.
Attachment #8814304 -
Flags: review?(tlin) → review+
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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 46•6 years ago
|
||
mozreview-review |
Comment on attachment 8813094 [details] Bug 1311270 - Part 2. Rename StyleClipPathGeometryBox to StyleGeometryBox and extend contained values. https://reviewboard.mozilla.org/r/94618/#review95650
Attachment #8813094 -
Flags: review?(tlin) → review+
Attachment #8813799 -
Flags: review?(mstange)
Attachment #8813800 -
Flags: review?(mstange)
Attachment #8813802 -
Flags: review?(mstange)
Attachment #8814302 -
Flags: review?(mstange)
Comment 47•6 years ago
|
||
mozreview-review |
Comment on attachment 8813092 [details] Bug 1311270 - Part 1. Move geomery box computing from nsCSSClipPathInstance to nsLayoutUtils. https://reviewboard.mozilla.org/r/94614/#review95652 s/geomery/geometry/ in the commit message.
Attachment #8813092 -
Flags: review?(cam) → review+
Comment 48•6 years ago
|
||
mozreview-review |
Comment on attachment 8813094 [details] Bug 1311270 - Part 2. Rename StyleClipPathGeometryBox to StyleGeometryBox and extend contained values. https://reviewboard.mozilla.org/r/94618/#review95656 ::: layout/style/nsStyleConsts.h:145 (Diff revision 4) > Content, > Padding, > Border, > - Margin, > - Fill, > - Stroke, > + Margin, // XXX Bug 1260094 comment 9. > + // Although margin-box is required by mask-origin and mask-clip, we > + // don't implement that due to lack support of other browser. Nit: s/lack support of other browser/lack of support in other browsers/. ::: layout/style/nsStyleConsts.h:152 (Diff revision 4) > + NoBox, // Depending on which kind of element this style value applied on, > + // the initial value of a reference-box can be different. > + // For an HTML element, the initial value of reference-box is > + // border-box; for an SVG element, the initial value is fill-box. > + // Since we can not determine the initial value at parsing time, > + // set it as NoBox so that we make a decision later. > + // clip-path reference-box only. Nit: I would say "default" rather than "initial", because "initial" sounds like it is talking about the CSS property initial value (which for clip-path is 'none').
Attachment #8813094 -
Flags: review?(cam) → review+
Comment 49•6 years ago
|
||
mozreview-review |
Comment on attachment 8813095 [details] Bug 1311270 - Part 3. Replace {bg|mask}-origin/{bg|mask}-clip constants by StyleGeometryBox. https://reviewboard.mozilla.org/r/94620/#review95660 ::: layout/painting/nsCSSRendering.cpp:3350 (Diff revision 4) > // Background images are tiled over the 'background-clip' area > // but the origin of the tiling is based on the 'background-origin' area > // XXX: Bug 1303623 will bring in new origin value, we should iterate from > - // NS_STYLE_IMAGELAYER_ORIGIN_MARGIN instead of > - // NS_STYLE_IMAGELAYER_ORIGIN_BORDER. > + // StyleGeometryBox::Margin instead of > + // StyleGeometryBox::Border. Remove this comment? I guess it's out of date now that we don't have margin-box for background. ::: layout/style/nsComputedDOMStyle.cpp:1819 (Diff revision 4) > +nsComputedDOMStyle::GetBackgroundList(StyleGeometryBox nsStyleImageLayers::Layer::* aMember, > + uint32_t nsStyleImageLayers::* aCount, > + const nsStyleImageLayers& aLayers, > + const KTableEntry aTable[]) The function isn't too long, so it's not so bad, but it would be nice to avoid the duplication if possible. Especially if later we convert other image layer properties (like background-attachment, etc.) to be enum classes. I think just making GetBackgroundList a template function with a template argument T used in place of StyleGeometryBox in the aMember type should work. ::: layout/style/nsComputedDOMStyle.cpp:1828 (Diff revision 4) > + uint8_t index = static_cast<uint8_t>(aLayers.mLayers[i].*aMember); > + val->SetIdent(nsCSSProps::ValueToKeywordEnum(index, aTable)); I think you shouldn't need to cast to a uint8_t here, since there is a templated nsCSSProps::ValueToKeywordEnum function. ::: layout/style/nsRuleNode.cpp:6735 (Diff revision 4) > +template <> > +struct BackgroundItemComputer<nsCSSValueList, StyleGeometryBox> > +{ > + static void ComputeValue(nsStyleContext* aStyleContext, > + const nsCSSValueList* aSpecifiedValue, > + StyleGeometryBox& aComputedValue, > + RuleNodeCacheConditions& aConditions) > + { > + aComputedValue = > + static_cast<StyleGeometryBox>(aSpecifiedValue->mValue.GetIntValue()); > + } > +}; I think you could avoid this copy of BackgroundItemComputer too, if you want, by making the uint8_t version a bit more general, like: template<typename T> struct BackgroundItemComputer<nsCSSValueList, EnableIf<IsIntegral<T>::value || IsEnum<T>::value>::Type> { static void ComputeValue(..., T& aComputedValue, ...) { aComputedValue = static_cast<T>(...); } }; Again, we'll probably need to do that later anyway, once we convert more image layer properties to enum classes. ::: layout/style/nsStyleStruct.h:802 (Diff revision 4) > // or an ImageValue.) > mozilla::Position mPosition; // [reset] > Size mSize; // [reset] > - uint8_t mClip; // [reset] See nsStyleConsts.h > + StyleGeometryBox mClip; // [reset] See nsStyleConsts.h > MOZ_INIT_OUTSIDE_CTOR > - uint8_t mOrigin; // [reset] See nsStyleConsts.h > + StyleGeometryBox mOrigin; // [reset] See nsStyleConsts.h We could still align the comment, though...
Attachment #8813095 -
Flags: review?(cam) → review+
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) |
Comment hidden (mozreview-request) |
Attachment #8814303 -
Attachment is obsolete: true
Attachment #8814303 -
Flags: review?(jfkthame)
Comment 58•6 years ago
|
||
mozreview-review |
Comment on attachment 8813799 [details] Bug 1311270 - Part 4. Handle rendering of mask-clip:no-clip. https://reviewboard.mozilla.org/r/95160/#review96618
Attachment #8813799 -
Flags: review?(mstange) → review+
Comment 59•6 years ago
|
||
mozreview-review |
Comment on attachment 8813800 [details] Bug 1311270 - Part 5. Handle rendering of mask-clip:fill-box/stroke-box/view-box. https://reviewboard.mozilla.org/r/95162/#review96620 ::: layout/painting/nsCSSRendering.cpp:1903 (Diff revision 5) > MOZ_ASSERT(!aDirtyRect->IsEmpty() || aDirtyRectGfx->IsEmpty(), > "second should be empty if first is"); > } > > +static bool > +IsSVGStyleGeomertyBox(StyleGeometryBox aBox) Geomerty -> Geometry, here and in other places ::: layout/painting/nsCSSRendering.cpp:1922 (Diff revision 5) > + if (aForFrame->IsFrameOfType(nsIFrame::eSVG) && > + (aForFrame->GetType() != nsGkAtoms::svgOuterSVGFrame)) { Could this check be replaced with (aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT)? I don't really know the exact differences between the two. Ah, I see that further down you assert that these are actually the same thing. So I think it would be good to just use NS_FRAME_SVG_LAYOUT in both places, and maybe even drop the asserts (up to you). ::: layout/painting/nsCSSRendering.cpp:1959 (Diff revision 5) > + nsPoint toStrokeBoxOffset = clipArea.TopLeft() - strokeBox.TopLeft(); > + aClipState->mBGClipArea = > + nsRect(aBorderArea.TopLeft() + toStrokeBoxOffset, clipArea.Size()); This doesn't seem quite right. Can you explain what each of these rects is relative to, and why subtracting and adding the points in this way is the right thing to do? What would happen if you just did aClipState->mBGClipArea = clipArea;?
Comment 60•6 years ago
|
||
mozreview-review |
Comment on attachment 8813802 [details] Bug 1311270 - Part 6. Handle mask-origin. https://reviewboard.mozilla.org/r/95166/#review96624 ::: layout/painting/nsCSSRendering.cpp:3469 (Diff revision 5) > + nsPoint toStrokeBoxOffset = > + bgPositioningArea.TopLeft() - strokeBox.TopLeft(); > + > + return nsRect(toStrokeBoxOffset, bgPositioningArea.Size()); So is this assuming that the returned rect needs to be relative to the stroke box? Is that the case?
Comment 61•6 years ago
|
||
mozreview-review |
Comment on attachment 8814302 [details] Bug 1311270 - Part 7. Rename local variable in GetImageLayerClip and ComputeImageLayerPositioningArea. https://reviewboard.mozilla.org/r/95560/#review96626
Attachment #8814302 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 62•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8813802 [details] Bug 1311270 - Part 6. Handle mask-origin. https://reviewboard.mozilla.org/r/95166/#review96624 > So is this assuming that the returned rect needs to be relative to the stroke box? Is that the case? I would say relative to aBoderArea(which is border-box for HTML element and stroke-box for SVG element). But since aBorderArea == stroke-box, the answer is yes.
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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 72•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8813800 [details] Bug 1311270 - Part 5. Handle rendering of mask-clip:fill-box/stroke-box/view-box. https://reviewboard.mozilla.org/r/95162/#review96620 > Could this check be replaced with (aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT)? I don't really know the exact differences between the two. > > Ah, I see that further down you assert that these are actually the same thing. So I think it would be good to just use NS_FRAME_SVG_LAYOUT in both places, and maybe even drop the asserts (up to you). I should change that assertion. Unlike other SVG elements, <svg> is the only SVG element associate with CSS layout box. I learn that from heycam's review. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1289011#c25 > This doesn't seem quite right. Can you explain what each of these rects is relative to, and why subtracting and adding the points in this way is the right thing to do? > What would happen if you just did aClipState->mBGClipArea = clipArea;? clipArea - According to mask-clip prop value, it can be view-box, fill-box or stroke-box in *svg user space*. strokeBox - stroke-box in *svg user space*. aBoderArea - Depend on the caller, it can be stroke-box area in the *refernence-frame's space* or in *svg user space*, or anything else. The coordinate space of the returning value(aClipState->mBGClipArea/mDirtyRect etc) - be the same with aBoderArea. we can not always set "aClipState->mBGClipArea = clipArea" directly, since clipArea is in svg user space, but according aBoderArea we need mBGClipArea be in reference-frame's space sometimes. I will add more comments to explain detail in code.
Attachment #8815630 -
Flags: review?(mstange)
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) |
Comment hidden (mozreview-request) |
Comment 81•6 years ago
|
||
mozreview-review |
Comment on attachment 8815630 [details] Bug 1311270 - Part 8. Add assertion for margin-box. https://reviewboard.mozilla.org/r/96498/#review96790
Attachment #8815630 -
Flags: review?(mstange) → review+
Comment 82•6 years ago
|
||
(In reply to C.J. Ku[:cjku](UTC+8) from comment #72) > > This doesn't seem quite right. Can you explain what each of these rects is relative to, and why subtracting and adding the points in this way is the right thing to do? > > What would happen if you just did aClipState->mBGClipArea = clipArea;? > > clipArea - According to mask-clip prop value, it can be view-box, fill-box > or stroke-box in *svg user space*. > strokeBox - stroke-box in *svg user space*. > aBoderArea - Depend on the caller, it can be stroke-box area in the > *refernence-frame's space* or in *svg user space*, or anything else. > The coordinate space of the returning > value(aClipState->mBGClipArea/mDirtyRect etc) - be the same with aBoderArea. > > we can not always set "aClipState->mBGClipArea = clipArea" directly, since > clipArea is in svg user space, but according aBoderArea we need mBGClipArea > be in reference-frame's space sometimes. > I will add more comments to explain detail in code. Thanks, I'll need to think about this some more. Sorry that this is taking so long.
Comment 83•6 years ago
|
||
mozreview-review |
Comment on attachment 8814304 [details] Bug 1311270 - Part 9. Test cases for new mask-clip/mask-origin values. https://reviewboard.mozilla.org/r/95564/#review100990 Sorry, forgot about this one.
Attachment #8814304 -
Flags: review?(cam) → review+
Comment 84•6 years ago
|
||
mozreview-review |
Comment on attachment 8813800 [details] Bug 1311270 - Part 5. Handle rendering of mask-clip:fill-box/stroke-box/view-box. https://reviewboard.mozilla.org/r/95162/#review101128 Ok, it makes sense to me now. I think it would be easier to understand if it were written like this: ``` nsRect clipArea = ...; nsRect strokeBox = (backgroundClip == StyleGeometryBox::Stroke) ? clipArea : nsLayoutUtils::ComputeGeometryBox(aForFrame, StyleGeometryBox::Stroke); nsRect clipAreaRelativeToStrokeBox = clipArea - strokeBox.TopLeft(); aClipState->mBGClipArea = clipAreaRelativeToStrokeBox + aBorderArea.TopLeft(); ``` but I can see how that would be slightly slower.
Attachment #8813800 -
Flags: review?(mstange) → review+
Comment 85•6 years ago
|
||
mozreview-review |
Comment on attachment 8813802 [details] Bug 1311270 - Part 6. Handle mask-origin. https://reviewboard.mozilla.org/r/95166/#review101132 ::: layout/painting/nsCSSRendering.cpp:3494 (Diff revision 7) > + nsLayoutUtils::ComputeGeometryBox(aForFrame, > + StyleGeometryBox::Stroke); > + toStrokeBoxOffset = bgPositioningArea.TopLeft() - strokeBox.TopLeft(); > + } > + > + return nsRect(toStrokeBoxOffset, bgPositioningArea.Size()); Please add a comment that says that for SVG frames the return value is relative to the stroke box.
Attachment #8813802 -
Flags: review?(mstange) → review+
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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 95•6 years ago
|
||
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1cf53de2b4 Part 1. Move geomery box computing from nsCSSClipPathInstance to nsLayoutUtils. https://hg.mozilla.org/integration/mozilla-inbound/rev/3f11d4c79554 Part 2. Rename StyleClipPathGeometryBox to StyleGeometryBox and extend contained values. https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc272b52c9a Part 3. Replace {bg|mask}-origin/{bg|mask}-clip constants by StyleGeometryBox. https://hg.mozilla.org/integration/mozilla-inbound/rev/36b977bb8bbe Part 4. Handle rendering of mask-clip:no-clip. https://hg.mozilla.org/integration/mozilla-inbound/rev/6509a168badf Part 5. Handle rendering of mask-clip:fill-box/stroke-box/view-box. https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0ac433a100 Part 6. Handle mask-origin. https://hg.mozilla.org/integration/mozilla-inbound/rev/d83d4dd9f36a Part 7. Rename local variable in GetImageLayerClip and ComputeImageLayerPositioningArea. https://hg.mozilla.org/integration/mozilla-inbound/rev/0627bbd0b8c3 Part 8. Add assertion for margin-box. https://hg.mozilla.org/integration/mozilla-inbound/rev/c82b1188b47a Part 9. Test cases for new mask-clip/mask-origin values.
Comment 96•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6d1cf53de2b4 https://hg.mozilla.org/mozilla-central/rev/3f11d4c79554 https://hg.mozilla.org/mozilla-central/rev/9fc272b52c9a https://hg.mozilla.org/mozilla-central/rev/36b977bb8bbe https://hg.mozilla.org/mozilla-central/rev/6509a168badf https://hg.mozilla.org/mozilla-central/rev/1b0ac433a100 https://hg.mozilla.org/mozilla-central/rev/d83d4dd9f36a https://hg.mozilla.org/mozilla-central/rev/0627bbd0b8c3 https://hg.mozilla.org/mozilla-central/rev/c82b1188b47a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•