Rik Cabanier Assignee
	Description • 10 years ago

The CSS blending spec defines the 'isolation' keyword: http://dev.w3.org/fxtf/compositing-1/#propdef-isolation

It's already implemented by Chrome and WebKit and shipping in Safari.
This patch will implement the CSS parsing, the implementation and add a couple of test files.

	Rik Cabanier Assignee
	Comment 1 • 10 years ago

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8938e6f3880d
Will follow up with patches for rendering + tests

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

Shouldn't this be behind a separate pref (and in particular, one that isn't currently enabled)?

Did you run the mochitests in layout/style with the pref enabled?

	Rik Cabanier Assignee
	Comment 3 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #2)
> Shouldn't this be behind a separate pref (and in particular, one that isn't
> currently enabled)?

I could add it as a separate pref if you wanted. However, since it's such a minor feature and will reuse the existing code that manages the parent stacking contexts of mix-blend-mode, I didn't think it was needed.

> Did you run the mochitests in layout/style with the pref enabled?
yes. mix-blend-mode is enabled by default so the tests ran with this property enabled.

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

(In reply to Rik Cabanier from comment #3)
> (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from
> comment #2)
> > Shouldn't this be behind a separate pref (and in particular, one that isn't
> > currently enabled)?
> 
> I could add it as a separate pref if you wanted. However, since it's such a
> minor feature and will reuse the existing code that manages the parent
> stacking contexts of mix-blend-mode, I didn't think it was needed.

So it either needs to be a separate pref, or you need to wait to land this patch until the feature is ready to turn on.  I think it's better to do a separate pref.

	Rik Cabanier Assignee
	Comment 5 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #4)
> (In reply to Rik Cabanier from comment #3)
> > (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from
> > comment #2)
> > > Shouldn't this be behind a separate pref (and in particular, one that isn't
> > > currently enabled)?
> > 
> > I could add it as a separate pref if you wanted. However, since it's such a
> > minor feature and will reuse the existing code that manages the parent
> > stacking contexts of mix-blend-mode, I didn't think it was needed.
> 
> So it either needs to be a separate pref, or you need to wait to land this
> patch until the feature is ready to turn on.  I think it's better to do a
> separate pref.

ok. Will do.

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

Comment on attachment 8500228 [details] [diff] [review]
Implemented parsing of CSS isolation property

>diff --git a/layout/style/nsCSSKeywordList.h b/layout/style/nsCSSKeywordList.h

>+CSS_KEY(isolated, isolated)

The spec says this is called "isolate" rather than "isolated".  "isolated" is probably more consistent with the rest of CSS, though.  If WebKit is shipping this, though, we should match whatever they're doing, and if it's "isolated", get the spec changed.

>diff --git a/layout/style/nsCSSPropList.h b/layout/style/nsCSSPropList.h

>+    "layout.css.mix-blend-mode.enabled",

Per above, give this its own pref (and add the default, off, to modules/libpref/init/all.js).

>diff --git a/layout/style/nsStyleConsts.h b/layout/style/nsStyleConsts.h
>+// See nsStyleDisplay
>+#define NS_STYLE_ISOLATION_MODE_AUTO            0
>+#define NS_STYLE_ISOLATION_MODE_ISOLATED        1

Drop "MODE_" from these keywords, and if appropriate change ISOLATED to ISOLATE to match the name.

>diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp

You need to add code to nsStyleDisplay::CalcDifference to cause a nsChangeHint_RepaintFrame to be processed when mIsolation changes.  It can go as an || along with the check for if (mMixBlendMode != aOther.mMixBlendMode).


>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h
>   uint8_t mOrient;              // [reset] see nsStyleConsts.h
>   uint8_t mMixBlendMode;        // [reset] see nsStyleConsts.h
>+  uint8_t mIsolation;            // [reset] see nsStyleConsts.h
>   uint8_t mWillChangeBitField;  // [reset] see nsStyleConsts.h. Stores a

Fix indent of the comment to match surrounding lines.  (Did you get tabs in the patch?)

>diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js

Condition these changes on the new pref.

>diff --git a/layout/style/test/test_computed_style_prefs.html b/layout/style/test/test_computed_style_prefs.html

And these.

r=dbaron with that

	Rik Cabanier Assignee
	Comment 7 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #6)
> Comment on attachment 8500228 [details] [diff] [review]
> Implemented parsing of CSS isolation property
> 
> >diff --git a/layout/style/nsCSSKeywordList.h b/layout/style/nsCSSKeywordList.h
> 
> >+CSS_KEY(isolated, isolated)
> 
> The spec says this is called "isolate" rather than "isolated".  "isolated"
> is probably more consistent with the rest of CSS, though.  If WebKit is
> shipping this, though, we should match whatever they're doing, and if it's
> "isolated", get the spec changed.

Good catch! WebKit 7.2 and 8 already ships with this keyword so I will change it. 

> >diff --git a/layout/style/nsCSSPropList.h b/layout/style/nsCSSPropList.h
> 
> >+    "layout.css.mix-blend-mode.enabled",
> 
> Per above, give this its own pref (and add the default, off, to
> modules/libpref/init/all.js).
> 
> >diff --git a/layout/style/nsStyleConsts.h b/layout/style/nsStyleConsts.h
> >+// See nsStyleDisplay
> >+#define NS_STYLE_ISOLATION_MODE_AUTO            0
> >+#define NS_STYLE_ISOLATION_MODE_ISOLATED        1
> 
> Drop "MODE_" from these keywords, and if appropriate change ISOLATED to
> ISOLATE to match the name.
> 
> >diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
> 
> You need to add code to nsStyleDisplay::CalcDifference to cause a
> nsChangeHint_RepaintFrame to be processed when mIsolation changes.  It can
> go as an || along with the check for if (mMixBlendMode !=
> aOther.mMixBlendMode).

I had that in the rendering patch, but will move it to this one.

	Rik Cabanier Assignee
	Comment 10 • 10 years ago

Comment on attachment 8502256 [details] [diff] [review]
2. Implementation + test file for isolation property

Try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=857945fa10ef

This one is nice and simple :-)

	Carsten Book [:Tomcat]
	Comment 11 • 10 years ago

(In reply to Rik Cabanier from comment #10)
> Comment on attachment 8502256 [details] [diff] [review]
> Implementation + test file for isolation property
> 
> Try build:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=857945fa10ef
> 
> This one is nice and simple :-)

The try run shows a lot of errors like REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/css-blending/blend-isolation.html | boolean preference 'layout.css.isolation.enable' not known or wrong type - do we care about this erors on the try run ?

	Rik Cabanier Assignee
	Comment 12 • 10 years ago

(In reply to Carsten Book [:Tomcat] from comment #11)
> (In reply to Rik Cabanier from comment #10)
> > Comment on attachment 8502256 [details] [diff] [review]
> > Implementation + test file for isolation property
> > 
> > Try build:
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=857945fa10ef
> > 
> > This one is nice and simple :-)
> 
> The try run shows a lot of errors like REFTEST TEST-UNEXPECTED-FAIL |
> file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/css-blending/
> blend-isolation.html | boolean preference 'layout.css.isolation.enable' not
> known or wrong type - do we care about this erors on the try run ?

yes, I had a typo in the test file. Sorry about that.
Will upload a new version.

	Rik Cabanier Assignee
	Comment 14 • 10 years ago

try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bf3d338f7f26

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 10 years ago

Please don't request checkin until your Try push is complete and you've verified that there are no issues.

	Rik Cabanier Assignee
	Comment 16 • 10 years ago

succesful try run: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bf3d338f7f26

	Carsten Book [:Tomcat]
	Comment 17 • 10 years ago

there are still failures like REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/css-blending/blend-isolation.html | boolean preference 'layout.css.isolation.enable' not known or wrong type - so i guess this need be fixed first or ?

	Rik Cabanier Assignee
	Comment 18 • 10 years ago

Sorry, I pasted the wrong try build:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f19fea9d37de

	Carsten Book [:Tomcat]
	Comment 19 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e457165df2a3

	Carsten Book [:Tomcat]
	Comment 20 • 10 years ago

Hi, Rik

so there are 2 issues:

had to backout 2. Implementation + test file for isolation property for bustage 

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3017115&repo=mozilla-inbound

and in case this depends on  the patch Implemented parsing of CSS isolation property, this patch didn;t apply cleanly and so couldn't checked in because of

applying isolationparsing.patch
patching file modules/libpref/init/all.js
Hunk #1 FAILED at 1997
1 out of 1 hunks FAILED -- saving rejects to file modules/libpref/init/all.js.rej

	Rik Cabanier Assignee
	Comment 22 • 10 years ago

try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=32515453f407

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5108055f652
https://hg.mozilla.org/integration/mozilla-inbound/rev/3012ee3c0083

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 10 years ago

Backed out for reftest asserts. Please run debug builds on your Try pushes. You can save a lot of resources by only running reftests, though, i.e. |try: -b do -p all -u reftest -t none|.
https://hg.mozilla.org/integration/mozilla-inbound/rev/46b36a35fddd

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3050687&repo=mozilla-inbound

	Rik Cabanier Assignee
	Comment 25 • 10 years ago

try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=005b07de2745

	Carsten Book [:Tomcat]
	Comment 26 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6fd04ea4ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/c58d8895c9ed

	Carsten Book [:Tomcat]
	Comment 27 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/4e6fd04ea4ba
https://hg.mozilla.org/mozilla-central/rev/c58d8895c9ed

	Jean-Yves Perrier [:teoli]
	Comment 28 • 10 years ago

Doc created:
https://developer.mozilla.org/en-US/docs/Web/CSS/isolation
and
https://developer.mozilla.org/en-US/Firefox/Releases/36#CSS