1303623 - Parsing mask-clip/mask-origin: fill-box|stroke-box|view-box|no-clip values

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[u459114]

According to masking spec:
https://drafts.fxtf.org/css-masking-1/#the-mask-clip

4 more properties support needed.

Comment 1

[u459114]

Hi Astley,
This bug is similiar to bug 1289011. Want to find someone fix it?
If no one want to fix it, please assign back to me.

Comment 2

[Astley Chen (inactive)]

rename to correct prop name.

Comment 3

[Ethan Lin[:ethlin]]

Attachment: css parser

This patch is for CSS parse. But I seems to mix background-clip and mask-clip.

Comment 4

[u459114]

We may just handle css parsing + style and make sure all test case pass in this bug. And create another one to fix rendering issues in nsCSSRendering + nsDisplayMask + nsSVGIntergrationUtils

Comment 5

[u459114]

And, not only mask-clip, mask-position need to support these new box as well

Comment 6

[Ethan Lin[:ethlin]]

Attachment: css parser

Fix the struct 'CSS_PROP_SVGRESET'. 'mask-clip' can read these values correctly, but 'background-clip' can read too. I need make these values illegal for 'background-clip'.

Comment 7

[Ethan Lin[:ethlin]]

Attachment: Part1. Style const changes.

Style constant changes.

Comment 8

[Ethan Lin[:ethlin]]

Attachment: Part2. Style structure changes

Style structure changes.

Comment 9

[Ethan Lin[:ethlin]]

Attachment: Part3. Parser changes

The patch is for parser changes.

Comment 10

[Ethan Lin[:ethlin]]

Attachment: Part4. Rendering changes

Bug 1311270 will implement the rendering part. So this patch is for fallback to default value.

Comment 11

[Ethan Lin[:ethlin]]

Attachment: Part5. Testcase changes

Fix testcase after adding fill-box|stroke-box|view-box|no-clip values.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8802419 [details] [diff] [review]
Part2. Style structure changes

Review of attachment 8802419 [details] [diff] [review]:
-----------------------------------------------------------------

OK, but a couple of things to fix.

::: layout/style/nsCSSProps.cpp
@@ +930,5 @@
> +  { eCSSKeyword_view_box, NS_STYLE_IMAGELAYER_CLIP_VIEW },
> +  { eCSSKeyword_no_clip, NS_STYLE_IMAGELAYER_CLIP_NO_CLIP },
> +  // The next entry is controlled by the layout.css.background-clip-text.enabled
> +  // pref.
> +  { eCSSKeyword_text, NS_STYLE_IMAGELAYER_CLIP_TEXT },

The "text" value should only be supported for background-clip, not for mask-clip.  The value is defined here:

https://compat.spec.whatwg.org/#valdef--webkit-background-clip-text

@@ +936,5 @@
> +};
> +
> +static_assert(MOZ_ARRAY_LENGTH(nsCSSProps::kMaskOriginKTable) ==
> +              MOZ_ARRAY_LENGTH(nsCSSProps::kMaskClipKTable) - 2,
> +              "background-clip has two extra value, which are no-clip and text, compared"

The original assertion was checking that {background,mask}-origin and background-clip had almost identical arrays.  If we want to preserve that, we should keep that assertion (just for background-origin vs background-clip) and have a second one for the mask arrays.

But I think it's probably not important to keep.  We have should have mochitest that check we support the right set of values.  So I suggest removing it.

::: layout/style/nsCSSProps.h
@@ +691,5 @@
>    static const KTableEntry kAzimuthKTable[];
>    static const KTableEntry kBackfaceVisibilityKTable[];
>    static const KTableEntry kTransformStyleKTable[];
>    static const KTableEntry kImageLayerAttachmentKTable[];
>    static const KTableEntry kImageLayerOriginKTable[];

Please rename this to kBackgroundOriginKTable, now that it is not being used for both background-origin and mask-origin.

@@ +702,5 @@
>    static const KTableEntry kImageLayerModeKTable[];
>    // Not const because we modify its entries when the pref
>    // "layout.css.background-clip.text" changes:
>    static KTableEntry kBackgroundClipKTable[];
> +  static KTableEntry kMaskClipKTable[];

Then we can make this const.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8802418 [details] [diff] [review]
Part1. Style const changes.

Review of attachment 8802418 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleConsts.h
@@ +351,5 @@
> +#define NS_STYLE_IMAGELAYER_CLIP_MARGIN              3
> +#define NS_STYLE_IMAGELAYER_CLIP_FILL                4
> +#define NS_STYLE_IMAGELAYER_CLIP_STROKE              5
> +#define NS_STYLE_IMAGELAYER_CLIP_VIEW                6
> +#define NS_STYLE_IMAGELAYER_CLIP_NO_CLIP             7

Can you comment somewhere in here that MARGIN, FILL, etc. are only supported by mask-clip, and not background-clip.

@@ +370,5 @@
>  #define NS_STYLE_IMAGELAYER_ORIGIN_CONTENT           2
> +#define NS_STYLE_IMAGELAYER_ORIGIN_MARGIN            3
> +#define NS_STYLE_IMAGELAYER_ORIGIN_FILL              4
> +#define NS_STYLE_IMAGELAYER_ORIGIN_STROKE            5
> +#define NS_STYLE_IMAGELAYER_ORIGIN_VIEW              6

Similarly here, that that MARGIN, FILL, etc. are only supported by mask-origin, and not background-origin.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8802422 [details] [diff] [review]
Part4. Rendering changes

Review of attachment 8802422 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +1835,5 @@
> +      backgroundClip == NS_STYLE_IMAGELAYER_CLIP_FILL ||
> +      backgroundClip == NS_STYLE_IMAGELAYER_CLIP_STROKE ||
> +      backgroundClip == NS_STYLE_IMAGELAYER_CLIP_VIEW ||
> +      backgroundClip == NS_STYLE_IMAGELAYER_CLIP_NO_CLIP) {
> +    backgroundClip = NS_STYLE_IMAGELAYER_CLIP_BORDER;

It would be nice to be able to assert that we don't get these values for background-clip (and only for mask-clip).  But inside GetImageLayerClip we don't know if we're here for background or mask...

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8802423 [details] [diff] [review]
Part5. Testcase changes

Review of attachment 8802423 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/property_database.js
@@ +2372,5 @@
>      inherited: false,
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "padding-box" ],
>      other_values: [ "border-box", "content-box", "border-box, padding-box", "padding-box, padding-box, padding-box", "border-box, border-box" ],
> +    invalid_values: [ "margin-box", "padding-box padding-box", "fill-box", "stroke-box", "view-box" ]

Put no-clip in here too?

Comment 16

[Cameron McCormack (:heycam)]

Since these properties are enabled by default on Nightly (and Aurora) currently, we shouldn't land these changes until we have the rendering changes ready to land at the same time.  Otherwise style sheets like the following will be broken, as it will seem like these keywords are supported but their rendering isn't:

  @supports (mask-clip: fill-box) {
    /* for when the browser supports mask-clip:fill-box */
    #abc { mask-image: url(...); mask-clip: fill-box; ... }
    #abc-fallback { display: none; }
  }

  @supports not (mask-clip: fill-box) {
    /* for when the browser doesn't support mask-clip:fill-box */
    #abc { display: none; }
    #abc-fallback { display: block; }
  }

Comment 17

[Ethan Lin[:ethlin]]

(In reply to Cameron McCormack (:heycam) (away Oct 25) from comment #15)
> Comment on attachment 8802423 [details] [diff] [review]
> Part5. Testcase changes
> 
> Review of attachment 8802423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/test/property_database.js
> @@ +2372,5 @@
> >      inherited: false,
> >      type: CSS_TYPE_LONGHAND,
> >      initial_values: [ "padding-box" ],
> >      other_values: [ "border-box", "content-box", "border-box, padding-box", "padding-box, padding-box, padding-box", "border-box, border-box" ],
> > +    invalid_values: [ "margin-box", "padding-box padding-box", "fill-box", "stroke-box", "view-box" ]
> 
> Put no-clip in here too?

Oh, 'no-clip' should be here. I'll add it.

Comment 18

[Ethan Lin[:ethlin]]

Attachment: Part1. Style const changes. (carry r+: heycam)

Address heycam's comments.

Comment 19

[Ethan Lin[:ethlin]]

Attachment: Part5. Testcase changes (carry r+: heycam)

Add 'no-clip' for invalid value of background-origin.

Comment 20

[Ethan Lin[:ethlin]]

Attachment: Part2. Style structure changes

Address comment 12.

Comment 21

[Cameron McCormack (:heycam)]

(In reply to Ethan Lin[:ethlin] from comment #20)
> Created attachment 8806223 [details] [diff] [review]
> Part2. Style structure changes
> 
> Address comment 12.

Don't we need to change how CSSParserImpl::ParseImageLayersItem works so that it uses the correct keyword table for the origin property, i.e. uses kBackgroundOriginKTable or kMaskOriginKTable depending on what property we're parsing?

Comment 22

[Ethan Lin[:ethlin]]

(In reply to Cameron McCormack (:heycam) from comment #21)

> Don't we need to change how CSSParserImpl::ParseImageLayersItem works so
> that it uses the correct keyword table for the origin property, i.e. uses
> kBackgroundOriginKTable or kMaskOriginKTable depending on what property
> we're parsing?

We should do this. I'll update the patch.

Comment 23

[Ethan Lin[:ethlin]]

Attachment: Part2. Style structure changes

Choose correct table (kBackgroundOriginKTable or kMaskOriginKTable) in ParseImageLayersItem.

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8807412 [details] [diff] [review]
Part2. Style structure changes

Review of attachment 8807412 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +12226,5 @@
>  
> +  MOZ_ASSERT(aTable == nsStyleImageLayers::kBackgroundLayerTable ||
> +             aTable == nsStyleImageLayers::kMaskLayerTable);
> +  const KTableEntry* originTable = (aTable == nsStyleImageLayers::kBackgroundLayerTable) ?
> +    nsCSSProps::kBackgroundOriginKTable : nsCSSProps::kMaskOriginKTable;

I just thought of a slightly better way to get the origin table.  You can use nsCSSProps::kKeywordTableTable to look it up based on whether we have background-origin or mask-origin in aTable.  So:

  const KTableEntry* originTable = nsCSSProps::kKeywordTableTable[aTable[nsStyleImageLayers::origin]];

Then we don't need the assertion on the aTable value itself.

So, r=me with that (here, and below for looping and the clip keyword table, and in Declaration.cpp too).

Comment 25

[Ethan Lin[:ethlin]]

(In reply to Cameron McCormack (:heycam) from comment #24)
> Comment on attachment 8807412 [details] [diff] [review]
> Part2. Style structure changes
> 
> Review of attachment 8807412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsCSSParser.cpp
> @@ +12226,5 @@
> >  
> > +  MOZ_ASSERT(aTable == nsStyleImageLayers::kBackgroundLayerTable ||
> > +             aTable == nsStyleImageLayers::kMaskLayerTable);
> > +  const KTableEntry* originTable = (aTable == nsStyleImageLayers::kBackgroundLayerTable) ?
> > +    nsCSSProps::kBackgroundOriginKTable : nsCSSProps::kMaskOriginKTable;
> 
> I just thought of a slightly better way to get the origin table.  You can
> use nsCSSProps::kKeywordTableTable to look it up based on whether we have
> background-origin or mask-origin in aTable.  So:
> 
>   const KTableEntry* originTable =
> nsCSSProps::kKeywordTableTable[aTable[nsStyleImageLayers::origin]];
> 
> Then we don't need the assertion on the aTable value itself.
> 
> So, r=me with that (here, and below for looping and the clip keyword table,
> and in Declaration.cpp too).

Okay, thanks for the review.

Comment 26

[Ethan Lin[:ethlin]]

Attachment: Part2. Style structure changes (carry r: heycam)

Address :heycam's comment.

Comment 27

[u459114]

Ehtan,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99a88b0c346c6e38eab49dbd190c01d4b4119fb3&selectedJob=31657719
TEST-UNEXPECTED-FAIL | layout/style/test/test_property_syntax_errors.html | invalid value 'margin-box' not accepted for 'mask-origin' property - got "margin-box", expected ""

Not only mask-clip, you probably should also update mask-origin valid/invalid value in property database

Comment 28

[Ethan Lin[:ethlin]]

Attachment: Bug 1303623 - Part1. Style const changes.

Review commit: https://reviewboard.mozilla.org/r/95326/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95326/

Comment 29

[Ethan Lin[:ethlin]]

Attachment: Bug 1303623 - Part2. Style structure changes.

Review commit: https://reviewboard.mozilla.org/r/95328/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95328/

Comment 30

[Ethan Lin[:ethlin]]

Attachment: Bug 1303623 - Part3. Parser changes.

Review commit: https://reviewboard.mozilla.org/r/95330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95330/

Comment 31

[Ethan Lin[:ethlin]]

Attachment: Bug 1303623 - Part4. Rendering changes.

Review commit: https://reviewboard.mozilla.org/r/95332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95332/

Comment 32

[Ethan Lin[:ethlin]]

Attachment: Bug 1303623 - Part5. Testcase changes.

Review commit: https://reviewboard.mozilla.org/r/95334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95334/

Comment 33

[Ethan Lin[:ethlin]]

Using mozreview is easier for bug 1311270 to merge and push the patches. I also rebased the patches. :heycam, sorry for sending review request again.

Comment 34

[u459114]

Ethan, bug 1260094 commend 6. We probably should remove implementation of margin-box first.

Comment 35

[Ethan Lin[:ethlin]]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #34)
> Ethan, bug 1260094 commend 6. We probably should remove implementation of
> margin-box first.

Okay, I'll update the patches.

Comment 36

[Ethan Lin[:ethlin]]

Comment on attachment 8814023 [details]
Bug 1303623 - Part1. Style const changes.

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

Comment 37

[Ethan Lin[:ethlin]]

Comment on attachment 8814024 [details]
Bug 1303623 - Part2. Style structure changes.

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

Comment 38

[Ethan Lin[:ethlin]]

Comment on attachment 8814025 [details]
Bug 1303623 - Part3. Parser changes.

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

Comment 39

[Ethan Lin[:ethlin]]

Comment on attachment 8814026 [details]
Bug 1303623 - Part4. Rendering changes.

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

Comment 40

[Ethan Lin[:ethlin]]

Comment on attachment 8814027 [details]
Bug 1303623 - Part5. Testcase changes.

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

Comment 41

[Cameron McCormack (:heycam)]

Comment on attachment 8814024 [details]
Bug 1303623 - Part2. Style structure changes.

https://reviewboard.mozilla.org/r/95328/#review95618

::: layout/style/Declaration.cpp:404
(Diff revision 2)
> -      for (size_t i = 0; nsCSSProps::kImageLayerOriginKTable[i].mValue != -1; i++) {
> +      const nsCSSProps::KTableEntry* originTable = nsCSSProps::kKeywordTableTable[aTable[nsStyleImageLayers::origin]];
> +      const nsCSSProps::KTableEntry* clipTable = nsCSSProps::kKeywordTableTable[aTable[nsStyleImageLayers::clip]];

Please keep these within 80 columns.

::: layout/style/nsCSSParser.cpp:12365
(Diff revision 2)
>         haveOrigin = false,
>         haveComposite = false,
>         haveMode = false,
>         haveSomething = false;
>  
> +  const KTableEntry* originTable = nsCSSProps::kKeywordTableTable[aTable[nsStyleImageLayers::origin]];

Here too.

::: layout/style/nsCSSParser.cpp:12450
(Diff revision 2)
>  
>          // The spec allows a second box value (for background-clip),
>          // immediately following the first one (for background-origin).
>  
>  #ifdef DEBUG
> -        for (size_t i = 0; nsCSSProps::kImageLayerOriginKTable[i].mValue != -1; i++) {
> +        const KTableEntry* clipTable = nsCSSProps::kKeywordTableTable[aTable[nsStyleImageLayers::clip]];

And here.

Comment 42

[Cameron McCormack (:heycam)]

Comment on attachment 8814023 [details]
Bug 1303623 - Part1. Style const changes.

https://reviewboard.mozilla.org/r/95326/#review95622

Comment 43

[Cameron McCormack (:heycam)]

Comment on attachment 8814025 [details]
Bug 1303623 - Part3. Parser changes.

https://reviewboard.mozilla.org/r/95330/#review95626

Comment 44

[Cameron McCormack (:heycam)]

Comment on attachment 8814026 [details]
Bug 1303623 - Part4. Rendering changes.

https://reviewboard.mozilla.org/r/95332/#review95628

Comment 45

[Cameron McCormack (:heycam)]

Comment on attachment 8814027 [details]
Bug 1303623 - Part5. Testcase changes.

https://reviewboard.mozilla.org/r/95334/#review95630

Comment 46

[Cameron McCormack (:heycam)]

Looks good.  Again, let's not land this until bug 1311270 is ready to land.

Comment 47

[Ethan Lin[:ethlin]]

Comment on attachment 8814024 [details]
Bug 1303623 - Part2. Style structure changes.

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

Comment 48

[Ethan Lin[:ethlin]]

Comment on attachment 8814025 [details]
Bug 1303623 - Part3. Parser changes.

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

Comment 49

[Ethan Lin[:ethlin]]

Comment on attachment 8814026 [details]
Bug 1303623 - Part4. Rendering changes.

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

Comment 50

[Ethan Lin[:ethlin]]

Comment on attachment 8814027 [details]
Bug 1303623 - Part5. Testcase changes.

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

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d992a04bfc
Part1. Style const changes. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5a2498583f1
Part2. Style structure changes. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/72f4b8000ec2
Part3. Parser changes. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5dee39ac4db
Part4. Rendering changes. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f837ec9ae16
Part5. Testcase changes. r=heycam

Comment 52

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a5d992a04bfc
https://hg.mozilla.org/mozilla-central/rev/e5a2498583f1
https://hg.mozilla.org/mozilla-central/rev/72f4b8000ec2
https://hg.mozilla.org/mozilla-central/rev/c5dee39ac4db
https://hg.mozilla.org/mozilla-central/rev/1f837ec9ae16

Comment 53

[Chris Mills [:cmills]]

These are already documented at 

https://developer.mozilla.org/en-US/docs/Web/CSS/mask-clip
https://developer.mozilla.org/en-US/docs/Web/CSS/mask-origin

And the Fx53 release notes already mention support for the mask-* properties.

So I don't think anything else needs to be done for the docs.