Closed Bug 1308963 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: fvsch, Assigned: u459114)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [webcompat])

Attachments

(4 files)

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: 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]
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)
Blocks: mask-ship
Blocks: 1305697
Revert the patches in Bug 1258623 and Bug 1277788
Blocks: 1300384
Blocks: 1301106
Keywords: dev-doc-needed
Priority: -- → P1
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 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 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 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 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 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+
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 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 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 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 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 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.
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)
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.

Attachment

General

Creator:
Created:
Updated:
Size: