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

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: fvsch, Assigned: u459114)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [webcompat], )

Attachments

(4 attachments)

Reporter

Description

3 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)
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.
Blocks: 1294660
Flags: needinfo?(cku)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Updated

3 years ago
Assignee: nobody → cku
Flags: needinfo?(cku)
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");
Whiteboard: [webcompat]
Assignee

Comment 3

3 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)
Assignee

Updated

3 years ago
Blocks: mask-ship
Assignee

Updated

3 years ago
Blocks: 1305697
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 8

3 years ago
Revert the patches in Bug 1258623 and Bug 1277788
Assignee

Updated

3 years ago
Blocks: 1300384
Assignee

Updated

3 years ago
Blocks: 1301106
Assignee

Comment 9

3 years ago
File an issue to w3c
https://github.com/w3c/csswg-drafts/issues/599
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Duplicate of this bug: 1310135
Keywords: dev-doc-needed
Priority: -- → P1
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8800716 - Flags: review?(dbaron)
Attachment #8800717 - Flags: review?(dbaron)
Attachment #8800718 - Flags: review?(dbaron)
Attachment #8800719 - Flags: review?(dbaron)
Status: NEW → ASSIGNED
Flags: needinfo?(aschen)
Per the resolution from https://github.com/w3c/csswg-drafts/issues/599, it's time to move on the change.
Flags: needinfo?(aschen)

Comment 18

3 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.

Comment 19

3 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.

Comment 20

3 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.

Comment 21

3 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
Attachment #8800717 - Flags: review+
Per discussion with CJKu on IRC, taking the r?.
Attachment #8800716 - Flags: review?(dbaron)
Attachment #8800717 - Flags: review?(dbaron)
Attachment #8800718 - Flags: review?(dbaron)
Attachment #8800719 - Flags: review?(dbaron) → review?(xidorn+moz)

Comment 23

3 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?
Attachment #8800719 - Flags: review?(xidorn+moz) → review+
Assignee

Comment 24

3 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

Comment 25

3 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

3 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.
Attachment #8800716 - Flags: review?(xidorn+moz) → review+

Comment 31

3 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
Attachment #8800718 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 36

3 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

Comment 37

3 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 41

3 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
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 ?
Flags: needinfo?(cku)
er, never mind, comment 24 answers that
Flags: needinfo?(cku)
Duplicate of this bug: 1305697
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
You need to log in before you can comment on or make changes to this bug.