stylo: make `-moz-context-properties` support `fill-opacity` and `stroke-opacity` values

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: kuoe0, Assigned: kuoe0)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

`-moz-context-properties` only supports `all`, `none`, and `auto`. We should make it support `fill` and `stroke` to make `context-fill` and `context-stroke` work.

[1]: http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/servo/components/style/properties/longhand/inherited_svg.mako.rs#290
Since this is a non-standard property, we can only refer to MDN [1] and the code base [2] to see how much gap that we need to bridge. So, you might want to support all four values: fill, stroke, fill-opacity, stroke-opacity to match Gecko's behavior.


[1] https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-context-properties
[2] http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/layout/style/nsStyleConsts.h#1065-1069
Blocks: 1243581, 1352284
The value list pass into `CustomIdent::from_ident` is an exclusive list[1], so it doesn't mean we don't support "fill", "stroke", "fill-opacity" and "stroke-opacity". The value what we support is defined in [2], and we can see stylo already support "fill" and "stroke".

I change this bug to make it support "fill-opacity" and "stroke-opacity".

[1]: http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/servo/components/style/properties/longhand/inherited_svg.mako.rs#290
[2]: http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/servo/components/style/properties/gecko.mako.rs#4110-4114
Summary: stylo: make `-moz-context-properties` support `fill` and `stroke` values → stylo: make `-moz-context-properties` support `fill-opacity` and `stroke-opacity` values
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12a34534d550079b8a60f3ba414c491d2673e2b0
Attachment #8880283 - Flags: review?(cam)
Attachment #8880284 - Flags: review?(cam)

Comment 6

6 months ago
mozreview-review
Comment on attachment 8880283 [details]
Bug 1373159 - [servo] Make '-moz-context-properties' support 'fill-opacity' and 'stroke-opacity'.

https://reviewboard.mozilla.org/r/151642/#review157444
Attachment #8880283 - Flags: review?(cam) → review+

Comment 7

6 months ago
mozreview-review
Comment on attachment 8880284 [details]
Bug 1373159 - Add a bunch of test cases for 'context-fill-opacity' and 'context-stroke-opacity'.

https://reviewboard.mozilla.org/r/151644/#review157446

::: layout/reftests/svg/as-image/context-fill-opacity-02.svg:2
(Diff revision 1)
> +<svg xmlns='http://www.w3.org/2000/svg'>
> +  <rect width='100%' height='100%' fill='red' fill-opacity='context-fill-opacity'/>

Red is generally used in tests to indicate failure, so let's avoid using it here (and in the other test files) and just go with blue.

http://web-platform-tests.org/writing-tests/rendering.html

::: layout/reftests/svg/as-image/context-fill-or-stroke-opacity-01-ref.html:1
(Diff revision 1)
> +<html>

Let's put <!DOCTYPE html> here (and in the other reference files).  Although it doesn't make a difference for these tests, having the test file run in standards mode and the reference run in quirks mode is an unnecessary difference.

::: layout/reftests/svg/as-image/context-stroke-opacity-05.html:11
(Diff revision 1)
> +  stroke: 0.5;
> +  stroke: 0.5;

Drop one of these.
Attachment #8880284 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Attachment #8880283 - Attachment is obsolete: true
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=16ce34b806d30e35406e77439f74a50235019c47&selectedJob=110190934

Comment 10

5 months ago
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbd783f7fd35
Add a bunch of test cases for 'context-fill-opacity' and 'context-stroke-opacity'. r=heycam
Backed out for failing own tests on Windows 8 x64:

https://hg.mozilla.org/integration/autoland/rev/9e21351829db16b5ddf59f9335a000c75fce6fc6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bbd783f7fd35e57b31e77d884031a43a1bb723f6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110773139&repo=autoland

 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-fill-opacity-01.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-fill-or-stroke-opacity-01-ref.html | image comparison, max difference: 1, number of differing pixels: 10000
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-fill-opacity-02.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-fill-or-stroke-opacity-02-ref.html | image comparison, max difference: 1, number of differing pixels: 20000
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-fill-opacity-03.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-fill-or-stroke-opacity-03-ref.html | image comparison, max difference: 1, number of differing pixels: 10000
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-stroke-opacity-01.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-fill-or-stroke-opacity-01-ref.html | image comparison, max difference: 1, number of differing pixels: 10000
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-stroke-opacity-02.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-fill-or-stroke-opacity-02-ref.html | image comparison, max difference: 1, number of differing pixels: 20000
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-stroke-opacity-03.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/context-fill-or-stroke-opacity-03-ref.html | image comparison, max difference: 1, number of differing pixels: 10000
Flags: needinfo?(kuoe0)
I think that's the color rounding issue. All the diffs of the color are 1. I'll add a fuzzy(1,1) to prevent it fail for now. I'll file another bug to track that.
Flags: needinfo?(kuoe0)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

5 months ago
Pushed by tokuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07b3b72fe522
Add a bunch of test cases for 'context-fill-opacity' and 'context-stroke-opacity'. r=heycam
Backed out again in https://hg.mozilla.org/integration/autoland/rev/144114baf865

fuzzy(1,1) means that 1 pixel can be off by 1. You have 10000 or 20000 pixels off by one so it still failed the exact same way, and since it's only on one platform, that really should be fuzzy-if.
The bug to track the color rounding issue on Windows 8 is Bug 1377327.
See Also: → bug 1377327
Comment hidden (mozreview-request)

Comment 19

5 months ago
Pushed by tokuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ace0ff16ec7
Add a bunch of test cases for 'context-fill-opacity' and 'context-stroke-opacity'. r=heycam

Comment 20

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7ace0ff16ec7
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.