Florens Verschelde :fvsch Reporter
	Description • 8 years ago

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)

	Daniel Holbert [:dholbert]
	Comment 1 • 8 years ago

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.

	Karl Dubost💡 :karlcow
	Comment 2 • 8 years ago

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");

	u459114 Assignee
	Comment 3 • 8 years ago

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)

	u459114 Assignee
	Comment 8 • 8 years ago

Revert the patches in Bug 1258623 and Bug 1277788

	u459114 Assignee
	Comment 9 • 8 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 16 • 8 years ago

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

	Astley Chen (inactive)
	Comment 17 • 8 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 18 • 8 years ago
mozreview-review

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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 19 • 8 years ago
mozreview-review

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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 20 • 8 years ago
mozreview-review

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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 21 • 8 years ago
mozreview-review

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 22 • 8 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 23 • 8 years ago
mozreview-review

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?

	u459114 Assignee
	Comment 24 • 8 years ago
mozreview-review-reply

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 25 • 8 years ago
mozreview-review-reply

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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 30 • 8 years ago
mozreview-review

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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 31 • 8 years ago
mozreview-review

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

	u459114 Assignee
	Comment 36 • 8 years ago
mozreview-review-reply

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 37 • 8 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 41 • 8 years ago

Pushed by cku@mozilla.com:
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

	Phil Ringnalda (:philor)
	Comment 42 • 8 years ago
bugherder

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 43 • 8 years ago

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 ?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 44 • 8 years ago

er, never mind, comment 24 answers that

	Chris Mills [:cmills]
	Comment 46 • 7 years ago

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