1308963 - Content with -webkit-mask-size but not -webkit-mask-repeat is clipped in Firefox, not Safari

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Florens Verschelde :fvsch]

For instance on https://viljamis.com/ the header and post title use a 400x400 PNG as a mask:

-webkit-mask-image: url(https://viljamis.com/img/mask-eea06722.png);
-webkit-mask-size: 13%;

With those properties, Safari aligns the mask to the top left (instead of center), and repeats the mask. It is as if its initial values were:

-webkit-mask-position: 0 0;
-webkit-mask-repeat: repeat;

As a result, the whole illustration or title or displayed correctly. (The PNG mask merely adds noise.)

On the other hand, Firefox applies mask-position:center; and mask-repeat:no-repeat; which means only a small section of the illustration and post title are visible, and the rest is clipped.

This might be an issue with other web content out there that relies on non-standard WebKit behavior.

Related:
- bug 1224422 (META mask-image bug)
- bug 1258623 (initial value for mask-repeat property should be no-repeat, but is implemented as repeat)

Comment 1

[Daniel Holbert [:dholbert]]

mozregression says the site in question ( https://viljamis.com/ ) went from nice-looking to weird-looking when bug 1294660 landed. ("Enable mask-* longhand properties support on nightly & aurora channels")   Not too surprising, given the analysis in comment 0.

Hopefully C.J. can do some further analysis & see what we should do here.

Comment 2

[Karl Dubost💡 :karlcow]

Adding to webcompatibility issues related to the default value.

I wonder if/when `-webkit-mask` is the only value available aka without standards fallback we should adopt the rest of the default values of `-webkit-mask-*`.

Testing with 
data:text/html,<p%20style="-webkit-mask-image:%20url('https://upload.wikimedia.org/wikipedia/commons/thumb/7/76/Mozilla_Firefox_logo_2013.svg/2000px-Mozilla_Firefox_logo_2013.svg.png')">foobar</p>

In Safari WebKit:
Release 14 (Safari 10.1, WebKit 12603.1.6.0.2)


-webkit-mask-box-image-outset: 0px;
-webkit-mask-box-image-repeat: stretch;
-webkit-mask-box-image-slice: 0 fill;
-webkit-mask-box-image-source: none;
-webkit-mask-box-image-width: auto;
-webkit-mask-box-image: none;
-webkit-mask-clip: border-box;
-webkit-mask-composite: source-over;
-webkit-mask-image: url(https://upload.wikimedia.org/wikipedia/commons/thumb/7/76/Mozilla_Firefox_logo_2013.svg/2000px-Mozilla_Firefox_logo_2013.svg.png);
-webkit-mask-origin: border-box;
-webkit-mask-position: 0% 0%;
-webkit-mask-repeat: repeat;
-webkit-mask-size: auto;
-webkit-mask-source-type: alpha;
mask-type: luminance;
mask: none;

In Opera Blink:
Chrome/54.0.2840.27 OPR/41.0.2353.10 (Edition beta)


-webkit-mask-box-image-outset: 0px;
-webkit-mask-box-image-repeat: stretch;
-webkit-mask-box-image-slice: 0 fill;
-webkit-mask-box-image-source: none;
-webkit-mask-box-image-width: auto;
-webkit-mask-clip: border-box;
-webkit-mask-composite: source-over;
-webkit-mask-image: url(https://upload.wikimedia.org/wikipedia/commons/thumb/7/76/Mozilla_Firefox_logo_2013.svg/2000px-Mozilla_Firefox_logo_2013.svg.png);
-webkit-mask-origin: border-box;
-webkit-mask-position-x: 0%;
-webkit-mask-position-y: 0%;
-webkit-mask-repeat-x: ;
-webkit-mask-repeat-y: ;
-webkit-mask-size: auto;
mask-type: luminance;
mask: none;

In Firefox:
52.0a1 (2016-10-08) (64-bit)

mask-clip: border-box;
mask-composite: add;
mask-image: url("https://upload.wikimedia.org/wikipedia/commons/thumb/7/76/Mozilla_Firefox_logo_2013.svg/2000px-Mozilla_Firefox_logo_2013.svg.png");
mask-mode: match-source;
mask-origin: border-box;
mask-position-x: 50%;
mask-position-y: 50%;
mask-position: 50% 50%;
mask-repeat: no-repeat;
mask-size: auto auto;
mask-type: luminance;
mask: url("https://upload.wikimedia.org/wikipedia/commons/thumb/7/76/Mozilla_Firefox_logo_2013.svg/2000px-Mozilla_Firefox_logo_2013.svg.png");

Comment 3

[u459114]

Hmm...
* mask-size: auto auto; 
  this looks not quite right, I will look into this.
*-webkit-mask-composite: source-over;
 -webkit-mask-composite's prop values is not the same with the definition in masking spec.
* mask: url("https://upload.wikimedia.org/wikipedia/commons/thumb/7/76/Mozilla_Firefox_logo_2013.svg/2000px-Mozilla_Firefox_logo_2013.svg.png"): this is for FF backward compatible. "mask" is treated as a longhand before bug 686281 landed.

For mask-repeat and mask-position, google had reverted the change of mask-repeat 
https://bugs.chromium.org/p/chromium/issues/detail?id=628968#c11
So, blink will keep using repeat as initial value. And mask-position is always left top(like background-position). webkit has no intension to follow the masking spec so far.

At current stage, I think we should align with mask-repeat/position's initial value on webkit/blink, so that web developer can see the same rendering result among different browsers.(And I will follow up this)

Comment 4

[u459114]

Attachment: Bug 1308963 - Part 1. Set initial value of mask-repeat as repeat.

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

Comment 5

[u459114]

Attachment: Bug 1308963 - Part 2. Correct initial value of mask-repeat in property_database.js.

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

Comment 6

[u459114]

Attachment: Bug 1308963 - Part 3. Set initial value of mask-position as (0, 0).

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

Comment 7

[u459114]

Attachment: Bug 1308963 - Part 4. Correct initial value of mask-position in property_database.js

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

Comment 8

[u459114]

Revert the patches in Bug 1258623 and Bug 1277788

Comment 9

[u459114]

File an issue to w3c
https://github.com/w3c/csswg-drafts/issues/599

Comment 10

[u459114]

Comment on attachment 8800719 [details]
Bug 1308963 - Part 4. Correct initial value of mask-position in property_database.js

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

Comment 11

[u459114]

Comment on attachment 8800717 [details]
Bug 1308963 - Part 2. Correct initial value of mask-repeat in property_database.js.

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

Comment 12

[u459114]

Comment on attachment 8800718 [details]
Bug 1308963 - Part 3. Set initial value of mask-position as (0, 0).

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

Comment 13

[u459114]

Comment on attachment 8800719 [details]
Bug 1308963 - Part 4. Correct initial value of mask-position in property_database.js

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

Comment 15

[u459114]

Comment on attachment 8800719 [details]
Bug 1308963 - Part 4. Correct initial value of mask-position in property_database.js

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

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we should at least wait for feedback on https://github.com/w3c/csswg-drafts/issues/599 before proceeding here.

Comment 17

[Astley Chen (inactive)]

Per the resolution from https://github.com/w3c/csswg-drafts/issues/599, it's time to move on the change.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8800718 [details]
Bug 1308963 - Part 3. Set initial value of mask-position as (0, 0).

https://reviewboard.mozilla.org/r/85592/#review87978

::: layout/style/nsCSSParser.cpp:12199
(Diff revision 2)
> -    aState.mRepeat->mXValue.SetIntValue(NS_STYLE_IMAGELAYER_REPEAT_REPEAT,
> +  }
> -                                        eCSSUnit_Enumerated);

Where does the initial value of repeat go here? I suppose you still need it.

::: layout/style/nsStyleStruct.h:577
(Diff revision 2)
>    static float GetInitialPositionForLayerType(LayerType aType) {
> -    return (aType == LayerType::Background) ? 0.0f : 0.5f;
> +    return 0.0f;
>    }

Given that it now just returns 0.0f, could we replace all its callsites with this literal instead? Or if you prefer, use a constant for 0.0f? I don't see much value to have a constant with 0.0f, though.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8800716 [details]
Bug 1308963 - Part 1. Set initial value of mask-repeat as repeat.

https://reviewboard.mozilla.org/r/85588/#review87980

::: layout/style/nsStyleStruct.cpp:2590
(Diff revision 1)
>  bool
>  nsStyleImageLayers::Repeat::IsInitialValue(LayerType aType) const

You should probably remove the parameter given it is unused now.

::: layout/style/nsStyleStruct.cpp:2597
(Diff revision 1)
>  void
>  nsStyleImageLayers::Repeat::SetInitialValues(LayerType aType)

Same here.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8800716 [details]
Bug 1308963 - Part 1. Set initial value of mask-repeat as repeat.

https://reviewboard.mozilla.org/r/85588/#review87984

::: layout/style/nsStyleStruct.cpp:2597
(Diff revision 1)
>  void
>  nsStyleImageLayers::Repeat::SetInitialValues(LayerType aType)

I think you can also just put these functions inline in the class.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8800717 [details]
Bug 1308963 - Part 2. Correct initial value of mask-repeat in property_database.js.

https://reviewboard.mozilla.org/r/85590/#review87988

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Per discussion with CJKu on IRC, taking the r?.

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8800719 [details]
Bug 1308963 - Part 4. Correct initial value of mask-position in property_database.js

https://reviewboard.mozilla.org/r/85594/#review87990

::: layout/style/test/test_computed_style.html:313
(Diff revision 4)
>      ],
>      "mask-mode": [
>        "match-source"
>      ],
>      "mask-position": [
> -      "50%", "50% 50%", "center"
> +      "0% 0%", "left top"

Should there also be a "0%" then?

Comment 24

[u459114]

Comment on attachment 8800718 [details]
Bug 1308963 - Part 3. Set initial value of mask-position as (0, 0).

https://reviewboard.mozilla.org/r/85592/#review87978

> Where does the initial value of repeat go here? I suppose you still need it.

We already give an initial value several lines above
http://searchfox.org/mozilla-central/source/layout/style/nsCSSParser.cpp#12237

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8800718 [details]
Bug 1308963 - Part 3. Set initial value of mask-position as (0, 0).

https://reviewboard.mozilla.org/r/85592/#review87978

> We already give an initial value several lines above
> http://searchfox.org/mozilla-central/source/layout/style/nsCSSParser.cpp#12237

Hmm, okay. Then I guess you can remove them in part 1.

Comment 26

[u459114]

Comment on attachment 8800716 [details]
Bug 1308963 - Part 1. Set initial value of mask-repeat as repeat.

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

Comment 27

[u459114]

Comment on attachment 8800717 [details]
Bug 1308963 - Part 2. Correct initial value of mask-repeat in property_database.js.

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

Comment 28

[u459114]

Comment on attachment 8800718 [details]
Bug 1308963 - Part 3. Set initial value of mask-position as (0, 0).

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

Comment 29

[u459114]

Comment on attachment 8800719 [details]
Bug 1308963 - Part 4. Correct initial value of mask-position in property_database.js

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

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8800716 [details]
Bug 1308963 - Part 1. Set initial value of mask-repeat as repeat.

https://reviewboard.mozilla.org/r/85588/#review88010

r=me with the following issue addressed.

::: layout/style/nsStyleStruct.h:749
(Diff revision 2)
> +      return mXRepeat == NS_STYLE_IMAGELAYER_REPEAT_NO_REPEAT &&
> +             mYRepeat == NS_STYLE_IMAGELAYER_REPEAT_NO_REPEAT;

These should be NS_STYLE_IMAGELAYER_REPEAT_REPEAT.

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8800718 [details]
Bug 1308963 - Part 3. Set initial value of mask-position as (0, 0).

https://reviewboard.mozilla.org/r/85592/#review88012

Comment 32

[u459114]

Comment on attachment 8800716 [details]
Bug 1308963 - Part 1. Set initial value of mask-repeat as repeat.

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

Comment 33

[u459114]

Comment on attachment 8800717 [details]
Bug 1308963 - Part 2. Correct initial value of mask-repeat in property_database.js.

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

Comment 34

[u459114]

Comment on attachment 8800718 [details]
Bug 1308963 - Part 3. Set initial value of mask-position as (0, 0).

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

Comment 35

[u459114]

Comment on attachment 8800719 [details]
Bug 1308963 - Part 4. Correct initial value of mask-position in property_database.js

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

Comment 36

[u459114]

Comment on attachment 8800719 [details]
Bug 1308963 - Part 4. Correct initial value of mask-position in property_database.js

https://reviewboard.mozilla.org/r/85594/#review87990

> Should there also be a "0%" then?

https://drafts.csswg.org/css-backgrounds-3/#position
If only one value is specified, the second value is assumed to be ‘center’.

So, "0%" is equal to "0% center" which is not a initila value

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8800719 [details]
Bug 1308963 - Part 4. Correct initial value of mask-position in property_database.js

https://reviewboard.mozilla.org/r/85594/#review87990

> https://drafts.csswg.org/css-backgrounds-3/#position
> If only one value is specified, the second value is assumed to be ‘center’.
> 
> So, "0%" is equal to "0% center" which is not a initila value

OK. Then it is probably better putting "0%" into the other list.

Comment 38

[u459114]

Comment on attachment 8800717 [details]
Bug 1308963 - Part 2. Correct initial value of mask-repeat in property_database.js.

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

Comment 39

[u459114]

Comment on attachment 8800718 [details]
Bug 1308963 - Part 3. Set initial value of mask-position as (0, 0).

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

Comment 40

[u459114]

Comment on attachment 8800719 [details]
Bug 1308963 - Part 4. Correct initial value of mask-position in property_database.js

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

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82ea0e89d193
Part 1. Set initial value of mask-repeat as repeat. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/7ecb34e2ed3a
Part 2. Correct initial value of mask-repeat in property_database.js. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/ac12c1e5bea7
Part 3. Set initial value of mask-position as (0, 0). r=xidorn
https://hg.mozilla.org/integration/autoland/rev/2161133a1350
Part 4. Correct initial value of mask-position in property_database.js r=xidorn

Comment 42

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/82ea0e89d193
https://hg.mozilla.org/mozilla-central/rev/7ecb34e2ed3a
https://hg.mozilla.org/mozilla-central/rev/ac12c1e5bea7
https://hg.mozilla.org/mozilla-central/rev/2161133a1350

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In https://hg.mozilla.org/mozilla-central/rev/82ea0e89d193 in the change to ParseImageLayersItem, why is there no *new* (common) call to aState.mRepeat->mXValue.SetIntValue ?

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

er, never mind, comment 24 answers that

Comment 46

[Chris Mills [:cmills]]

I've submitted a PR to change the initial values of these properties to the updated ones in the MDN properties data repo:

https://github.com/mdn/data/pull/48

I've also added a note to the Fx52 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/52#CSS