1251161 - (mask-ship) Ship CSS positioned mask support on beta & release channels

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[u459114]

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.

Comment 1

[u459114]

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

Comment 2

[Astley Chen (inactive)]

Attachment: Bug 1251161 - enable CSS positioned mask support.

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

Comment 3

[u459114]

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

Comment 4

[Astley Chen (inactive)]

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/

Comment 5

[Astley Chen (inactive)]

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

Comment 6

[Astley Chen (inactive)]

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/

Comment 7

[Astley Chen (inactive)]

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.

Comment 8

[David Baron :dbaron:]

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)

Comment 9

[David Baron :dbaron:]

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

Comment 10

[Astley Chen (inactive)]

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

Comment 11

[Astley Chen (inactive)]

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

Comment 12

[Astley Chen (inactive)]

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/

Comment 13

[David Baron :dbaron:]

bug 1283679 definitely needs to be fixed before this.

Comment 14

[David Baron :dbaron:]

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.

Comment 15

[David Baron :dbaron:]

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

Comment 16

[Astley Chen (inactive)]

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

Comment 17

[Astley Chen (inactive)]

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

Comment 18

[Astley Chen (inactive)]

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

Comment 19

[u459114]

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

Comment 20

[Astley Chen (inactive)]

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/

Comment 21

[Astley Chen (inactive)]

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

resend the review request.

Comment 22

[David Baron :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.

Comment 23

[David Baron :dbaron:]

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?

Comment 24

[Astley Chen (inactive)]

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

Based on comment 22, let's re-define the ship criteria. Thanks.

Comment 25

[u459114]

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

Comment 26

[u459114]

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

Comment 27

[u459114]

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

Comment 28

[u459114]

(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

Comment 29

[Astley Chen (inactive)]

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

Comment 30

[Astley Chen (inactive)]

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

Comment 31

[Astley Chen (inactive)]

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

Comment 32

[Sebastian Zartner [:sebo]]

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

Comment 33

[Astley Chen (inactive)]

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.

Comment 34

[Astley Chen (inactive)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59472/diff/5-6/

Comment 35

[Astley Chen (inactive)]

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

direct enabling patch review to heycam.

Comment 36

[Cameron McCormack (:heycam)]

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

Comment 37

[Astley Chen (inactive)]

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

Comment 38

[Astley Chen (inactive)]

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

Comment 39

[Cameron McCormack (:heycam)]

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

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

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/367db1b3a184
enable CSS positioned mask support. r=heycam

Comment 41

[Carsten Book [:Tomcat]]

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

Comment 43

[Chris Mills [:cmills]]

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!

Comment 44

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.