u459114 Reporter
	Description • 8 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.

	u459114 Reporter
	Comment 1 • 8 years ago

bug 1258286 - r?
bug 1272859 - r?
bug 1273804 - start working on this issue from Jun 1th.

	Astley Chen (inactive) Assignee
	Comment 2 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/59472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59472/

	u459114 Reporter
	Comment 3 • 8 years ago

bug 1231643 comment 10 - we need fuzzy-if on MacOS

	Astley Chen (inactive) Assignee
	Comment 4 • 8 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/

	Astley Chen (inactive) Assignee
	Comment 5 • 8 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.

	Astley Chen (inactive) Assignee
	Comment 6 • 8 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/

	Astley Chen (inactive) Assignee
	Comment 7 • 8 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.

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

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)

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

(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?

	Astley Chen (inactive) Assignee
	Comment 10 • 8 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.

	Astley Chen (inactive) Assignee
	Comment 11 • 8 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.

	Astley Chen (inactive) Assignee
	Comment 12 • 8 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/

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

bug 1283679 definitely needs to be fixed before this.

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

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.

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

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

	Astley Chen (inactive) Assignee
	Comment 16 • 8 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.

	Astley Chen (inactive) Assignee
	Comment 17 • 8 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.

	Astley Chen (inactive) Assignee
	Comment 18 • 8 years ago

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

	u459114 Reporter
	Comment 19 • 8 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.

	Astley Chen (inactive) Assignee
	Comment 20 • 8 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/

	Astley Chen (inactive) Assignee
	Comment 21 • 8 years ago

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

resend the review request.

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

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.

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

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?

	Astley Chen (inactive) Assignee
	Comment 24 • 8 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.

	u459114 Reporter
	Comment 25 • 8 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.

	u459114 Reporter
	Comment 26 • 8 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)

	u459114 Reporter
	Comment 27 • 8 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.

	u459114 Reporter
	Comment 28 • 8 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

	Astley Chen (inactive) Assignee
	Comment 29 • 8 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 ?

	Astley Chen (inactive) Assignee
	Comment 30 • 8 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

	Astley Chen (inactive) Assignee
	Comment 31 • 8 years ago

Per bug 1294660 and current status, presumably it's about time to enable this feature on nightly and aurora channels.

	Sebastian Zartner [:sebo]
	Comment 32 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 33 • 8 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.

	Astley Chen (inactive) Assignee
	Comment 35 • 8 years ago

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

direct enabling patch review to heycam.

	Cameron McCormack (:heycam)
	Comment 36 • 8 years ago

(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?

	Astley Chen (inactive) Assignee
	Comment 37 • 8 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.

	Astley Chen (inactive) Assignee
	Comment 38 • 8 years ago

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1a4484b4b9e210edf18e5eee1c13afc269bd7fb

	Cameron McCormack (:heycam)
	Comment 39 • 8 years ago
mozreview-review

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

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

	Pulsebot
	Comment 40 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 41 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/367db1b3a184

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

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!

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 44 • 7 years ago

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.