Bug 1251161 (mask-ship)

Ship CSS positioned mask support on beta & release channels

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: u459114, Assigned: bmo)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla53
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox -, firefox53 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Mask longhand properties it turned off by MOZ_ENABLE_MASK_AS_SHORTHAND compile flag in bug 1243734. We should turn it on again after all mask longhand are supported.
Summary: Turn on mask-xxx longhand properties → Turn on mask-* longhand properties
(Reporter)

Updated

3 years ago
Blocks: 1224422
No longer depends on: 1224422
Keywords: dev-doc-needed
(Assignee)

Updated

3 years ago
Depends on: 1258286
(Assignee)

Updated

3 years ago
Depends on: 1258623
(Assignee)

Updated

3 years ago
Depends on: 1258626
(Assignee)

Updated

3 years ago
Summary: Turn on mask-* longhand properties → Ship mask-* longhand properties support
(Assignee)

Updated

3 years ago
No longer depends on: 1258623
(Reporter)

Updated

3 years ago
Depends on: 1272004
(Reporter)

Updated

3 years ago
Depends on: 1272859
(Reporter)

Updated

3 years ago
Depends on: 1272970
(Reporter)

Updated

3 years ago
Depends on: 1272973
(Assignee)

Updated

3 years ago
No longer depends on: 1272973
(Reporter)

Updated

3 years ago
Depends on: 1273804
(Reporter)

Updated

3 years ago
Depends on: 1275451
(Reporter)

Updated

3 years ago
Depends on: 1275478
(Reporter)

Updated

3 years ago
No longer depends on: 1275478
(Reporter)

Comment 1

3 years ago
bug 1258286 - r?
bug 1272859 - r?
bug 1273804 - start working on this issue from Jun 1th.
(Reporter)

Comment 3

3 years ago
bug 1231643 comment 10 - we need fuzzy-if on MacOS
(Assignee)

Comment 4

3 years ago
Comment on attachment 8763150 [details]
Bug 1251161 - enable CSS positioned mask support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59472/diff/1-2/
(Assignee)

Comment 5

3 years ago
(In reply to C.J. Ku[:cjku](UTC+1) from comment #3)
> bug 1231643 comment 10 - we need fuzzy-if on MacOS

Per comment 3, add fuzzy-if(OSX,x,x) into relevant test cases.
(Assignee)

Updated

3 years ago
Depends on: 1280707
(Reporter)

Updated

3 years ago
Assignee: cku → aschen
(Assignee)

Updated

3 years ago
Blocks: 1281101
(Assignee)

Updated

3 years ago
Blocks: 1276834
(Assignee)

Comment 6

3 years ago
Comment on attachment 8763150 [details]
Bug 1251161 - enable CSS positioned mask support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59472/diff/2-3/
(Assignee)

Updated

3 years ago
Attachment #8763150 - Flags: review?(dbaron)
(Assignee)

Comment 7

3 years ago
Submit try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b36234414b65

For bug 1280707, temporarily added *random* failure type for its intermittent failure. We'd found that reduce png image size could ease the failure and need some more time to figure out the root cause. At least, confirmed it's nothing to do with masking implementation. Leave bug 1280707 open for further investigation.
This failure in mochitest-5 seems like a problem:
 PROCESS-CRASH | layout/style/test/test_value_cloning.html | application crashed [@ TryToStartImageLoadOnValue] 
(so far happening on Linux opt, both 32 and 64 bit)
(In reply to Astley Chen [:astley] (UTC+8) from comment #7)
> For bug 1280707, temporarily added *random* failure type for its
> intermittent failure. We'd found that reduce png image size could ease the
> failure and need some more time to figure out the root cause. At least,
> confirmed it's nothing to do with masking implementation. Leave bug 1280707
> open for further investigation.

It seems like it would be good to understand what's going on here.




The intent to ship thread is:
https://groups.google.com/d/topic/mozilla.dev.platform/6lMOvomdZFU/discussion

Are there other implementations other than Gecko and Chrome?
(Assignee)

Updated

3 years ago
Depends on: 1281971
(Assignee)

Comment 10

3 years ago
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #8)
> This failure in mochitest-5 seems like a problem:
>  PROCESS-CRASH | layout/style/test/test_value_cloning.html | application
> crashed [@ TryToStartImageLoadOnValue] 
> (so far happening on Linux opt, both 32 and 64 bit)

Thanks to point this out. Fired bug 1281971 to investigate this issue and set as ship blocker.
(Assignee)

Comment 11

3 years ago
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #9)
> It seems like it would be good to understand what's going on here.
> 
Addressed in bug 1280707 comment 1, image decoding is async so that larger size image could not be ready for render or used to be a mask before snapshot. Ethan's shrink the test image size to avoid this problem.
Eventually, we might need to come up with a better mechanism to overcome this problem.
> 
> The intent to ship thread is:
> https://groups.google.com/d/topic/mozilla.dev.platform/6lMOvomdZFU/discussion
> 
> Are there other implementations other than Gecko and Chrome?
So far only Webkit/Blink/Gecko support mask-* properties, IE/Edge are still in the status of under consideration.
(Assignee)

Comment 12

3 years ago
Comment on attachment 8763150 [details]
Bug 1251161 - enable CSS positioned mask support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59472/diff/3-4/
(Assignee)

Updated

3 years ago
Depends on: 1284169
(Reporter)

Updated

3 years ago
Depends on: 1258510
(Reporter)

Updated

3 years ago
Depends on: 1286299
(Reporter)

Updated

3 years ago
Alias: mask-ship
(Reporter)

Updated

3 years ago
Depends on: 1286337
Comment on attachment 8763150 [details]
Bug 1251161 - enable CSS positioned mask support.

https://reviewboard.mozilla.org/r/59472/#review60858

Rather than continuing to check whether the dependencies are fixed, I'm going to clear this review request for now.  Please re-request when you think it's actually ready.
Also, do some/all of the dependencies of bug 1224422 also need to be fixed before we can do this?  It's not clear what the difference is between the dependency lists of these bugs (or why this bug is separate from bug 1224422).
(Assignee)

Comment 16

3 years ago
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #14)

> Rather than continuing to check whether the dependencies are fixed, I'm
> going to clear this review request for now.  Please re-request when you
> think it's actually ready.

Yes. sorry for keeping it pending so long. I'll re-send the request once we clean up *ALL* ship blockers that this bug is dependent on.
(Assignee)

Comment 17

3 years ago
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #15)
> Also, do some/all of the dependencies of bug 1224422 also need to be fixed
> before we can do this?  It's not clear what the difference is between the
> dependency lists of these bugs (or why this bug is separate from bug
> 1224422).

Ship bug is represented a control to enable or pref-on the feature which means it's turning to a developer-facing stage after the bug is fixed. All ship blockers should be set as dependencies on this bug. Meta bug 1224422 would be used to track all remaining bugs including code refactoring, performance tweaking, or any other suggestions that won't give great impact to authors or developers. Thanks.
(Reporter)

Updated

3 years ago
No longer depends on: 1280707
(Reporter)

Comment 19

3 years ago
(In reply to Astley Chen [:astley] (UTC+8) from comment #18)
> https://reviewboard.mozilla.org/r/59472/#review60858
> 
> All known dependencies were cleared and waiting for new try.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=77c47dbb988a
Looks like you need add fuzzy-if(skiaContent,1,XXX) to some test cases, win10 try machines fallback to skia-backend.
(Assignee)

Comment 20

3 years ago
Comment on attachment 8763150 [details]
Bug 1251161 - enable CSS positioned mask support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59472/diff/4-5/
Attachment #8763150 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8763150 - Flags: review?(dbaron)
(Assignee)

Comment 21

3 years ago
Comment on attachment 8763150 [details]
Bug 1251161 - enable CSS positioned mask support.

resend the review request.
Attachment #8763150 - Flags: review?(dbaron)
So, some concerns about things that seem like they should (or maybe should) block enabling:

bug 1280707 - If this isn't fixed, it seems like it should block; if it is, it should be marked as fixed.

bug 1277788 - we should at least have a decision here

bug 1258623 - same

bug 1234485 - how does our performance compare to other implementations?  Would this make a big difference?

I'd like to understand what's going on with bug 1235494.  It seems like a performance regression that we've *already* shipped.

We should also get a decision on bug 1251106 before shipping.

How big a performance problem is bug 1274236?

bug 1275478 seems like it needs to be fixed.  Same for bug 1285857.

Bug 1276834 also seems like it needs to be fixed, assuming it's what the spec requires and matches other implementations.
Also, that list was just from going through the dependencies of this bug and bug 1224422.  Did you do some bugzilla searches to see if there are bugs not connected to them that should block shipping?
(Assignee)

Comment 24

3 years ago
Comment on attachment 8763150 [details]
Bug 1251161 - enable CSS positioned mask support.

Based on comment 22, let's re-define the ship criteria. Thanks.
Attachment #8763150 - Flags: review?(dbaron)
(Reporter)

Comment 25

3 years ago
(In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #22)
> So, some concerns about things that seem like they should (or maybe should)
> block enabling:
> 
> bug 1280707 - If this isn't fixed, it seems like it should block; if it is,
> it should be marked as fixed.
That bug is already fixed by bug 1258510. We changed summary of it and keep it open for creating a new test case.(bug 1280707 comment 15)
> bug 1277788 - we should at least have a decision here
> bug 1258623 - same
For these two bugs, I think we should follow the spec. (And take repeat and "0% 0%" as initial value for webkit-mask-repeat & webkit-mask-position for compatibility if necessary.)
> 
> bug 1234485 - how does our performance compare to other implementations? 
> Would this make a big difference?
I will check how chrome do it.
> I'd like to understand what's going on with bug 1235494.  It seems like a
> performance regression that we've *already* shipped.
Will do.
> How big a performance problem is bug 1274236?
Not much I guess, but I don't have real data on hand. Will do benchmark.
> 
> bug 1275478 seems like it needs to be fixed.  Same for bug 1285857.
bug 1275478 is fixed by using skia surface. We let it open to keep tracing why D2D1 has problem in this use case. So, I don't think this one is a blocker.
And yes, I think bug 1285857 need to be fixed and landed before mask-as-shorhand enable.
> Bug 1276834 also seems like it needs to be fixed, assuming it's what the
> spec requires and matches other implementations.
We already follow the spec. There is a test case to guarantee correct behavior(https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/masking/mask-image-4a.html). This one is an optimization.
Depends on: 1285857
(Reporter)

Comment 26

3 years ago
(In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #22)
> We should also get a decision on bug 1251106 before shipping.
> 
Current gecko's behavior
<mask-layer> = <mask-reference> || <masking-mode>
Spec defined
<mask-layer> = <mask-reference> <masking-mode>?

I am inclined to keep current CSSParserImpl::ParseImageLayersItem behavior, since it's more reasonable. I don't see a benefit by forcing <masking-mode> be next to <mask-reference>. 

(By my testing, in chrome, you can not even add alpha or luminance value in -webkit-mask shorthand)
(Reporter)

Comment 27

3 years ago
(In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #23)
> Also, that list was just from going through the dependencies of this bug and
> bug 1224422.  Did you do some bugzilla searches to see if there are bugs not
> connected to them that should block shipping?
I searched "mask" keyword in summary field and did not see anything missed.
(Reporter)

Updated

3 years ago
Depends on: 1277788, 1258623

Updated

3 years ago
No longer depends on: 1285857
(Reporter)

Comment 28

3 years ago
(In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #22)
> I'd like to understand what's going on with bug 1235494.  It seems like a
> performance regression that we've *already* shipped.
I don't think it a regression. We already use nsChangeHint_UpdateEffects & nsChangeHint_UpdateOverflow before bug 686281 landed:
https://hg.mozilla.org/mozilla-central/annotate/ecafedaedd09/layout/style/nsStyleStruct.cpp#l1285
(Reporter)

Updated

3 years ago
No longer blocks: 1224422
(Assignee)

Comment 29

3 years ago
(In reply to C.J. Ku[:cjku](UTC+8) from comment #27)
> (In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain
> patch) from comment #23)
> > Also, that list was just from going through the dependencies of this bug and
> > bug 1224422.  Did you do some bugzilla searches to see if there are bugs not
> > connected to them that should block shipping?
> I searched "mask" keyword in summary field and did not see anything missed.

Doing keyword search on bugzilla may not give us sufficient information on how our current implementation quality is. I tend to enable mask-* properties support *only* on nightly and aurora from next release cycle(FF51) to stress out more potential bugs and give us more concrete information of the ship plan.

Hi dbaron, any thoughts on this plan ?
Status: NEW → ASSIGNED
Flags: needinfo?(dbaron)
(Assignee)

Updated

3 years ago
Depends on: 1234485
(Assignee)

Comment 30

3 years ago
Update latest try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a9c30d01070

Known failure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a9c30d01070&selectedJob=24912732

TEST-UNEXPECTED-FAIL | devtools/shared/tests/unit/test_css-properties-db.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | devtools/shared/tests/unit/test_css-properties-db.js | run_test - [run_test : 45] The client side CSS properties database names match those found on the platform. If this assertion fails, then the client side CSS properties list in devtools is out of date with the CSS properties on the platform. To fix this assertion open devtools/shared/css-properties-db.js and follow the instructions above the CSS_PROPERTIES on how to re-generate the list. - ["align-
Return code: 1

Updated

3 years ago
Depends on: 1291231
(Assignee)

Updated

3 years ago
Depends on: 1294660
(Assignee)

Comment 31

3 years ago
Per bug 1294660 and current status, presumably it's about time to enable this feature on nightly and aurora channels.
Flags: needinfo?(dbaron)
(Assignee)

Updated

3 years ago
Depends on: 1295062
(Reporter)

Updated

3 years ago
Depends on: 1295065
(Assignee)

Updated

3 years ago
Depends on: 1295095
(Reporter)

Updated

3 years ago
Depends on: 1296250
(Assignee)

Updated

3 years ago
Depends on: 1297031
Asking for the release note, which was originally added in bug 1228280.

Release Note Request (optional, but appreciated)
[Why is this notable]: Allowing web designers to apply masks to elements
[Suggested wording]: Added support for CSS mask-* longhand properties
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Masks, https://developer.mozilla.org/en-US/docs/tag/CSS%20Masks

Sebastian
relnote-firefox: --- → ?
(Assignee)

Comment 33

3 years ago
In bug 1294660, as of Firefox 51, we've enabled mask-* properties on firefox nightly and aurora to collect more feedback from developers and potential bugs from users.
In the meantime, bug 1295095 will be followed up to improve masking performance along with css animation before this feature is formally released.
Summary: Ship mask-* longhand properties support → Ship CSS positioned mask support on beta & release channels
(Reporter)

Updated

3 years ago
Depends on: 1300384
(Reporter)

Updated

3 years ago
Depends on: 1300401
(Assignee)

Updated

3 years ago
Depends on: 1301638
(Reporter)

Updated

3 years ago
Depends on: 1301106
(Reporter)

Updated

3 years ago
Depends on: 1303623
(Assignee)

Updated

3 years ago
Depends on: 1304437
Depends on: 1304991
Blocks: 658467
(Reporter)

Updated

3 years ago
Depends on: 1305636
(Reporter)

Updated

3 years ago
Depends on: 1299715
(Reporter)

Updated

3 years ago
Depends on: 1308617
(Reporter)

Updated

3 years ago
Depends on: 1308963
(Assignee)

Updated

3 years ago
Depends on: 1310135
(Reporter)

Updated

2 years ago
No longer depends on: 1295095, 1303623
(Reporter)

Updated

2 years ago
No longer depends on: 1300384
(Reporter)

Updated

2 years ago
Depends on: 1316719
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8763150 - Flags: review?(cam)
(Assignee)

Comment 35

2 years ago
Comment on attachment 8763150 [details]
Bug 1251161 - enable CSS positioned mask support.

direct enabling patch review to heycam.
Attachment #8763150 - Flags: review?(dbaron)
(In reply to Astley Chen [:astley] (UTC+8) from comment #33)
> In bug 1294660, as of Firefox 51, we've enabled mask-* properties on firefox
> nightly and aurora to collect more feedback from developers and potential
> bugs from users.
> In the meantime, bug 1295095 will be followed up to improve masking
> performance along with css animation before this feature is formally
> released.

Do we still plan to do this before shipping?
Flags: needinfo?(aschen)
(Assignee)

Comment 37

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #36)
> Do we still plan to do this before shipping?

The image mask performance improvement tracked in bug 1295095 were cleaned up, remaining bug 1313898 is for clip-path performance which targets to be fixed in week 47(this week).
It's time to take image mask feature ride the train.
Flags: needinfo?(aschen)

Comment 39

2 years ago
mozreview-review
Comment on attachment 8763150 [details]
Bug 1251161 - enable CSS positioned mask support.

https://reviewboard.mozilla.org/r/59472/#review94472
Attachment #8763150 - Flags: review?(cam) → review+

Comment 40

2 years ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/367db1b3a184
enable CSS positioned mask support. r=heycam

Comment 41

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/367db1b3a184
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Updated

2 years ago
Depends on: 1319667
Duplicate of this bug: 1304698
(Assignee)

Updated

2 years ago
Depends on: 1322286
(Assignee)

Updated

2 years ago
Depends on: 1322341
(Assignee)

Updated

2 years ago
Blocks: 1312613
(Reporter)

Updated

2 years ago
Depends on: 1329091
I've updated the support information on all the mask-* pages (see https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Masks for links), and added a note to the Fx53 release notes:

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

Let me know if you think it needs anything else. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
Thanks Chris! Since this is in developer notes already, and 53 is about to move to beta, I don't think we need to note it separately for 53 beta or release.
relnote-firefox: ? → -
You need to log in before you can comment on or make changes to this bug.