Implement CSS Mask Image properties (mask, mask-image, etc.)

RESOLVED FIXED in Firefox 47

Status

()

enhancement
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: amfibia95, Assigned: u459114)

Tracking

(Blocks 3 bugs, {dev-doc-complete})

Trunk
mozilla47
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: p=0, [DocArea=CSS], )

Attachments

(13 attachments, 41 obsolete attachments)

84.13 KB, patch
u459114
: review+
Details | Diff | Splinter Review
81.44 KB, patch
u459114
: review+
Details | Diff | Splinter Review
30.28 KB, patch
u459114
: review+
Details | Diff | Splinter Review
58.44 KB, patch
u459114
: review+
Details | Diff | Splinter Review
14.89 KB, patch
u459114
: review+
Details | Diff | Splinter Review
9.95 KB, patch
u459114
: review+
Details | Diff | Splinter Review
1.72 KB, patch
u459114
: review+
Details | Diff | Splinter Review
15.37 KB, patch
u459114
: review+
Details | Diff | Splinter Review
2.56 KB, patch
u459114
: review+
Details | Diff | Splinter Review
864 bytes, patch
u459114
: review+
Details | Diff | Splinter Review
10.72 KB, patch
u459114
: review+
Details | Diff | Splinter Review
14.25 KB, patch
u459114
: review+
Details | Diff | Splinter Review
42.45 KB, patch
u459114
: review+
Details | Diff | Splinter Review
Reporter

Description

8 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 Firefox/7.0
Build ID: 20110908135051

Steps to reproduce:

I suggest adding a new CSS property-moz-mask.
Make analog: http://www.webkit.org/blog/181/css-masks/
Reporter

Updated

8 years ago
Version: 7 Branch → unspecified
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Summary: CSS Mask Image (-moz-mask) proporse → Implement CSS Mask Image (-moz-mask) proposal

Updated

8 years ago
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
We already support this, using SVG masks, no?
Two different approaches. SVG is probably more powerful, but using an image is easier.
Supporting the image syntax as well as SVG makes sense.
Reporter

Comment 4

8 years ago
Да лучше бы реализовали CSS Content, а то JNG приходится чинить за счет места в src. А из-за этого сохранить JNG нельзя!

Comment 5

7 years ago
Will mask-image accept the -moz-element() image type? I'd love to use text as a mask for an image (=text with a pattern).
SVG masks already let you use text as a mask.

Comment 7

7 years ago
Han any on had experience in animating or using transition on a svg mask?

Comment 8

7 years ago
I was first hoping I would be able to use a CSS gradient for a mask. When I found I couldn't do that I was then hoping I could use an image, preferably a data uri I could embed right into the CSS, but alas it currently requires an external SVG file, a file type I would rather not have to deal with.
See also the related W3C Working Draft at http://www.w3.org/TR/css-masking/
I just voted for this bug, so let me explain:
SVG masks are theoretically more powerful and versatile. It's only when you use it in practice that you notice its main issue: performance. When you try to use it for browser-chrome (applied to XUL), you notice that hardware-accelerated rendering is problematic and slow.
I have no benchmarks backing this statement at the moment, but I believe when you ask the UX or fx-desktop team members, you'll get a confirmation nonetheless.

Hardware-accelerated rendering of a raster of pixels (with and without alpha-blending), however, is a solved problem! If we would have had this feature right now, implementing customizable curvy tabs, for example, would've been jet-boosted.
But let's not stop at curvy tabs! Any custom-shaped panel, overlays, toolbars, multi-layer buttons (do I hear mobile?), etc.

To make things scale properly across multiple resolutions and/ or pixel densities, developers will have to use @media queries in this case, though, which is less flexible than what SVG offers out of the box. But this is a trade-off that developers can reason about, which they have to do anyway nowadays when they prepare content for various form factors.

So, having this feature implemented would be a BIG step forward imho!
We will do this since there's a spec for it now, but we should also accelerate rendering of SVG masks.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> We will do this since there's a spec for it now, but we should also
> accelerate rendering of SVG masks.

Epic, on both counts! I guess this won't just happen overnight, but good to hear nonetheless.

Updated

6 years ago
Assignee: nobody → ncameron
Blocks: 877294
Depends on: 700926
Whiteboard: [Australis:P?][Australis:M?]
Whiteboard: [Australis:P?][Australis:M?]
Summary: Implement CSS Mask Image (-moz-mask) proposal → Implement CSS Mask Image properties (mask, mask-image, etc.)

Comment 13

6 years ago
The spec (http://www.w3.org/TR/css-masking/) is in last call, the editor's version has been updated since then, though.
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+

Comment 14

5 years ago
Is mask-size in the spec and proposition?

Comment 15

5 years ago
(In reply to adrian from comment #14)
> Is mask-size in the spec and proposition?

mask-size and mask-image, mask-type property, mask-repeat, mask-position, mask-clip, mask-origin, mask-composite. See http://dev.w3.org/fxtf/css-masking-1/
Picking this up until I can find a proper assignee.
Assignee: ncameron → bugs
This issue should be blocking bug 913153.

Sebastian
Comment hidden (me-too)
Comment hidden (me-too)
Assignee

Updated

4 years ago
Assignee: bugs → cku
Assignee

Comment 20

4 years ago
Assignee

Comment 21

4 years ago
Attachment #8667692 - Attachment is obsolete: true
Assignee

Comment 22

4 years ago
Attachment #8668867 - Attachment is obsolete: true
Assignee

Comment 23

4 years ago
TBD
1. (CSS parsing) Integrate ParseMaskXXX & ParseBackgroundXXX
2. (CSS computing) Create nsStyleSVGStyle::Layers, instead of using nsStyleBackgroundLayer, since attributes of them is not the same.
3. (CSS rendering) Mask painting in nsSVGIntegrationUtils::PaintFramesWithEffects is only a simple prototype.
Assignee

Comment 24

4 years ago
TBD:
1. integration between css-background and css-mask.
   Apparently, we can use most background-image functions and data structure for mask.
2. mask rendering position.
Attachment #8669592 - Attachment is obsolete: true
Assignee

Comment 25

4 years ago
Attachment #8671293 - Attachment is obsolete: true
Assignee

Comment 26

4 years ago
Posted patch WIP 5 (obsolete) — Splinter Review
Attachment #8674131 - Attachment is obsolete: true
Assignee

Comment 27

4 years ago
TBD:
1. Most requirements are implemented, except mask-mode(luminance) and short-hand.
2. Change PaintBackground semantic to PaintCSSLayer.
Attachment #8674796 - Attachment is obsolete: true
Assignee

Comment 28

4 years ago
Rebase
Attachment #8677956 - Attachment is obsolete: true
Assignee

Comment 29

4 years ago
Attachment #8678121 - Attachment is obsolete: true
Assignee

Comment 30

4 years ago
Attachment #8681281 - Attachment is obsolete: true
Comment on attachment 8681973 [details] [diff] [review]
WIP 9 - merge mask & background properties into layer property

Why are you trying to use the same layer structure for both backgrounds and masks?  (I could understand wanting a common base class for the things they share, but it seems wasteful and probably also confusing to use the same struct.)  Or does it save space (because of packing) or code to have it all together?  If so, it would be good to have something to prevent it from being confusing about what's actually used.  I also wouldn't use nsCSSLayers as the name; we tend to use nsCSS* for specified style data and nsStyle* for computed style data.

Also I'm not inclined to think we want a separate style struct.  That would perhaps be desirable for if the mask properties were widely used, but I don't see them being as widely used as the background properties.  (It is sometimes nice for rule tree optimizations if a single shorthand sets all of the properties in a struct, although it's less important for non-inherited properties like these.)  I'd probably suggest just adding to nsStyleSVGReset for now.
It's also going to be a *lot* easier to review this if it is done as a stack of patches rather than a single patch.
Assignee

Comment 33

4 years ago
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #31)
> Comment on attachment 8681973 [details] [diff] [review]
> WIP 9 - merge mask & background properties into layer property
> 
> Why are you trying to use the same layer structure for both backgrounds and
> masks?  (I could understand wanting a common base class for the things they
> share, but it seems wasteful and probably also confusing to use the same
> struct.)  Or does it save space (because of packing) or code to have it all
> together?  If so, it would be good to have something to prevent it from
> being confusing about what's actually used.  I also wouldn't use nsCSSLayers
> as the name; we tend to use nsCSS* for specified style data and nsStyle* for
> computed style data.
Have a single base class does not really helpful in CSS parser side, but I can see benefit in CSS render side.
I use nsCSSRendering::PaintBackgroundWithSC, with minor code change, for both background and mask rendering because of this new base class. Without this base class, I need either to create another nsCSSRendering::PaintMaskWithSC function for mask rendering, or change nsCSSRendering::PaintBackgroundWithSC and some functions called by it(PrepareBackgroundLayer for exp) to template functions, so that it can process both background style data and mask style data.

nsCSSLayers was designed to be a member data of nsStyleBackground and nsStyleMask(or nsStyleSVGReset as your suggestion), so named it as nsStyleLayer is not that fit. nsLayer is to common and easy to confuse with gfx layer. Like you said, nsCSS* is not suitable as well.... I will pick up another name.
> Also I'm not inclined to think we want a separate style struct.  That would
> perhaps be desirable for if the mask properties were widely used, but I
> don't see them being as widely used as the background properties.  (It is
> sometimes nice for rule tree optimizations if a single shorthand sets all of
> the properties in a struct, although it's less important for non-inherited
> properties like these.)  I'd probably suggest just adding to nsStyleSVGReset
> for now.
OK
Assignee

Comment 34

4 years ago
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #32)
> It's also going to be a *lot* easier to review this if it is done as a stack
> of patches rather than a single patch.

OK, will do soon.
(In reply to C.J. Ku[:cjku] from comment #33)
> nsCSSLayers was designed to be a member data of nsStyleBackground and
> nsStyleMask(or nsStyleSVGReset as your suggestion), so named it as
> nsStyleLayer is not that fit. nsLayer is to common and easy to confuse with
> gfx layer. Like you said, nsCSS* is not suitable as well.... I will pick up
> another name.

I think nsStyleLayers is probably fine; it's similar to the existing nsStyleGradient or nsStyleImage.

But given how many properties are common, having a single class (without a hierarchy) seems fine, as long as there are comments explaining which members are/aren't used in which situations.
Assignee

Comment 36

4 years ago
Posted patch 1. Mask CSS property (obsolete) — Splinter Review
Attachment #8681973 - Attachment is obsolete: true
Assignee

Comment 37

4 years ago
Posted patch 2. Mask Style (obsolete) — Splinter Review
Assignee

Comment 38

4 years ago
Posted patch 3. Mask Rendering (obsolete) — Splinter Review
Assignee

Comment 39

4 years ago
Posted file Git hub working branch (obsolete) —
Assignee

Comment 40

4 years ago
Things not ready
1. mask-mode: luminance mask is not supported yet.
2. mask-size & mask-position animation are not supported yet.
3. svg-mask: mask properties, such as mask-repeat/origin/clip, are not apply to a mask-image if that image is a SVG#mask element.

I intend to do all these three items in the following up bugs, any thought?
(In reply to C.J. Ku[:cjku] from comment #40)
> I intend to do all these three items in the following up bugs, any thought?

It's nearly always fine to separate work like that into followup bugs when the plan is to ship the feature after the followup bugs land (that is, until they're done, have it behind a pref that's either disabled or enabled only for nightly and aurora).  If what you're asking is whether it's ok to ship the feature without those, then I'd have to actually think about it...
Assignee

Comment 42

4 years ago
Bug 686281 - CSS Mask prop.
Assignee

Comment 44

4 years ago
Bug 686281 - CSS Mask rendering.
Assignee

Updated

4 years ago
Attachment #8682995 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8682996 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8682999 - Attachment is obsolete: true
Assignee

Comment 45

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/1-2/
Assignee

Comment 46

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/1-2/
Assignee

Comment 47

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/1-2/
Did you intend to request review?
Flags: needinfo?(cku)
Assignee

Comment 49

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/2-3/
Assignee

Comment 50

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/2-3/
Assignee

Comment 51

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/2-3/
Assignee

Comment 52

4 years ago
Bug 686281 - CSS Mask animation.
Assignee

Updated

4 years ago
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - CSS Mask prop. → MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron
Attachment #8683556 - Flags: review?(dbaron)
Assignee

Comment 53

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/3-4/
Assignee

Updated

4 years ago
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - CSS Mask Style → MozReview Request: Bug 686281- Implement CSS mask style. r=:dbaron
Attachment #8683557 - Flags: review?(dbaron)
Assignee

Comment 54

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/3-4/
Assignee

Updated

4 years ago
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - CSS Mask rendering. → MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron
Attachment #8683558 - Flags: review?(dbaron)
Assignee

Comment 55

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/3-4/
Assignee

Updated

4 years ago
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - CSS Mask animation. → MozReview Request: Bug 686281- Implement CSS mask animation. r=:dbaron
Attachment #8683706 - Flags: review?(dbaron)
Assignee

Comment 56

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/1-2/
Assignee

Comment 57

4 years ago
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #48)
> Did you intend to request review?
I am still working on test case... but yes, if you have time, please help to review mask implementation.
Flags: needinfo?(cku)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

https://reviewboard.mozilla.org/r/24369/#review21991

::: layout/style/Declaration.cpp:396
(Diff revision 4)
> +    case eCSSProperty_mask: {

Is it possible for this (in Declaration.cpp) to share code with background?

::: layout/style/nsCSSParser.cpp:11307
(Diff revision 4)
> +CSSParserImpl::ParseMaskItem(CSSParserImpl::StyleLayersParseState& aState)

Is it possible for this to share code with background?

::: layout/style/nsCSSPropList.h:4014
(Diff revision 4)
> +    "",

Should the new properties be behind a pref for now, or do you think they'll be ready to ship once this lands?  Could you send either an intent to implement, or an intent to implement and ship, to dev-platform, per the policy at https://wiki.mozilla.org/WebAPI/ExposureGuidelines

::: layout/style/nsCSSPropList.h:4045
(Diff revision 4)
> +// TBD
> +// Copy from background-position, which animation type is eStyleAnimType_Custom
> +// Need to do something in StyleAnimationValue::ExtractComputedValue.

Do you need to deal with this TBD?

::: layout/style/nsCSSProps.cpp:865
(Diff revision 4)
> -const KTableValue nsCSSProps::kBackgroundAttachmentKTable[] = {
> -  eCSSKeyword_fixed, NS_STYLE_BG_ATTACHMENT_FIXED,
> -  eCSSKeyword_scroll, NS_STYLE_BG_ATTACHMENT_SCROLL,
> -  eCSSKeyword_local, NS_STYLE_BG_ATTACHMENT_LOCAL,
> +const KTableValue nsCSSProps::kLayerAttachmentKTable[] = {
> +  eCSSKeyword_fixed, NS_STYLE_LAYER_ATTACHMENT_FIXED,
> +  eCSSKeyword_scroll, NS_STYLE_LAYER_ATTACHMENT_SCROLL,
> +  eCSSKeyword_local, NS_STYLE_LAYER_ATTACHMENT_LOCAL,

I think you also need to include the nsStyleConsts.h changes that change the names of these constants in this initial patch; otherwise the patch won't compile on its own.

That said, I'm not sure about the name changes.  I think the current names for both the constants and the tables might be clearer:  I think people have a good idea what background-\* properties are, but less of an idea what yet another meaning for "layer" means here.  I'd be open to a better name, but I tend to think that the current names (NS_STYLE_BG\_\* and kBackground\*) are better than NS_STYLE_LAYER\_\* and kLayer\*.

::: layout/style/nsComputedDOMStylePropertyList.h:65
(Diff revision 4)
> +///// COMPUTED_STYLE_PROP(mask,                    Mask)
> +COMPUTED_STYLE_PROP(mask_clip,                     MaskClip)
> +COMPUTED_STYLE_PROP(mask_image,                    MaskImage)
> +COMPUTED_STYLE_PROP(mask_origin,                   MaskOrigin)
> +COMPUTED_STYLE_PROP(mask_position,                 MaskPosition)
> +COMPUTED_STYLE_PROP(mask_repeat,                   MaskRepeat)
> +COMPUTED_STYLE_PROP(mask_size,                     MaskSize)
> +COMPUTED_STYLE_PROP(mask_mode,                     MaskMode)
> +COMPUTED_STYLE_PROP(mask_composite,                MaskComposite)

I believe this will fail tests because it's out of order.  Have you run this patch through try yet?
https://reviewboard.mozilla.org/r/24369/#review21993

::: layout/style/nsCSSPropList.h:3990
(Diff revision 4)
> +CSS_PROP_SVGRESET(

Also, addition of new properties should have corresponding changes in layout/style/test/property_database.js -- and then the tests should pass with those changes.
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

https://reviewboard.mozilla.org/r/24371/#review21995

::: layout/base/nsCSSRendering.h:178
(Diff revision 4)
> -  static void ComputeObjectAnchorPoint(const nsStyleBackground::Position& aPos,
> +  static void ComputeObjectAnchorPoint(const nsStyleLayers::Position& aPos,

I don't think the separation between what's in the previous patch and what's in this patch makes any sense; things that are a single logical change that could be in its own patch (e.g., renaming a constant, pulling nsStyleBackground::Layer out into a class that's not within nsStyleBackground) are mixed between the previous patch and this one, thus preventing either from being compiled on their own or understood as a simple change.

This should be merged with the previous patch, but probably some of the initial refactoring steps should then be pulled out into separate patches (such as renaming the constants if you want to keep that renaming, and definitely the refactoring of nsStyleBackground::Layer into something that's reusable outside of nsStyleBackground).
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

https://reviewboard.mozilla.org/r/24373/#review21997

::: layout/base/nsCSSRendering.cpp:3081
(Diff revision 4)
> -                   NS_STYLE_BG_ATTACHMENT_LOCAL == aLayer.mAttachment)) {
> +                   NS_STYLE_LAYER_ATTACHMENT_LOCAL == aLayer.mAttachment)) {

This again has tons of changes that should have been in an earlier patch.

I think the important aspects of keeping parts of work separate to make things easy to review are that:
 (1) changes that are big and boring should be separate from changes that are small and interesting
 (2) unless it is hard to do without violating (1), each patch should compile on its own, since that helps regression-finding
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

https://reviewboard.mozilla.org/r/24415/#review21999

This does seem reasonable to have as a separate patch, although I *think* to make it work sensibly on its own, you'd need to have an nsCSSPropList.h change in this patch changing the properties from a temporary eStyleAnimType_None to the eStyleAnimType_Custom that you currently have in the first patch.

::: layout/style/StyleAnimationValue.h:255
(Diff revision 2)
> +    eUnit_MaskPosition, // nsCSSValueList* (never null)

Why does this need to be a separate unit from eUnit_BackgroundPosition?
Attachment #8683706 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron → MozReview Request: Bug 686281- Implement nsStyleLayers and modify nsStyleBackground. r=:dbaron.
Attachment #8683556 - Flags: review?(dbaron)
Assignee

Comment 64

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/4-5/
Assignee

Updated

4 years ago
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask style. r=:dbaron → MozReview Request: Bug 686281 - Rename *background* to *layer*. r=:dbaron
Attachment #8683557 - Flags: review?(dbaron)
Assignee

Comment 65

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/4-5/
Assignee

Updated

4 years ago
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron → MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron
Attachment #8683558 - Flags: review?(dbaron)
Assignee

Comment 66

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/4-5/
Assignee

Updated

4 years ago
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask animation. r=:dbaron → MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron
Attachment #8683706 - Flags: review?(dbaron)
Assignee

Comment 67

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/2-3/
Assignee

Comment 68

4 years ago
Bug 686281- Implement CSS mask animation. r=:dbaron
Attachment #8684292 - Flags: review?(dbaron)
Assignee

Comment 69

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/5-6/
Assignee

Comment 70

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/5-6/
Assignee

Comment 71

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/5-6/
Assignee

Comment 72

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/3-4/
Assignee

Comment 73

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/1-2/
Assignee

Comment 74

4 years ago
https://reviewboard.mozilla.org/r/24415/#review21999

> Why does this need to be a separate unit from eUnit_BackgroundPosition?

We don't need it. my bad.
What we need to to change eUnit_BackgroundPosition to eUnit_StyleLayerPosition, but since the name is still under discussion, keep it untouch
Assignee

Comment 75

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/6-7/
Assignee

Comment 76

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/6-7/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *layer*. r=:dbaron → MozReview Request: Bug 686281- Rename *background* to *layer*. r=:dbaron
Assignee

Comment 77

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/6-7/
Assignee

Comment 78

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/4-5/
Assignee

Comment 79

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/2-3/
Assignee

Comment 80

4 years ago
https://reviewboard.mozilla.org/r/24371/#review22087

::: layout/base/nsDisplayList.cpp:2372
(Diff revision 7)
> -  if (mBackgroundStyle->mLayers.mLayers.Length() != 1)
> +  if (mBackgroundStyle->Layers().Length() != 1)

Move back to the first patch
Assignee

Comment 81

4 years ago
https://reviewboard.mozilla.org/r/24373/#review22089

::: layout/style/nsStyleStruct.cpp:1292
(Diff revision 7)
> +  mLayers.mImageCount = 0;

Fix this TODO
Assignee

Comment 82

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/7-8/
Assignee

Comment 83

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/7-8/
Assignee

Comment 84

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/7-8/
Assignee

Comment 85

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/5-6/
Assignee

Comment 86

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/3-4/
Assignee

Comment 87

4 years ago
https://reviewboard.mozilla.org/r/24371/#review21995

> I don't think the separation between what's in the previous patch and what's in this patch makes any sense; things that are a single logical change that could be in its own patch (e.g., renaming a constant, pulling nsStyleBackground::Layer out into a class that's not within nsStyleBackground) are mixed between the previous patch and this one, thus preventing either from being compiled on their own or understood as a simple change.
> 
> This should be merged with the previous patch, but probably some of the initial refactoring steps should then be pulled out into separate patches (such as renaming the constants if you want to keep that renaming, and definitely the refactoring of nsStyleBackground::Layer into something that's reusable outside of nsStyleBackground).

1st patch: Pull out background layer property from nsStyleBackground and put them into nsStyleLayer. Include all necessary change because of this new class.
2st patch: rename work.
Assignee

Comment 88

4 years ago
https://reviewboard.mozilla.org/r/24371/#review22105

::: layout/style/nsCSSParser.cpp:10521
(Diff revision 8)
> -    return ParseBackgroundRepeat();
> +    return ParseLayerRepeat(eCSSProperty_background_repeat);

Move function parameter change back to the first patch
Assignee

Comment 89

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/8-9/
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281- Implement nsStyleLayers and modify nsStyleBackground. r=:dbaron. → MozReview Request: Bug 686281 - Implement nsStyleLayers and modify nsStyleBackground. r?:dbaron.
Assignee

Comment 90

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/8-9/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281- Rename *background* to *layer*. r=:dbaron → MozReview Request: Bug 686281 - Rename *background* to *layer*. r?:dbaron
Assignee

Comment 91

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/8-9/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask properties. r=:dbaron → MozReview Request: Bug 686281 - Implement CSS mask properties. r?:dbaron
Assignee

Comment 92

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/6-7/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask rendering. r=:dbaron → MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron
Assignee

Comment 93

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/4-5/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281- Implement CSS mask animation. r=:dbaron → MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron
Assignee

Comment 94

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/9-10/
Assignee

Comment 95

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/9-10/
Assignee

Comment 96

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/9-10/
Assignee

Comment 97

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/7-8/
Assignee

Comment 98

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/5-6/
Assignee

Comment 99

4 years ago
https://reviewboard.mozilla.org/r/24369/#review21991

> Is it possible for this (in Declaration.cpp) to share code with background?

Fixed in the third patch(Bug 686281 - Implement CSS mask properties)

> Is it possible for this to share code with background?

Fixed in the third patch(Bug 686281 - Implement CSS mask properties)

> I think you also need to include the nsStyleConsts.h changes that change the names of these constants in this initial patch; otherwise the patch won't compile on its own.
> 
> That said, I'm not sure about the name changes.  I think the current names for both the constants and the tables might be clearer:  I think people have a good idea what background-\* properties are, but less of an idea what yet another meaning for "layer" means here.  I'd be open to a better name, but I tend to think that the current names (NS_STYLE_BG\_\* and kBackground\*) are better than NS_STYLE_LAYER\_\* and kLayer\*.

How about use StyleLayer to replace Background, SL to replace BG?
And have some place explain what is style layer
Assignee

Comment 100

4 years ago
https://reviewboard.mozilla.org/r/24369/#review21991

> Should the new properties be behind a pref for now, or do you think they'll be ready to ship once this lands?  Could you send either an intent to implement, or an intent to implement and ship, to dev-platform, per the policy at https://wiki.mozilla.org/WebAPI/ExposureGuidelines

Land this feature with a pref is safer. Will do.
Assignee

Comment 101

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/10-11/
Assignee

Comment 102

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/10-11/
Assignee

Comment 103

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/10-11/
Assignee

Comment 104

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/8-9/
Assignee

Comment 105

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/6-7/
Assignee

Comment 106

4 years ago
Bug 686281 - Implement CSS mask alias. r?:dbaron
Attachment #8684679 - Flags: review?(dbaron)
Assignee

Comment 107

4 years ago
Bug 686281 - Implement CSS mask test. (WIP)
Assignee

Comment 108

4 years ago
https://reviewboard.mozilla.org/r/24369/#review21993

> Also, addition of new properties should have corresponding changes in layout/style/test/property_database.js -- and then the tests should pass with those changes.

Will do it in the last patch(Bug 686281 - Implement CSS mask test)
Assignee

Comment 109

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/11-12/
Assignee

Comment 110

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/11-12/
Assignee

Comment 111

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/11-12/
Assignee

Comment 112

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/9-10/
Assignee

Comment 113

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/7-8/
Assignee

Comment 114

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/1-2/
Assignee

Comment 115

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/1-2/
Assignee

Comment 116

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/12-13/
Assignee

Comment 117

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/12-13/
Assignee

Comment 118

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/12-13/
Assignee

Comment 119

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/10-11/
Assignee

Comment 120

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/8-9/
Assignee

Comment 121

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/2-3/
Assignee

Comment 122

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/2-3/
Assignee

Comment 123

4 years ago
Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthad to longhand. r?dbaron
Attachment #8684815 - Flags: review?(dbaron)
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

https://reviewboard.mozilla.org/r/24369/#review22177

I've only been partway through this patch so far, but here are my comments so far.

From what I've seen, I think this should have been at least 4 separate patches:
 (1) refactor part of nsStyleBackground into nsStyleLayers (or whatever we choose to call it
 (2) add mask properties to the layer parsing code in nsCSSParser
 (3) add mask properties to nsStyleLayers
 (4) the assertion about rule node bits
 
There might even be more than that, since I haven't been through the whole thing yet.

Given that I expect this will go through a few cycles of review, and I think it would make re-reviews a lot easier if things are more separated, I think it's at least worth splitting (2) and (4) into separate patches.  IT's probably ok for (1) and (3) to stay together since they're harder to separate.


I'd note that there are some comments about naming here; we might want to get to an agreement about what names to use before you go through the work of making a bunch more substitutions in the patch.

::: layout/base/nsCSSRendering.cpp:2900
(Diff revision 13)
> +  const nsStyleLayers &layers = aBackgroundSC->StyleBackground()->mLayers;

Current style rules prefer & and * before the space rather than after.  (Yes, there's plenty of old code around that doesn't match, but when you're introducing new code you should follow the rule.)

::: layout/base/nsCSSRendering.cpp:2962
(Diff revision 13)
> -    // Return if there are no background layers, all work from this point
> +    // Return if there are no style layers, all work from this point

The comment should stay as it is; it's specific to backgrounds.

::: layout/style/nsStyleStruct.h:98
(Diff revision 13)
> +static_assert(NS_RULE_NODE_IS_ANIMATION_RULE ==
> +  (1 << (nsStyleStructID_Inherited_Count + nsStyleStructID_Reset_Count)),
> +  "NS_RULE_NODE_IS_ANIMATION_RULE cannot fit the number of style struct.");
> +

I'm not sure why this is in this patch.  (I guess you were initially adding a new style struct.)

But if you put it in a separate patch, r=dbaron on that separate patch if you change (nsStyleStructID_Inherited_Count + nsStyleStructID_Reset_Count) to just be nsStyleStructID_Length.

::: layout/style/nsStyleStruct.h:390
(Diff revision 13)
> -struct nsStyleBackground {
> +struct nsStyleLayers {

As I said in comment 58, I'm not happy about this name, especially given the other things we have called layers.

I think nsStyleImageLayers (roughly heycam's suggestion) would be better than nsStyleLayers.  But we might want to agree on new names before you make big changes to the patches and then have to do them again.  (Although depending on what you're using for the patches, name search-and-replace might be very easy; it is with mq.)

::: layout/style/nsStyleStruct.h:507
(Diff revision 13)
> -    uint8_t mAttachment;                // [reset] See nsStyleConsts.h
> -    uint8_t mClip;                      // [reset] See nsStyleConsts.h
> -    uint8_t mOrigin;                    // [reset] See nsStyleConsts.h
> -    uint8_t mBlendMode;                 // [reset] See nsStyleConsts.h
> -    Repeat mRepeat;                     // [reset] See nsStyleConsts.h
> -    Position mPosition;                 // [reset]
> -    nsStyleImage mImage;                // [reset]
> -    Size mSize;                         // [reset]
> +    nsStyleImage  mImage;         // [reset]
> +    uint8_t       mClip;          // [reset] See nsStyleConsts.h
> +    uint8_t       mOrigin;        // [reset] See nsStyleConsts.h
> +    Repeat        mRepeat;        // [reset] See nsStyleConsts.h
> +    Position      mPosition;      // [reset] See nsStyleConsts.h
> +    Size          mSize;          // [reset]
> +    uint8_t       mAttachment;    // [reset] See nsStyleConsts.h
> +                                  // background layer property
> +    uint8_t       mBlendMode;     // [reset] See nsStyleConsts.h
> +                                  // background layer property
> +    uint8_t       mComposite;     // [reset] See nsStyleConsts.h
> +                                  // mask layer property
> +    uint8_t       mMaskMode;      // [reset] See nsStyleConsts.h
> +                                  // mask layer property

It would have been clearer to do the refactoring into its own class in one patch, and the addition of new properties for masks in a separate patch.

I guess I can deal with them being in the same patch, at least so far.

However, you should still pack all of the uint8_t values together, since the struct will pack better that way.  Repeat is a pair of uint8_t values, so it should be next to them.  Position and Size are both pairs of nsStyleCoord, so they have pointer size and alignment, and should be next to each other.  nsStyleImage also has pointer alignment.  So mImage, mPosition, and mSize should be next to each other either at the start or the end, and the other values should all be next to each other.  Otherwise you'll end up with holes in the struct.

::: layout/style/nsStyleStruct.h:513
(Diff revision 13)
> -    nsStyleImage mImage;                // [reset]
> -    Size mSize;                         // [reset]
> +    uint8_t       mAttachment;    // [reset] See nsStyleConsts.h
> +                                  // background layer property
> +    uint8_t       mBlendMode;     // [reset] See nsStyleConsts.h
> +                                  // background layer property
> +    uint8_t       mComposite;     // [reset] See nsStyleConsts.h
> +                                  // mask layer property
> +    uint8_t       mMaskMode;      // [reset] See nsStyleConsts.h
> +                                  // mask layer property

These comments that say "background layer property" and "mask layer property" should explain substantially more clearly what that means, and what rules the variables obey in the case when they're not used.

::: layout/style/nsStyleStruct.h:600
(Diff revision 13)
> +  #define NS_FOR_VISIBLE_STYLELAYER_BACK_TO_FRONT(var_, layers_) \
> +    for (uint32_t var_ = layers_.mImageCount; var_-- != 0; )
> +  #define NS_FOR_VISIBLE_STYLELAYER_BACK_TO_FRONT_WITH_RANGE(var_, layers_, start_, count_) \
> +    NS_ASSERTION((int32_t)(start_) >= 0 && (uint32_t)(start_) < (layers_.mImageCount), "Invalid layer start!"); \

These should probably be called NS_FOR_VISIBLE_STYLEIMAGELAYER..., if we want to use nsStyleImageLayers as the name.

I'm not really happy about that name, though, and would like to find something better.
Assignee

Comment 125

4 years ago
https://reviewboard.mozilla.org/r/24369/#review22177

> I'm not sure why this is in this patch.  (I guess you were initially adding a new style struct.)
> 
> But if you put it in a separate patch, r=dbaron on that separate patch if you change (nsStyleStructID_Inherited_Count + nsStyleStructID_Reset_Count) to just be nsStyleStructID_Length.

Will do. When I added a new style(nsStyleMask), to fix an assertion crash at [1], I spend almost two days to figue out need to left shift NS_RULE_NODE_IS_ANIMATION_RULE one bit. So, I think it's better to have a static assertion here.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.h#863
Assignee

Comment 126

4 years ago
https://reviewboard.mozilla.org/r/24369/#review22177

> The comment should stay as it is; it's specific to backgrounds.

In fact, I use this function nsCSSRendering::PaintBackgroundWithSC to generate mask image as well. I will rollback all this name change before we make the decision of the name of this new concept.
(In reply to C.J. Ku[:cjku] from comment #126)
> I will rollback all this name change before we
> make the decision of the name of this new concept.

I'd recommend not doing that; it should be easy to substitute one new name for another in patches, but it's a little harder to regenerate a list of all the correct places to substitute (at least if there's a risk of false matches).
https://reviewboard.mozilla.org/r/24369/#review22281

::: layout/base/nsCSSRendering.cpp:1749
(Diff revision 13)
> -nsCSSRendering::GetBackgroundClip(const nsStyleBackground::Layer& aLayer,
> +nsCSSRendering::GetBackgroundClip(const nsStyleLayers::Layer& aLayer,
> -                                  nsIFrame* aForFrame, const nsStyleBorder& aBorder,
> +                             nsIFrame* aForFrame, const nsStyleBorder& aBorder,
> -                                  const nsRect& aBorderArea, const nsRect& aCallerDirtyRect,
> +                             const nsRect& aBorderArea, const nsRect& aCallerDirtyRect,
> -                                  bool aWillPaintBorder, nscoord aAppUnitsPerPixel,
> +                             bool aWillPaintBorder, nscoord aAppUnitsPerPixel,
> -                                  /* out */ BackgroundClipState* aClipState)
> +                             /* out */ BackgroundClipState* aClipState)

This should not be changing indentation (although maybe a later patch should be).

::: layout/base/nsCSSRendering.cpp:4840
(Diff revision 13)
> -      //     Make sure to change nsStyleBackground::Size::DependsOnFrameSize
> +      //     Make sure to change nsStyleLayer::Size::DependsOnFrameSize

Missed the "s" here in nsStyleLayers

::: layout/style/nsCSSParser.cpp:824
(Diff revision 13)
> +    nsCSSValueList* mComposite;
> +    nsCSSValueList* mMode;

This should have comments pointing out properties that are only used for background or for mask.

A later (probably separate) patch should also rename BackgroundParseState, perhaps to ImageLayersShorthandParseState.

::: layout/style/nsRuleNode.cpp:6698
(Diff revision 13)
> -  for (uint32_t i = 0; i < bg->mImageCount; ++i)
> +  for (uint32_t i = 0; i < bg->mLayers.mImageCount; ++i)
>      bg->mLayers[i].TrackImages(aContext->PresContext());

This should use nsStyleLayers::TrackImages, rather than calling TrackImages on each layer directly.

::: layout/style/nsStyleConsts.h:311
(Diff revision 13)
> +// See nsStyleLayers
> +#define NS_STYLE_LAYER_MODE_ALPHA               0
> +#define NS_STYLE_LAYER_MODE_LUMINANCE           1
> +#define NS_STYLE_LAYER_MODE_AUTO                2

Probably best to rename LAYER_MODE -> MASK_MODE, since that's what they're used for.

::: layout/style/nsStyleConsts.h:1120
(Diff revision 13)
> +// composite
> +#define NS_STYLE_COMPOSITE_ADD                      0
> +#define NS_STYLE_COMPOSITE_SUBTRACT                 1
> +#define NS_STYLE_COMPOSITE_INTERSECT                2
> +#define NS_STYLE_COMPOSITE_EXCLUDE                  3

Probably COMPOSITE -> COMPOSITE_MODE

::: layout/style/nsStyleStruct.h:592
(Diff revision 13)
> +  const Layer &operator [](uint32_t aIndex) const {
> +    return mLayers[aIndex];
> +  }
> +
> +  Layer &operator [](uint32_t aIndex) {
> +    return mLayers[aIndex];
> +  }

Does anything use these?  If not, I think it's better not to add them, just because it's confusing to have too many ways to do the same thing.

::: layout/style/nsStyleStruct.h:600
(Diff revision 13)
> +  #define NS_FOR_VISIBLE_STYLELAYER_BACK_TO_FRONT(var_, layers_) \
> +    for (uint32_t var_ = layers_.mImageCount; var_-- != 0; )
> +  #define NS_FOR_VISIBLE_STYLELAYER_BACK_TO_FRONT_WITH_RANGE(var_, layers_, start_, count_) \
> +    NS_ASSERTION((int32_t)(start_) >= 0 && (uint32_t)(start_) < (layers_.mImageCount), "Invalid layer start!"); \

These should be renamed to NS_FOR_VISIBLE_IMAGE_LAYERS_... (with an S)

::: layout/style/nsStyleStruct.h:643
(Diff revision 13)
> +  const nsAutoTArray<nsStyleLayers::Layer, 1>& Layers() const {
> +    return mLayers.mLayers;
> +  }
> +
> +  nsAutoTArray<nsStyleLayers::Layer, 1>& Layers()  {
> +    return mLayers.mLayers;
> +  }

I find it confusing to have a function called Layers() that returns something different from mLayers (in particular, it returns mLayers.mLayers).  I think it's probably better to explicitly write mLayers.mLayers than to write Layers(), so I'd prefer to remove these, although I'm ok with it if it's hard to clean up.

::: layout/style/nsStyleStruct.cpp:2316
(Diff revision 13)
> -  NS_ASSERTION(onlyLayer, "auto array must have room for 1 element");
> -  onlyLayer->SetInitialValues();
> +  // We always have at least one background layer.
> +  mLayers.mImageCount = 1;

This is true for masks too; you should just leave the initialization of mImageCount to 1 in nsStyleImageLayer's constructor, and move the comment there.

::: layout/style/nsStyleStruct.cpp:2346
(Diff revision 13)
>    const nsStyleBackground* moreLayers =
> -    mImageCount > aOther.mImageCount ? this : &aOther;
> +    mLayers.mImageCount > aOther.mLayers.mImageCount ? this : &aOther;
>    const nsStyleBackground* lessLayers =
> -    mImageCount > aOther.mImageCount ? &aOther : this;
> +    mLayers.mImageCount > aOther.mLayers.mImageCount ? &aOther : this;

It would be better to make these const nsStyleImageLayers\* instead of const nsStyleBackground\*; then you won't need changes to the code below (or use of the operator[] above that I asked you to remove).

::: layout/style/nsStyleStruct.cpp:2410
(Diff revision 13)
> +// --------------------
> +// nsStyleLayers
> +//

It would be better to have this code before nsStyleBackground, to match the order in the .h file.

::: layout/style/nsStyleStruct.cpp:2420
(Diff revision 13)
> +  , mImageCount(0)

Why are you now initializing mImageCount to 0?  That seems wrong.  There's always at least one item in the computed value of the background-image or mask-image property.

::: layout/style/nsStyleStruct.cpp:2637
(Diff revision 13)
>  nsChangeHint
> -nsStyleBackground::Layer::CalcDifference(const Layer& aOther) const
> +nsStyleLayers::Layer::CalcDifference(const nsStyleLayers::Layer& aOther) const

You need to adjust CalcDifference to produce an appropriate change hint for mMaskMode and mComposite.  (It's ok if that's in another patch, but it better be somewhere.)

I have now finished going through this patch, so hopefully that's all the comments I have and I won't notice anything more when you post a revised version.
Oh, and for naming, I think we should do:
  nsStyleLayers -> nsStyleImageLayers
  NS_FOR_VISIBLE_STYLELAYER... -> NS_FOR_VISIBLE_IMAGE_LAYERS... (note LAYERS not LAYER)

I think it's probably best to just go with that for now; if somebody comes up with a better idea later, we can rename it.
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #129)
> Oh, and for naming, I think we should do:
>   nsStyleLayers -> nsStyleImageLayers
>   NS_FOR_VISIBLE_STYLELAYER... -> NS_FOR_VISIBLE_IMAGE_LAYERS... (note
> LAYERS not LAYER)
> 
> I think it's probably best to just go with that for now; if somebody comes
> up with a better idea later, we can rename it.

Though I just replied to the dev-platform thread, so maybe somebody will have a better idea.
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

https://reviewboard.mozilla.org/r/24373/#review22283

::: layout/generic/nsFrame.cpp:812
(Diff revision 13)
> +  const nsStyleSVGReset *oldSVGReset = aOldStyleContext ?
> +                                   aOldStyleContext->StyleSVGReset() :
> +                                   nullptr;
> +  const nsStyleSVGReset *newSVGReset = StyleSVGReset();

fix indentation of 2nd and 3rd lines, and also use "\* " rather than " \*"

::: layout/generic/nsFrame.cpp:834
(Diff revision 13)
> +    if (!oldBG || i >= oldBG->mLayers.mImageCount ||
> +        !newSVGReset->mLayers[i].mImage.ImageDataEquals(oldBG->mLayers[i].mImage)) {

These three references to oldBG should be oldSVGReset!  (Currently this code will produce pretty bizarre results.)

::: layout/style/Declaration.h:352
(Diff revision 13)
> +  template <typename T>
> +  void GetStyleLayerValue(nsCSSCompressedDataBlock *data,
> +                          nsAString& aValue,
> +                          nsCSSValue::Serialization aSerialization) const;

This would be better done with an array of nsCSSProperty (or, better, a struct of nsCSSProperty) that is static const data than with templates.  You don't actually need the code to be generated twice; you just need a list of 10 enum values.

::: layout/style/Declaration.cpp:223
(Diff revision 13)
> +    image->mValue.AppendToString(eCSSProperty_mask_image, aValue,
> +                                 aSerialization);
> +
> +    aValue.Append(char16_t(' '));
> +    repeat->mXValue.AppendToString(eCSSProperty_mask_repeat, aValue,
> +                                   aSerialization);
> +    if (repeat->mYValue.GetUnit() != eCSSUnit_Null) {
> +      repeat->mYValue.AppendToString(eCSSProperty_mask_repeat, aValue,
> +                                     aSerialization);

For these common properties (and background and size and clip and origin below), I think it's better to use the eCSSProperty_background\* constants, since those are better-known.

This will also make this code easier to review since the "moved" chunks will be more consistent.

::: layout/style/Declaration.cpp:308
(Diff revision 13)
> +    if (!image) {
> +      if (repeat || position || clip || origin || size) {
> +        // Uneven length lists, so can't be serialized as shorthand.
> +        aValue.Truncate();
> +        return;
> +      }
> +      break;
> +    }
> +
> +    if (!repeat || !position || !clip || !origin || !size) {
> +      // Uneven length lists, so can't be serialized as shorthand.
> +      aValue.Truncate();
> +      return;
> +    }

These conditions all need to test all of the values allowed for the relevant property.  So you need to not remove the attachment test for backgrounds, and you need to test composite and mode for mask.

::: layout/style/Declaration.cpp:595
(Diff revision 13)
> -    case eCSSProperty_background: {
> -      // We know from above that all subproperties were specified.
> +    case eCSSProperty_mask: {
> +      GetStyleLayerValue<nsStyleSVGReset>(data, aValue, aSerialization);

Please put mask after background rather than before, since it's newer and less well known.

::: layout/style/Declaration.cpp:596
(Diff revision 13)
> -      // We know from above that all subproperties were specified.
> +      GetStyleLayerValue<nsStyleSVGReset>(data, aValue, aSerialization);
> -      // However, we still can't represent that in the shorthand unless
> -      // they're all lists of the same length.  So if they're different
> -      // lengths, we need to bail out.
> -      // We also need to bail out if an item has background-clip and
> -      // background-origin that are different and not the default
> -      // values.  (We omit them if they're both default.)

Please move this comment rather than deleting it.

::: layout/style/nsCSSParser.cpp:813
(Diff revision 13)
> -  bool ParseBackground();
> +  template <typename T>
> +  bool ParseStyleLayer();

Again, this should be doable using an array or struct of data rather than templates, which will lead to substantially smaller code.

::: layout/style/nsCSSParser.cpp:10962
(Diff revision 13)
>    // Check first for inherit/initial/unset.
> +  if (StyleLayerPropTrait<T>::color != eCSSProperty_UNKNOWN) {

Why this test?  You should accept inherit/initial/unset for 'mask' just like for 'background'.

::: layout/style/nsCSSParser.cpp:10967
(Diff revision 13)
> -           nsCSSProps::SubpropertyEntryFor(eCSSProperty_background);
> +             nsCSSProps::SubpropertyEntryFor(StyleLayerPropTrait<T>::shortHand);

Write "shorthand" with a lowercase h instead of capital H.

(In the struct instead of the template, too.)

::: layout/style/nsCSSParser.cpp:10988
(Diff revision 13)
> +    if (StyleLayerPropTrait<T>::color != eCSSProperty_UNKNOWN) {
> -    // If we saw a color, this must be the last item.
> +      // If we saw a color, this must be the last item.
> -    if (color.GetUnit() != eCSSUnit_Null) {
> +      if (color.GetUnit() != eCSSUnit_Null) {

You should be able to assert this rather than test it, since ParseStyleLayerItem shouldn't produce a color when parsing masks.

::: layout/style/nsCSSParser.cpp:11000
(Diff revision 13)
> +#define APPENDNEXT(_propID, _prop, _propType) \

\_ at the end rather than the beginning of macro parameters, please.  (\_ at the beginning of identifiers is reserved for the implementation.)

Also probably rename prop\_ to propMember\_ or similar.

::: layout/style/nsCSSParser.cpp:11025
(Diff revision 13)
> +#define APPENDVALUE(_propID, _propValue) \

again, _ at the end

::: layout/style/nsCSSParser.cpp:11076
(Diff revision 13)
> +template <typename T>
>  bool
> -CSSParserImpl::ParseBackgroundItem(CSSParserImpl::StyleLayersParseState& aState)
> +CSSParserImpl::ParseStyleLayerItem(CSSParserImpl::StyleLayersParseState& aState)

again, no need for templatization

::: layout/style/nsCSSParser.cpp:11232
(Diff revision 13)
> +      } else if (StyleLayerPropTrait<T>::mode!= eCSSProperty_UNKNOWN &&

space before !=

::: layout/style/nsCSSParser.cpp:11384
(Diff revision 13)
> -// This function is very similar to ParseScrollSnapCoordinate,
> -// ParseBackgroundList, and ParseBackgroundSize.
> +// This function is very similar to ParseScrollSnapCoordinate
> +// and ParseStyleLayerSize.

I think it's still similar, right?  Shouldn't this just substitute the new name?

::: layout/style/nsCSSParser.cpp:11702
(Diff revision 13)
>  // This function is very similar to ParseScrollSnapCoordinate,
> -// ParseBackgroundList, and ParseBackgroundPosition.
> +// and ParseLayerPosition.

same here

::: layout/style/nsCSSProps.cpp:915
(Diff revision 13)
> +const KTableValue nsCSSProps::kLayerModeKTable[] = {
> +  eCSSKeyword_alpha, NS_STYLE_LAYER_MODE_ALPHA,
> +  eCSSKeyword_luminance, NS_STYLE_LAYER_MODE_LUMINANCE,
> +  eCSSKeyword_auto, NS_STYLE_LAYER_MODE_AUTO,
> +  eCSSKeyword_UNKNOWN,-1
> +};

It's not acceptable to have a value called 'auto' here, since that makes parsing the shorthand ambiguous.  This is a spec bug, which I reported at https://lists.w3.org/Archives/Public/public-fx/2015OctDec/0021.html

Could you:
 (1) file a bug on shipping CSS masks, with this bug blocking the new bug
 (2) file a bug on renaming 'auto', marked as blocking the bug from (1), and
 (3) add a FIXME comment here pointing to the bug in (2)

I'm only partly done with this review; I need to continue starting at nsCSSPropList.h.
Attachment #8683558 - Flags: review?(dbaron)
Assignee

Comment 132

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/13-14/
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Implement nsStyleLayers and modify nsStyleBackground. r?:dbaron. → MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron
Attachment #8683556 - Flags: review?(dbaron)
Assignee

Comment 133

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/13-14/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *layer*. r?:dbaron → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground. r?:dbaron.
Assignee

Comment 134

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/13-14/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask properties. r?:dbaron → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron
Attachment #8683558 - Flags: review?(dbaron)
Assignee

Comment 135

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/11-12/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron
Assignee

Comment 136

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/9-10/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron
Assignee

Comment 137

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/3-4/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask alias. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron
Assignee

Comment 138

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/3-4/
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask test. (WIP) → MozReview Request: Bug 686281 - Implement CSS mask alias. r?:dbaron
Assignee

Comment 139

4 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/1-2/
Attachment #8684815 - Attachment description: MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthad to longhand. r?dbaron → MozReview Request: Bug 686281 - Implement CSS mask test. (WIP)
Assignee

Comment 140

4 years ago
Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands. r?dbaron
Attachment #8685498 - Flags: review?(dbaron)
Assignee

Comment 141

4 years ago
Bug 686281 - A static assertion to keep the rightness of NS_RULE_NODE_IS_ANIMATION_RULE. r=:dbaron.
Assignee

Updated

4 years ago
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron → MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron.
Assignee

Comment 142

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/14-15/
Assignee

Comment 143

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/14-15/
Assignee

Comment 144

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/14-15/
Assignee

Comment 145

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/12-13/
Assignee

Comment 146

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/10-11/
Assignee

Comment 147

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/4-5/
Assignee

Comment 148

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/4-5/
Assignee

Comment 149

4 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/2-3/
Assignee

Comment 150

4 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/1-2/
Assignee

Comment 151

4 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/1-2/
Assignee

Comment 152

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/15-16/
Assignee

Comment 153

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/15-16/
Assignee

Comment 154

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/15-16/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron.
Assignee

Updated

4 years ago
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron.
Assignee

Comment 155

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/13-14/
Assignee

Comment 156

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/11-12/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron.
Assignee

Comment 157

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/5-6/
Assignee

Comment 158

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/5-6/
Assignee

Comment 159

4 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/3-4/
Assignee

Comment 160

4 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/2-3/
Assignee

Comment 161

4 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/2-3/
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

https://reviewboard.mozilla.org/r/24415/#review22425

This is continued from comment 131, though my comments on the first half of the patch are now associated with a different patch in reviewboard, since reviewboard was confused by the patch addition.

::: layout/style/nsCSSPropList.h:3984
(Diff revision 13)
> -CSS_PROP_SVGRESET(
> +CSS_PROP_SHORTHAND(
>      mask,
>      mask,
>      Mask,
> -    CSS_PROPERTY_PARSE_VALUE |
> -        CSS_PROPERTY_CREATES_STACKING_CONTEXT,
> +    CSS_PROPERTY_PARSE_FUNCTION,
> +    "layout.css.masking.enabled")

Making the shorthand conditional on the pref doesn't work, because that means you're disabling our existing support for 'mask'.

If you want a pref, it's going to be extra work to make the existing stuff still work, but not enable the new stuff.

The other option is to just try not to land until you're reasonably confident it will be ready to ship in the same release.

::: layout/style/nsCSSPropList.h:3996
(Diff revision 13)
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |

I think all of these properties should not apply to ::first-line, ::first-letter, and ::placeholder.  Relevant specs are:
https://drafts.csswg.org/css-pseudo/#first-line-styling
https://drafts.csswg.org/css-pseudo/#first-letter-styling
https://drafts.fxtf.org/css-masking-1/ (which doesn't mention first-line or first-letter)

::: layout/style/nsCSSPropList.h:3999
(Diff revision 13)
> +        CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED |

I don't think you want CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED for any mask properties.

::: layout/style/nsCSSPropList.h:4002
(Diff revision 13)
> +    VARIANT_IMAGE,

Please keep the "// used by list parsing" comment here.

::: layout/style/nsCSSPropList.h:4006
(Diff revision 13)
>  CSS_PROP_SVGRESET(

please list these properties in alphabetical order in nsCSSPropList.h

::: layout/style/nsCSSPropList.h:4053
(Diff revision 13)
> +        CSS_PROPERTY_UNITLESS_LENGTH_QUIRK |

The unitless length quirk should not apply to any new properties.

::: layout/style/nsComputedDOMStyle.h
(Diff revision 13)
> -  mozilla::dom::CSSValue* DoGetMask();

I think you need to keep the mask getter, since it used to be a longhand, but it should return null when the shorthand can't express the values.  See DoGetOverflow for an example.  (You need to check if any of the properties other than mask-image have non-initial values, or if mask-image has a non-1 length.)

::: layout/style/nsComputedDOMStyle.cpp:2148
(Diff revision 13)
> +nsComputedDOMStyle::DoGetMaskImage()
> +{
> +  const nsStyleSVGReset* svgReset = StyleSVGReset();
> +
> +  nsDOMCSSValueList *valueList = GetROCSSValueList(true);
> +
> +  for (uint32_t i = 0, i_end = svgReset->mLayers.mImageCount; i < i_end; ++i) {
> +    nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;
> +    valueList->AppendCSSValue(val);
> +
> +    const nsStyleImage& image = svgReset->mLayers.mLayers[i].mImage;
> +    SetValueToStyleImage(image, val);
> +  }
> +
> +  return valueList;

This should share code with DoGetBackgroundImage.

::: layout/style/nsComputedDOMStyle.cpp:2256
(Diff revision 13)
>  CSSValue*
> +nsComputedDOMStyle::DoGetMaskPosition()
> +{
> +  const nsStyleSVGReset* svgReset = StyleSVGReset();
> +
> +  nsDOMCSSValueList *valueList = GetROCSSValueList(true);
> +
> +  for (uint32_t i = 0, i_end = svgReset->mLayers.mPositionCount; i < i_end; ++i) {
> +    nsDOMCSSValueList *itemList = GetROCSSValueList(false);
> +    valueList->AppendCSSValue(itemList);
> +
> +    SetValueToPosition(svgReset->mLayers.mLayers[i].mPosition, itemList);
> +  }
> +
> +  return valueList;
> +}

This should share code with DoGetBackgroundPosition.

::: layout/style/nsComputedDOMStyle.cpp:2320
(Diff revision 13)
> +
> +CSSValue*
> +nsComputedDOMStyle::DoGetMaskRepeat()

This should share code with DoGetBackgroundRepeat.

::: layout/style/nsComputedDOMStyle.cpp:2451
(Diff revision 13)
>  CSSValue*
> +nsComputedDOMStyle::DoGetMaskSize()
> +{

This should share code with DoGetBackgroundSize.

::: layout/style/nsComputedDOMStylePropertyList.h:314
(Diff revision 13)
> -COMPUTED_STYLE_PROP(mask,                          Mask)
> +//// COMPUTED_STYLE_PROP(mask,                     Mask)

As I said above, this needs to stay (like overflow) because it was once a longhand.

::: layout/style/nsRuleNode.h:760
(Diff revision 13)
>    const void*
> +    ComputeMaskData(void* aStartStruct,
> +                        const nsRuleData* aRuleData,
> +                        nsStyleContext* aContext, nsRuleNode* aHighestNode,
> +                        RuleDetail aRuleDetail,
> +                        const mozilla::RuleNodeCacheConditions aConditions);
> +
> +  const void*

This is obsolete, you can remove it now that you no longer have a separate style struct.

::: layout/style/nsRuleNode.cpp:1917
(Diff revision 13)
> +static const uint32_t gMaskFlags[] = {
> +#define CSS_PROP_MASK FLAG_DATA_FOR_PROPERTY
> +#include "nsCSSPropList.h"
> +#undef CSS_PROP_MASK
> +};

This is also obsolete now that you no longer have a separate style struct.

::: layout/style/nsRuleNode.cpp:6437
(Diff revision 13)
> -SetLayerList(nsStyleContext* aStyleContext,
> +SetImageLayerList(nsStyleContext* aStyleContext,

This (and the series of changes that follow) should really move back to the patch that changed them from SetBackgroundList to SetLayerList.

::: layout/style/nsRuleNode.cpp:9434
(Diff revision 13)
> -  const nsCSSValue* maskValue = aRuleData->ValueForMask();
> -  if (eCSSUnit_URL == maskValue->GetUnit()) {
> -    svgReset->mMask = maskValue->GetURLValue();
> -  } else if (eCSSUnit_None == maskValue->GetUnit() ||
> -             eCSSUnit_Initial == maskValue->GetUnit() ||
> -             eCSSUnit_Unset == maskValue->GetUnit()) {
> +  const nsCSSValue* maskValue = aRuleData->ValueForMaskImage();
> +  if (eCSSUnit_List == maskValue->GetUnit() ||
> +      eCSSUnit_ListDep == maskValue->GetUnit()) {
> +    const nsCSSValue& item = maskValue->GetListValue()->mValue;
> +    if (eCSSUnit_URL == item.GetUnit() ||
> +        eCSSUnit_Image == item.GetUnit()) {
> +      svgReset->mMask = item.GetURLValue();
> +    } else if (eCSSUnit_None == item.GetUnit() ||
> +               eCSSUnit_Initial == item.GetUnit() ||
> +               eCSSUnit_Unset == item.GetUnit()) {
> -    svgReset->mMask = nullptr;
> +      svgReset->mMask = nullptr;
> -  } else if (eCSSUnit_Inherit == maskValue->GetUnit()) {
> +    } else if (eCSSUnit_Inherit == item.GetUnit()) {
> -    conditions.SetUncacheable();
> +      conditions.SetUncacheable();
> -    svgReset->mMask = parentSVGReset->mMask;
> +      svgReset->mMask = parentSVGReset->mMask;
> -  }
> +    }
> +  }

This should be explicitly marked as temporary, to be removed when we enable the new properties (as should mMask itself).

(I guess you have done part of the work to allow the preference.)

::: layout/style/nsRuleNode.cpp:9435
(Diff revision 13)
> -  if (eCSSUnit_URL == maskValue->GetUnit()) {
> -    svgReset->mMask = maskValue->GetURLValue();
> -  } else if (eCSSUnit_None == maskValue->GetUnit() ||
> -             eCSSUnit_Initial == maskValue->GetUnit() ||
> -             eCSSUnit_Unset == maskValue->GetUnit()) {
> +  if (eCSSUnit_List == maskValue->GetUnit() ||
> +      eCSSUnit_ListDep == maskValue->GetUnit()) {
> +    const nsCSSValue& item = maskValue->GetListValue()->mValue;
> +    if (eCSSUnit_URL == item.GetUnit() ||
> +        eCSSUnit_Image == item.GetUnit()) {
> +      svgReset->mMask = item.GetURLValue();
> +    } else if (eCSSUnit_None == item.GetUnit() ||
> +               eCSSUnit_Initial == item.GetUnit() ||
> +               eCSSUnit_Unset == item.GetUnit()) {
> -    svgReset->mMask = nullptr;
> +      svgReset->mMask = nullptr;

This doesn't get the right compatibility behavior since inherit, initial, and unset are alternatives to having a list rather than items in a list.  None, however, is an item in the list.

::: layout/style/nsRuleNode.cpp:9460
(Diff revision 13)
> +  uint32_t maxItemCount = 1;
> +  bool rebuild = false;

Move these 2 lines before the mask-image comment

::: layout/style/nsRuleNode.cpp:9466
(Diff revision 13)
> +                    initialImage, parentSVGReset->mLayers.mImageCount, svgReset->mLayers.mImageCount,

please wrap this long line if it's at or over 80 characters

::: layout/style/nsRuleNode.cpp:9483
(Diff revision 13)
> +                    uint8_t(NS_STYLE_IMAGELAYER_CLIP_BORDER), parentSVGReset->mLayers.mClipCount,

please wrap this long line if it's at or over 80 characters

::: layout/style/nsRuleNode.cpp:9490
(Diff revision 13)
> +                    uint8_t(NS_STYLE_IMAGELAYER_ORIGIN_PADDING), parentSVGReset->mLayers.mOriginCount,

please wrap this long line if it's at or over 80 characters

::: layout/style/nsRuleNode.cpp:9514
(Diff revision 13)
> +  // mask-mode: alpha | luminance | auto

This should say enum, inherit, initial [list], to match the comments for others.

::: layout/style/nsRuleNode.cpp:9521
(Diff revision 13)
> +  // mask-composition: add | subtract | intersect | exclude

The property's name is mask-composite, not mask-composition.  And again, this should say enum, inherit, initial [list], to match the comments for others.

::: layout/style/nsRuleNode.cpp:9525
(Diff revision 13)
> +                    uint8_t(NS_STYLE_COMPOSITE_MODE_ADD), parentSVGReset->mLayers.mCompositeCount,

Again, please wrap the line if it's more than 80 characters (I'm not sure if it is).

::: layout/style/nsStyleStruct.h:386
(Diff revision 13)
> +template <typename T> struct StyleLayerPropTrait;
> +
> +struct nsStyleBackground;
> +template <>
> +struct StyleLayerPropTrait<nsStyleBackground> {
> +  static const nsCSSProperty shortHand = eCSSProperty_background;
> +  static const nsCSSProperty color     = eCSSProperty_background_color;
> +  static const nsCSSProperty image     = eCSSProperty_background_image;
> +  static const nsCSSProperty repeat    = eCSSProperty_background_repeat;
> +  static const nsCSSProperty position  = eCSSProperty_background_position;
> +  static const nsCSSProperty clip      = eCSSProperty_background_clip;
> +  static const nsCSSProperty origin    = eCSSProperty_background_origin;
> +  static const nsCSSProperty size      = eCSSProperty_background_size;
> +  static const nsCSSProperty attach    = eCSSProperty_background_attachment;
> +  static const nsCSSProperty mode      = eCSSProperty_UNKNOWN;
> +  static const nsCSSProperty composite = eCSSProperty_UNKNOWN;
> +};
> +
> +struct nsStyleSVGReset;
> +template <>
> +struct StyleLayerPropTrait<nsStyleSVGReset> {
> +  static const nsCSSProperty shortHand = eCSSProperty_mask;
> +  static const nsCSSProperty color     = eCSSProperty_UNKNOWN;
> +  static const nsCSSProperty image     = eCSSProperty_mask_image;
> +  static const nsCSSProperty repeat    = eCSSProperty_mask_repeat;
> +  static const nsCSSProperty position  = eCSSProperty_mask_position;
> +  static const nsCSSProperty clip      = eCSSProperty_mask_clip;
> +  static const nsCSSProperty origin    = eCSSProperty_mask_origin;
> +  static const nsCSSProperty size      = eCSSProperty_mask_size;
> +  static const nsCSSProperty attach    = eCSSProperty_UNKNOWN;
> +  static const nsCSSProperty mode      = eCSSProperty_mask_mode;
> +  static const nsCSSProperty composite = eCSSProperty_mask_composite;
> +};
> +
>  struct nsStyleImageLayers {

As I said before, these should be structs rather than templates.

::: layout/style/nsStyleStruct.h:391
(Diff revision 13)
> +  static const nsCSSProperty shortHand = eCSSProperty_background;

shortHand should be shorthand (lowercase h rather than capital H)

::: layout/style/nsStyleStruct.h:546
(Diff revision 13)
> +    uint8_t       mComposite;     // [reset] See nsStyleConsts.h
> +                                  // mask layer property
> +    uint8_t       mMaskMode;      // [reset] See nsStyleConsts.h
> +                                  // mask layer property

Like for the background-only ones, these comments need to be clearer, and say what happens to these values in the background layer (hopefully they always have the initial value), and be clearer that "mask layer property" means it's a property that's only used for masks and not for backgrounds.

::: layout/style/nsStyleStruct.h:593
(Diff revision 13)
>             mSizeCount,
> -           mBlendModeCount;
> +           mMaskModeCount,
> +           mBlendModeCount,
> +           mCompositeCount;

Are the counts always 1 for the values that aren't currently used?  That seems the most sensible thing to do, but it should be documented.

::: layout/style/nsStyleStruct.h:3340
(Diff revision 13)
>    void Destroy(nsPresContext* aContext) {
> +    mLayers.UntrackImages(aContext);
> +
>      this->~nsStyleSVGReset();
>      aContext->PresShell()->
>        FreeByObjectID(mozilla::eArenaObjectID_nsStyleSVGReset, this);
>    }

Please move the Destroy method into the .cpp file, like other Destroy methods that have nontrivial contents (e.g., nsStyleBackground::Destroy).

::: layout/style/nsStyleStruct.h:3369
(Diff revision 13)
> +  const nsAutoTArray<nsStyleImageLayers::Layer, 1>& Layers() const {
> +    return mLayers.mLayers;
> +  }
> +
> +  nsAutoTArray<nsStyleImageLayers::Layer, 1>& Layers()  {
> +    return mLayers.mLayers;
> +  }

As I said for backgrounds, I'd like to see these methods go away.

::: layout/style/nsStyleStruct.cpp:1351
(Diff revision 13)
> +  const nsStyleImageLayers& moreLayers =
> +    mLayers.mImageCount > aOther.mLayers.mImageCount ?
> +      this->mLayers : aOther.mLayers;
> +  const nsStyleImageLayers& lessLayers =
> +    mLayers.mImageCount > aOther.mLayers.mImageCount ?
> +      aOther.mLayers : this->mLayers;
> +
> +  bool hasVisualDifference = false;
> +
> +  NS_FOR_VISIBLE_IMAGELAYER_BACK_TO_FRONT(i, moreLayers) {
> +    if (i < lessLayers.mImageCount) {
> +      if (moreLayers.mLayers[i] != lessLayers.mLayers[i]) {
> +        if ((moreLayers.mLayers[i].mImage.GetType() == eStyleImageType_Element) ||
> +            (lessLayers.mLayers[i].mImage.GetType() == eStyleImageType_Element))
> +          return NS_CombineHint(nsChangeHint_UpdateEffects,
> +                                nsChangeHint_RepaintFrame);
> +        hasVisualDifference = true;
> +      }
> +    } else {
> +      if (moreLayers.mLayers[i].mImage.GetType() == eStyleImageType_Element)
> +        return NS_CombineHint(nsChangeHint_UpdateEffects,
> +                              nsChangeHint_RepaintFrame);
> +      hasVisualDifference = true;
> +    }
> +  }
> +
> +  if (mLayers.mClipCount != aOther.mLayers.mClipCount ||
> +      mLayers.mOriginCount != aOther.mLayers.mOriginCount ||
> +      mLayers.mRepeatCount != aOther.mLayers.mRepeatCount ||
> +      mLayers.mPositionCount != aOther.mLayers.mPositionCount ||
> +      mLayers.mSizeCount != aOther.mLayers.mSizeCount||
> +      mLayers.mMaskModeCount != aOther.mLayers.mMaskModeCount||
> +      mLayers.mCompositeCount != aOther.mLayers.mCompositeCount) {
> +    NS_UpdateHint(hint, nsChangeHint_NeutralChange);
> +  }

Please make this share code with nsStyleBackground, probably with a CalcDifference method on nsStyleImageLayers.  Note that while you're there, you should fix the fact that there's an mBlendModeCount check missing.

If you ensure that the counts are always 1 for the properties that are unused, and that the values in the layer are always initial values when unused, then you don't need to worry about using a common method.  (But you should ensure those are true, which I think they are.)

::: layout/style/nsStyleStruct.cpp:1362
(Diff revision 13)
> +      if (moreLayers.mLayers[i] != lessLayers.mLayers[i]) {

This should also probably use the recently-added CalcDifference method on Layer.  But you'll see that when you merge it with the background code, assuming your tree is up-to-date enough.

::: layout/style/nsStyleStruct.cpp:1388
(Diff revision 13)
> +    NS_UpdateHint(hint, nsChangeHint_RepaintFrame);

Mask changes also need UpdateEffects and UpdateOverflow hints; see the current mMask handling.

I'm now done going through this patch (although, as I said, half my comments are, in reviewboard, associated with the previous patch, which I haven't looked at at all yet).
Attachment #8683706 - Flags: review?(dbaron)
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

https://reviewboard.mozilla.org/r/24805/#review22427

::: layout/style/nsStyleStruct.h:99
(Diff revision 3)
> +  "NS_RULE_NODE_IS_ANIMATION_RULE cannot fit the number of style struct.");

instead of "cannot fit the number of style struct", how about "must not overlap the style struct bits".
Attachment #8685499 - Flags: review+
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

https://reviewboard.mozilla.org/r/24803/#review22429

::: layout/style/nsRuleNode.cpp:5903
(Diff revision 3)
> +            for (const nsCSSProperty *shorthands =

"\* " rather than " \*".

::: layout/style/nsRuleNode.cpp:5913
(Diff revision 3)
> +                                     CSS_PROPERTY_CREATES_STACKING_CONTEXT)) {

please fix indentation (unless that pushes it past 80 characters)
Attachment #8685498 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #164)
> Comment on attachment 8685498 [details]
> MozReview Request: Bug 686281 - (Bug Fix:
> will-change-stacking-context-mask-1.html fail) Expands shorthand prop to
> longhands. r?dbaron
> 
> https://reviewboard.mozilla.org/r/24803/#review22429
> 
> ::: layout/style/nsRuleNode.cpp:5903
> (Diff revision 3)
> > +            for (const nsCSSProperty *shorthands =
> 
> "\* " rather than " \*".

This comment looks somehow unclear to me (especially that the stars are escaped, which I've filed bug 1223687 for).

To make it clearer, in Gecko code, we prefer declaring pointer types this way
> const nsCSSProperty* shorthands
> ~~~~~~~~~~~~~~~~~~~^
rather than
> const nsCSSProperty *shorthands
> ~~~~~~~~~~~~~~~~~~~~^

(We really should have some kind of style checker integrated in MozReview so that reviewers do not need to spend time on correcting things like this)
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

https://reviewboard.mozilla.org/r/24617/#review22431

::: layout/generic/nsFrame.cpp:861
(Diff revision 6)
> -      if ((!StyleMargin()->GetMargin(newValue) || oldValue != newValue) &&
> +    if ((!StyleMargin()->GetMargin(newValue) || oldValue != newValue) &&

Remove this change; the indentation was correct before, and this makes it incorrect.

::: layout/style/StyleAnimationValue.cpp:2982
(Diff revision 6)
> +ExtractStyleLayerPosition(const nsStyleImageLayers &layers,

The parameter should have an a prefix, e.g., aLayers.

Also, I'd name the function ExtractImageLayerPositionList.

::: layout/style/StyleAnimationValue.cpp:2999
(Diff revision 6)
> +ExtractStyleLayerSize(const nsStyleImageLayers &layers,

same 2 comments here

::: layout/style/StyleAnimationValue.cpp:3367
(Diff revision 6)
> -          const nsStyleBackground *bg =
> +          const nsStyleImageLayers &layers =

Again, please switch to & before space rather than space before &.

::: layout/style/StyleAnimationValue.cpp:3369
(Diff revision 6)
> -          nsAutoPtr<nsCSSValueList> result;
> +          MOZ_ASSERT(layers.mPositionCount > 0, "unexpected count");

This MOZ_ASSERT should move into the shared function.

::: layout/style/StyleAnimationValue.cpp:3384
(Diff revision 6)
> -                // leave it null
> +          MOZ_ASSERT(layers.mSizeCount > 0, "unexpected count");

This MOZ_ASSERT should also move into the shared function.
Attachment #8684679 - Flags: review?(dbaron) → review+
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

https://reviewboard.mozilla.org/r/24619/#review22433

::: layout/style/nsCSSPropAliasList.h:377
(Diff revision 6)
> +CSS_PROP_ALIAS(-webkit-mask-image,

you're missing -webkit-mask-composite; please add that
Attachment #8684680 - Flags: review+
Assignee

Comment 168

4 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/3-4/
Attachment #8685499 - Flags: review+ → review?(dbaron)
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

https://reviewboard.mozilla.org/r/24373/#review22423

One saved comment I made on the previous revision that I need to publish from MozReview:

::: layout/style/nsCSSProps.h:639
(Diff revision 15)
> -  static const KTableValue kBackgroundAttachmentKTable[];
> -  static const KTableValue kBackgroundOriginKTable[];
> -  static const KTableValue kBackgroundPositionKTable[];
> -  static const KTableValue kBackgroundRepeatKTable[];
> -  static const KTableValue kBackgroundRepeatPartKTable[];
> -  static const KTableValue kBackgroundSizeKTable[];
> +  static const KTableValue kLayerAttachmentKTable[];
> +  static const KTableValue kLayerOriginKTable[];
> +  static const KTableValue kLayerPositionKTable[];
> +  static const KTableValue kLayerRepeatKTable[];
> +  static const KTableValue kLayerRepeatPartKTable[];
> +  static const KTableValue kLayerSizeKTable[];

To fit with the other name changes, I think these should be kImageLayer...KTable[].
Attachment #8683558 - Flags: review?(dbaron)
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

https://reviewboard.mozilla.org/r/24805/#review22437

::: layout/style/nsStyleStruct.h:99
(Diff revision 3)
> +  "NS_RULE_NODE_IS_ANIMATION_RULE cannot fit the number of style struct.");

Saying this a second time since MozReview is broken:

instead of "cannot fit the number of style struct", how about "must not overlap the style struct bits".
Attachment #8685499 - Flags: review?(dbaron) → review+
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

https://reviewboard.mozilla.org/r/24373/#review22435

::: layout/base/nsCSSRendering.h:324
(Diff revision 16)
>     * A rectangle that one copy of the image tile is mapped onto. Same
>     * coordinate system as aBorderArea/aBGClipRect passed into
> -   * PrepareBackgroundLayer.
> +   * PrepareLayer.
>     */
>    nsRect mDestArea;
>    /**
>     * The actual rectangle that should be filled with (complete or partial)
>     * image tiles. Same coordinate system as aBorderArea/aBGClipRect passed into
> -   * PrepareBackgroundLayer.
> +   * PrepareLayer.
>     */
>    nsRect mFillArea;
>    /**
>     * The anchor point that should be snapped to a pixel corner. Same
>     * coordinate system as aBorderArea/aBGClipRect passed into
> -   * PrepareBackgroundLayer.
> +   * PrepareLayer.

These should say PrepareImageLayer to match the code.

::: layout/base/nsCSSRendering.h:548
(Diff revision 16)
> -  struct BackgroundClipState {
> +  struct ImageLayersClipState {

This should probably be ImageLayerClipState, actually, since it's only for a single layer.

::: layout/base/nsCSSRendering.cpp:1859
(Diff revision 16)
> -SetupBackgroundClip(nsCSSRendering::BackgroundClipState& aClipState,
> +SetupLayerClip(nsCSSRendering::ImageLayersClipState& aClipState,

This should be SetupImageLayerClip (as the indentation already suggests it is).  (Was this one of the changes you had in a later patch by mistake?)

::: layout/base/nsCSSRendering.cpp:2940
(Diff revision 16)
> -    GetBackgroundClip(layers.BottomLayer(),
> +    GetImageLayersClip(layers.BottomLayer(),
> -                      aForFrame, aBorder, aBorderArea,
> +                 aForFrame, aBorder, aBorderArea,
> -                      aDirtyRect, (aFlags & PAINTBG_WILL_PAINT_BORDER), appUnitsPerPixel,
> +                 aDirtyRect, (aFlags & PAINTBG_WILL_PAINT_BORDER),
> +                 appUnitsPerPixel,
> -                      &clipState);
> +                 &clipState);

This should also be GetImageLayerClip (singular), since it's for one layer.

And please fix the indentation.

::: layout/base/nsCSSRendering.cpp:3233
(Diff revision 16)
>     * These properties have the following dependencies upon each other when
>     * determining rendering:

You should document that background-blend-mode, mask-mode, and mask-composite have no dependencies.

::: layout/base/nsCSSRendering.cpp:3369
(Diff revision 16)
> -      PrepareBackgroundLayer(aPresContext, aForFrame, aFlags, borderArea,
> +      PrepareImageLayer(aPresContext, aForFrame, aFlags, borderArea,
>                               aClipRect, aLayer);

Please fix the indentation (unless I told you to change the name elsewhere).

::: layout/base/nsDisplayList.cpp:2159
(Diff revision 16)
> -    nsCSSRendering::PrepareBackgroundLayer(presContext, mFrame, flags,
> +    nsCSSRendering::PrepareImageLayer(presContext, mFrame, flags,
>                                             borderArea, borderArea, layer);

again, indentation, unless it matches another name fix

::: layout/base/nsDisplayList.cpp:2203
(Diff revision 16)
> -  nsCSSRendering::GetBackgroundClip(aLayer, aFrame, *aFrame->StyleBorder(),
> +  nsCSSRendering::GetImageLayersClip(aLayer, aFrame, *aFrame->StyleBorder(),
>                                      borderBox, borderBox, aWillPaintBorder,
>                                      aFrame->PresContext()->AppUnitsPerDevPixel(),
>                                      &clip);

Conveniently, this indentation is wrong, but dropping the "s" from the name as I suggested above will fix that.

::: layout/style/nsCSSProps.h:639
(Diff revision 16)
> -  static const KTableValue kBackgroundAttachmentKTable[];
> -  static const KTableValue kBackgroundOriginKTable[];
> -  static const KTableValue kBackgroundPositionKTable[];
> -  static const KTableValue kBackgroundRepeatKTable[];
> -  static const KTableValue kBackgroundRepeatPartKTable[];
> -  static const KTableValue kBackgroundSizeKTable[];
> +  static const KTableValue kLayerAttachmentKTable[];
> +  static const KTableValue kLayerOriginKTable[];
> +  static const KTableValue kLayerPositionKTable[];
> +  static const KTableValue kLayerRepeatKTable[];
> +  static const KTableValue kLayerRepeatPartKTable[];
> +  static const KTableValue kLayerSizeKTable[];

These should probably be kImageLayer...KTable; I think I said that already on a different patch, though!

::: layout/style/nsCSSProps.cpp:2181
(Diff revision 16)
> +
> +

Don't add these extra 2 lines.

::: layout/style/nsStyleConsts.h
(Diff revision 16)
> -// See nsStyleSVG
> -

Don't delete this (although fixing it to say nsStyleSVGReset would be good).
Attachment #8683558 - Flags: review+
Assignee

Updated

4 years ago
Attachment #8684680 - Flags: review+ → review?(dbaron)
Assignee

Comment 172

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/6-7/
Assignee

Updated

4 years ago
Attachment #8684815 - Flags: review?(dbaron)
Assignee

Comment 173

4 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/4-5/
https://reviewboard.mozilla.org/r/24647/#review22441

I'm not marking this "Fix it and then ship it" because I'd like to understand how you didn't get test failures from mask-composite being missing.

I'm not entirely happy about so much copy and paste of tests here, but I guess it's ok; this isn't the first time that's happened in this file.

::: layout/style/test/property_database.js:3917
(Diff revision 4)
> +    type: CSS_TYPE_TRUE_SHORTHAND,

This should be CSS_TYPE_SHORTHAND_AND_LONGHAND given that the property was a longhand previously.

::: layout/style/test/property_database.js:3918
(Diff revision 4)
> +    subproperties: ["mask-clip", "mask-image", "mask-mode", "mask-origin", "mask-position", "mask-repeat", "mask-size" ],

mask-composite is missing in this list.

You should also add a fixme that all the mask-border properties should be added when we implement them.  (You should probably also add the same FIXME to the subproperty table in nsCSSProps.cpp, in the appropriate patch.)

::: layout/style/test/property_database.js:4017
(Diff revision 4)
> +  },

mask-composite is missing.

That should have led to test failures.  Did it?  If not, I'd like to understand why.

::: layout/style/test/property_database.js:4023
(Diff revision 4)
> -    other_values: [ "url(#mymask)" ],
> +    other_values: [

You should include "url(#mymask)" in the other_values line for both mask and mask-image, rather than dropping it.
You also need some reftests for CSS masking.  If there are some in the CSS WG repository, you could import them by modifying andusing layout/reftests/w3c-css/import-tests.py.  If not, you should write some for a new directory inside layout/reftests/w3c-css/submitted/ .

MozReview automatically makes new review requests to me when you revise patches.  Since many of the issues in MozReview are still open, I'm going to assume they're not actually ready for re-review.  Please let me know explicitly when you think that the patches that I didn't already review+ are ready for re-review.  (I may or may not go through and actually cancel the bugzilla requests.)

I'm also probably going to redirect the review for the rendering piece to someone else, probably mstange.
Flags: needinfo?(cku)
Attachment #8684292 - Flags: review?(dbaron) → review?(mstange)
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

https://reviewboard.mozilla.org/r/24619/#review22443

::: layout/style/nsCSSPropAliasList.h:377
(Diff revision 6)
> +CSS_PROP_ALIAS(-webkit-mask-image,

Saying this a second time to make MozReview happy: you're missing -webkit-mask-composite; please add that
Attachment #8684680 - Flags: review?(dbaron) → review+
Assignee

Comment 177

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/16-17/
Attachment #8683556 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8683557 - Flags: review?(dbaron)
Assignee

Comment 178

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/16-17/
Assignee

Comment 179

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/16-17/
Attachment #8683558 - Flags: review+ → review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8683706 - Flags: review?(dbaron)
Assignee

Comment 180

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/14-15/
Assignee

Updated

4 years ago
Attachment #8684292 - Flags: review?(mstange) → review?(dbaron)
Assignee

Comment 181

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/12-13/
Assignee

Updated

4 years ago
Attachment #8684679 - Flags: review+ → review?(dbaron)
Assignee

Comment 182

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/6-7/
Assignee

Updated

4 years ago
Attachment #8684680 - Flags: review+ → review?(dbaron)
Assignee

Comment 183

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/6-7/
Assignee

Comment 184

4 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/4-5/
Assignee

Updated

4 years ago
Attachment #8685498 - Flags: review+ → review?(dbaron)
Assignee

Comment 185

4 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/3-4/
Assignee

Comment 186

4 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/3-4/
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - A static assertion to keep the rightness of NS_RULE_NODE_IS_ANIMATION_RULE. r=:dbaron. → MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE. r=:dbaron.
Attachment #8685499 - Flags: review+ → review?(dbaron)
Could you please not request re-review on the ones I've already granted review to, and also change the r?:dbaron to r?:mstange on the one that I forwarded to him?  (I hope that changing the r? to r= will do that, assuming you've addressed the review comments.)

(I can fix the mstange one for now, at least.)


Also, please explicitly let me know if things are actually ready for re-review.

(But also, it's not so good to work so much late at night!)
Attachment #8684292 - Flags: review?(dbaron) → review?(mstange)
Assignee

Comment 188

4 years ago
https://reviewboard.mozilla.org/r/24415/#review22425

> please list these properties in alphabetical order in nsCSSPropList.h

OK. May I ask is there any special reason that we need to list them in alphabetical order? Or, it's only for readness?
(In reply to C.J. Ku[:cjku] from comment #188)
> OK. May I ask is there any special reason that we need to list them in
> alphabetical order? Or, it's only for readness?

No special reason other than readability and maintainability.
Assignee

Comment 190

4 years ago
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #187)
> Could you please not request re-review on the ones I've already granted
> review to, and also change the r?:dbaron to r?:mstange on the one that I
> forwarded to him?  (I hope that changing the r? to r= will do that, assuming
> you've addressed the review comments.)
No, unless you have Commit level 3, otherwise, r+ flag will be clear in each push review. Could you vouch for me?(Bug 1224089)
Assignee

Updated

4 years ago
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers. r?:dbaron. → MozReview Request: Bug 686281 - Create nsStyleImageLayers; r?:dbaron
Attachment #8683556 - Flags: review?(dbaron)
Assignee

Comment 191

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/17-18/
Assignee

Updated

4 years ago
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground. r?:dbaron. → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; r?:dbaron
Attachment #8683557 - Flags: review?(dbaron)
Assignee

Comment 192

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/17-18/
Assignee

Updated

4 years ago
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*. r?:dbaron. → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r?:dbaron
Attachment #8683558 - Flags: review?(dbaron)
Assignee

Comment 193

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/17-18/
Assignee

Comment 194

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/15-16/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop. r?:dbaron. → MozReview Request: Bug 686281 - Implement CSS mask prop; r?:dbaron.
Attachment #8683706 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering. r?:dbaron. → MozReview Request: Bug 686281 - Implement CSS mask rendering; r?:mstange
Attachment #8684292 - Flags: review?(mstange) → review?(dbaron)
Assignee

Comment 195

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/13-14/
Assignee

Comment 196

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/7-8/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask animation. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask animation; r?:dbaron
Attachment #8684679 - Flags: review?(dbaron)
Assignee

Comment 197

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/7-8/
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask alias. r?:dbaron → MozReview Request: Bug 686281 - Implement CSS mask alias; r=:dbaron
Attachment #8684680 - Flags: review?(dbaron)
Assignee

Comment 198

4 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/5-6/
Assignee

Updated

4 years ago
Attachment #8685498 - Attachment description: MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands. r?dbaron → MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands; r:=dbaron
Attachment #8685498 - Flags: review?(dbaron)
Assignee

Comment 199

4 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/4-5/
Assignee

Comment 200

4 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/4-5/
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE. r=:dbaron. → MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=:dbaron.
Attachment #8685499 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Flags: needinfo?(cku)
Attachment #8683556 - Flags: review?(dbaron)
Assignee

Comment 201

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/18-19/
Assignee

Comment 202

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/18-19/
Attachment #8683557 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8683558 - Flags: review?(dbaron)
Assignee

Comment 203

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/18-19/
Assignee

Comment 204

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/16-17/
Attachment #8683706 - Flags: review?(dbaron)
Assignee

Comment 205

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/14-15/
Attachment #8684292 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8684679 - Flags: review?(dbaron)
Assignee

Comment 206

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/8-9/
Assignee

Comment 207

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/8-9/
Attachment #8684680 - Flags: review?(dbaron)
Assignee

Comment 208

4 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/5-6/
Attachment #8685498 - Flags: review?(dbaron)
Assignee

Comment 209

4 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/5-6/
Attachment #8685499 - Flags: review?(dbaron)
Also, per bug 1224141, please stop using the ":" in the reviewer syntax in the commit messages.
Assignee

Updated

4 years ago
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers; r?:dbaron → MozReview Request: Bug 686281 - Create nsStyleImageLayers;
Assignee

Comment 211

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/18-19/
Assignee

Comment 212

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/18-19/
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; r?:dbaron → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground;
Assignee

Comment 213

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/18-19/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r?:dbaron → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*;
Assignee

Comment 214

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/16-17/
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop; r?:dbaron. → MozReview Request: Bug 686281 - Implement CSS mask prop;
Assignee

Updated

4 years ago
Attachment #8684292 - Flags: review?(mstange)
Assignee

Comment 215

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/14-15/
Assignee

Comment 216

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/8-9/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask animation; r?:dbaron → MozReview Request: Bug 686281 - (r+) Implement CSS mask animation;
Assignee

Updated

4 years ago
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask alias; r=:dbaron → MozReview Request: Bug 686281 - (r+) Implement CSS mask alias;
Assignee

Comment 217

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/8-9/
Assignee

Comment 218

4 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/6-7/
Assignee

Comment 219

4 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/5-6/
Attachment #8685498 - Attachment description: MozReview Request: Bug 686281 - (Bug Fix: will-change-stacking-context-mask-1.html fail) Expands shorthand prop to longhands; r:=dbaron → MozReview Request: Bug 686281 - (r+) Expands shorthand prop to longhands;
Assignee

Comment 220

4 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/5-6/
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=:dbaron. → MozReview Request: Bug 686281 - (r+) A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE;
Assignee

Comment 221

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/15-16/
Attachment #8684292 - Flags: review?(mstange)
Assignee

Comment 222

4 years ago
https://reviewboard.mozilla.org/r/24415/#review22553

::: layout/style/Declaration.cpp:321
(Diff revision 17)
> +

This assertion is totally wrong. Remvoe it.
Assignee

Updated

4 years ago
Blocks: mask-image
Assignee

Comment 223

4 years ago
https://reviewboard.mozilla.org/r/24369/#review21991

> I believe this will fail tests because it's out of order.  Have you run this patch through try yet?

Will do, but can you enlighten me why the order here matter?
(In reply to C.J. Ku[:cjku] from comment #223)
> https://reviewboard.mozilla.org/r/24369/#review21991
> 
> > I believe this will fail tests because it's out of order.  Have you run this patch through try yet?
> 
> Will do, but can you enlighten me why the order here matter?

I believe the order of the properties in the big table in nsComputedDOMStyle.cpp is exposed to Web content as the enumeration order of JS "for (prop in element.style)", although maybe that's changed with new bindings.
Assignee

Comment 225

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/19-20/
Assignee

Comment 226

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/19-20/
Assignee

Updated

4 years ago
Attachment #8683558 - Flags: review?(dbaron)
Assignee

Comment 227

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/19-20/
Assignee

Comment 228

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/17-18/
Assignee

Comment 229

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/15-16/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering; r?:mstange → MozReview Request: Bug 686281 - Implement CSS mask rendering; r?mstange
Assignee

Comment 230

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/9-10/
Attachment #8684679 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8684680 - Flags: review?(dbaron)
Assignee

Comment 231

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/9-10/
Assignee

Comment 232

4 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/7-8/
Assignee

Updated

4 years ago
Attachment #8685498 - Flags: review?(dbaron)
Assignee

Comment 233

4 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/6-7/
Assignee

Updated

4 years ago
Attachment #8685499 - Flags: review?(dbaron)
Assignee

Comment 234

4 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/6-7/
Assignee

Comment 235

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/16-17/
Attachment #8684292 - Flags: review?(mstange)
Assignee

Comment 236

4 years ago
(In reply to C.J. Ku[:cjku] from comment #235)
> Comment on attachment 8684292 [details]
> MozReview Request: Bug 686281 - Implement CSS mask rendering; r?mstange
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/24531/diff/16-17/

Since interface of gfxContext had changed, need to rework after rebase from master. Remove review?
As I said in comment 175, please let me know explicitly when you actually want me to review these again.

(That said, I'm not sure why you're pushing new versions to mozreview when they're not yet ready for re-review.)
Flags: needinfo?(cku)
Assignee

Comment 238

4 years ago
https://reviewboard.mozilla.org/r/24647/#review22441

> mask-composite is missing.
> 
> That should have led to test failures.  Did it?  If not, I'd like to understand why.

Yes. Without listing mask-composite in gCSSProperties, I saw this kind of failures
79177 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | 'mask-composite' listed in gCSSProperties
80336 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | property 'mask' has a good subproperty list
Assignee

Updated

4 years ago
Blocks: 1228280
Assignee

Comment 239

4 years ago
https://reviewboard.mozilla.org/r/24415/#review22425

> This should be explicitly marked as temporary, to be removed when we enable the new properties (as should mMask itself).
> 
> (I guess you have done part of the work to allow the preference.)

I plan to keep nsStyleSVGReset::mMask here, move it to nsStyleImageLayers::Layer in "Bug 1228280 - (mask-image) Support multiple SVG mask." since it's not a trivial fix.

After phase in patches in this bug, what we have is:
1. If you write one svg-mask in "mask" property, FF can still parse and visually render it correctly as usual(backward compatible)
2. If you write one svg-mask in "mask-image", FF can parse and visually render it correcly(new feature).
3. If you write one or more image masks(none-SVG-mask) in "mask" or "mask-image" property, FF can prarse and visually render it correctly(new feature).
4. If you write more then one svg#mask in "mask" or "mask-image", FF can not render it correctly as usual(should be fix in bug 1228280)
Assignee

Updated

4 years ago
Blocks: 1228354
Assignee

Comment 240

4 years ago
Comment on attachment 8683556 [details]
MozReview Request: Bug 686281 - Implement nsStyleImageLayers; r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24369/diff/20-21/
Attachment #8683556 - Attachment description: MozReview Request: Bug 686281 - Create nsStyleImageLayers; → MozReview Request: Bug 686281 - Create nsStyleImageLayers; r?dbaron.
Attachment #8683556 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8683557 - Attachment description: MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; → MozReview Request: Bug 686281 - Integrate nsStyleImageLayers with nsStyleBackground; r?dbaron.
Attachment #8683557 - Flags: review?(dbaron)
Assignee

Comment 241

4 years ago
Comment on attachment 8683557 [details]
MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24371/diff/20-21/
Assignee

Comment 242

4 years ago
Comment on attachment 8683558 [details]
MozReview Request: Bug 686281 - Implement CSS mask style; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24373/diff/20-21/
Attachment #8683558 - Attachment description: MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; → MozReview Request: Bug 686281 - Rename *background* to *imagelayer*; r=dbaron.
Attachment #8683558 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8683706 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask prop; → MozReview Request: Bug 686281 - Implement CSS mask style; r?dbaron.
Attachment #8683706 - Flags: review?(dbaron)
Assignee

Comment 243

4 years ago
Comment on attachment 8683706 [details]
MozReview Request: Bug 686281 - Mask CSS parsing and Mask DOM API. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24415/diff/18-19/
Assignee

Comment 244

4 years ago
Comment on attachment 8684292 [details]
MozReview Request: Bug 686281 - Mask CSS rendering; r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24531/diff/16-17/
Attachment #8684292 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask rendering; r?mstange → MozReview Request: Bug 686281 - Mask CSS parsing; r?dbaron
Attachment #8684292 - Flags: review?(dbaron)
Assignee

Comment 245

4 years ago
Comment on attachment 8684679 [details]
MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24617/diff/10-11/
Attachment #8684679 - Attachment description: MozReview Request: Bug 686281 - (r+) Implement CSS mask animation; → MozReview Request: Bug 686281 - Mask style computing; r?dbaron
Attachment #8684679 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8684680 - Attachment description: MozReview Request: Bug 686281 - (r+) Implement CSS mask alias; → MozReview Request: Bug 686281 - Mask DOM API; r?dbaron
Attachment #8684680 - Flags: review?(dbaron)
Assignee

Comment 246

4 years ago
Comment on attachment 8684680 [details]
MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24619/diff/10-11/
Assignee

Comment 247

4 years ago
Comment on attachment 8684815 [details]
MozReview Request: Bug 686281 - Mask mochitest; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24647/diff/8-9/
Attachment #8684815 - Attachment description: MozReview Request: Bug 686281 - Implement CSS mask test. (WIP) → MozReview Request: Bug 686281 - Mask CSS rendering; r?mstange
Attachment #8684815 - Flags: review?(mstange)
Assignee

Comment 248

4 years ago
Comment on attachment 8685498 [details]
MozReview Request: Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24803/diff/7-8/
Attachment #8685498 - Attachment description: MozReview Request: Bug 686281 - (r+) Expands shorthand prop to longhands; → MozReview Request: Bug 686281 - Mask CSS animation; r=dbaron.
Attachment #8685498 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8685499 - Attachment description: MozReview Request: Bug 686281 - (r+) A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; → MozReview Request: Bug 686281 - Mask CSS webkit-alias; r=dbaron.
Attachment #8685499 - Flags: review?(dbaron)
Assignee

Comment 249

4 years ago
Comment on attachment 8685499 [details]
MozReview Request: Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24805/diff/7-8/
Assignee

Comment 250

4 years ago
Bug 686281 - Remove nsStyleSVGReset::mMask; r?dbaron
Attachment #8692847 - Flags: review?(dbaron)
Assignee

Comment 251

4 years ago
Bug 686281 - Add mask property into property_database.js; r?dbaron.
Attachment #8692848 - Flags: review?(dbaron)
Assignee

Comment 252

4 years ago
Bug 686281 - Expands will-change of a shorthand prop to longhand ones; r=dbaron.
Attachment #8692849 - Flags: review?(dbaron)
Assignee

Comment 253

4 years ago
Bug 686281 - A static assertion to keep value correctness of NS_RULE_NODE_IS_ANIMATION_RULE; r=dbaron.
Attachment #8692850 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Flags: needinfo?(cku)
Attachment #8683558 - Flags: review?(dbaron) → review+
Assignee

Comment 254

4 years ago
Hi dbaron,
Had fixed issues that you mentioned, please help to review again.
1. Remove nsStyleSVGReset::mMask in "Bug 686281 - Remove nsStyleSVGReset::mMask"
2. Move Mask DOM relative change into "Bug 686281 - Mask DOM API"
3. Fix mochitest fail(property_database.js mainly).