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)
Core
Layout
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)
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.
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
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");
See Also: → 1301106,
https://webcompat.com/issues/3409
Updated•8 years ago
|
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Revert the patches in Bug 1258623 and Bug 1277788
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) |
Updated•8 years ago
|
Keywords: dev-doc-needed
Priority: -- → P1
Comment hidden (mozreview-request) |
Attachment #8800716 -
Flags: review?(dbaron)
Attachment #8800717 -
Flags: review?(dbaron)
Attachment #8800718 -
Flags: review?(dbaron)
Attachment #8800719 -
Flags: review?(dbaron)
I think we should at least wait for feedback on https://github.com/w3c/csswg-drafts/issues/599 before proceeding here.
See Also: → https://github.com/w3c/csswg-drafts/issues/599
Updated•8 years ago
|
Status: NEW → ASSIGNED
Flags: needinfo?(aschen)
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.
Flags: needinfo?(aschen)
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.
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.
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.
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
Attachment #8800717 -
Flags: review+
Comment 22•8 years ago
|
||
Per discussion with CJKu on IRC, taking the r?.
Updated•8 years ago
|
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•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?
Attachment #8800719 -
Flags: review?(xidorn+moz) → review+
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
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Attachment #8800716 -
Flags: review?(xidorn+moz) → review+
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
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•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
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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)
Comment 46•8 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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•