Implement isolation CSS property

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cabanier, Assigned: cabanier)

Tracking

({dev-doc-complete})

Trunk
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Assignee

Description

5 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.
Assignee

Updated

5 years ago
Assignee: nobody → cabanier
Assignee

Comment 1

5 years ago
try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8938e6f3880d
Will follow up with patches for rendering + tests
Attachment #8500228 - Flags: review?(dbaron)
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?
Flags: needinfo?(cabanier)
Assignee

Comment 3

5 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.
Flags: needinfo?(cabanier) → needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
Assignee

Comment 5

5 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.
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
Attachment #8500228 - Flags: review?(dbaron) → review+
Assignee

Comment 7

5 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.
Assignee

Comment 8

5 years ago
Attachment #8500228 - Attachment is obsolete: true
Assignee

Comment 10

5 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 :-)
Attachment #8502256 - Flags: review?(roc)
Assignee

Updated

5 years ago
Keywords: checkin-needed
(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 ?
Keywords: checkin-needed
Assignee

Comment 12

5 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.
Assignee

Comment 13

5 years ago
Attachment #8501249 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8502256 - Attachment description: Implementation + test file for isolation property → 2. Implementation + test file for isolation property
Assignee

Updated

5 years ago
Keywords: checkin-needed
Please don't request checkin until your Try push is complete and you've verified that there are no issues.
Keywords: checkin-needed
Assignee

Updated

5 years ago
Keywords: checkin-needed
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 ?
Keywords: checkin-needed
Assignee

Comment 18

5 years ago
Sorry, I pasted the wrong try build:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f19fea9d37de
Flags: needinfo?(cbook)
Assignee

Updated

5 years ago
Keywords: checkin-needed
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
Flags: needinfo?(cbook)
Assignee

Comment 21

5 years ago
Attachment #8502463 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
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
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4e6fd04ea4ba
https://hg.mozilla.org/mozilla-central/rev/c58d8895c9ed
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.