implement paint-order property from SVG 2

RESOLVED FIXED in mozilla21

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla21
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox -)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 700203 [details] [diff] [review]
WIP (v0.1)

Here's a WIP.  This works, but I haven't written any tests yet.  Also the property probably needs to go behind a pref.
(Assignee)

Comment 1

5 years ago
Created attachment 700863 [details] [diff] [review]
patch (v1.0)

r?bz for adding the property (the content/, layout/style/ and modules/ bits);
r?roc for the rendering (the layout/svg/ and gfx/ bits)
Assignee: nobody → cam
Attachment #700203 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #700863 - Flags: review?(roc)
Attachment #700863 - Flags: review?(bzbarsky)
Comment on attachment 700863 [details] [diff] [review]
patch (v1.0)

Review of attachment 700863 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsStyleConsts.h
@@ +868,5 @@
> +#define NS_STYLE_PAINT_ORDER_FILL               1
> +#define NS_STYLE_PAINT_ORDER_STROKE             2
> +#define NS_STYLE_PAINT_ORDER_MARKERS            3
> +#define NS_STYLE_PAINT_ORDER_LAST_VALUE NS_STYLE_PAINT_ORDER_MARKERS
> +#define NS_STYLE_PAINT_ORDER_BITWIDTH           2

Some documentation is needed here
Attachment #700863 - Flags: review?(roc) → review+
Comment on attachment 700863 [details] [diff] [review]
patch (v1.0)

>+          // Already seen this component.
>+          return false;

Shouldn't there be some error-reporting here?

>+        position += NS_STYLE_PAINT_ORDER_BITWIDTH;

There need to be some static asserts about the usable bits in "order" being enough for the maximal possible number of copies of BITWIDTH we might need.

In any case, please make seen and order unsigned?

>+       // Can't have "normal" in the middle of the list of paint components.
>+       return false;

Again, error reporting.

>+    value.SetIntValue(order, eCSSUnit_Enumerated);

Either this needs to live inside the "component != NS_STYLE_PAINT_ORDER_NORMAL" block or you need a static assert that 0 == NS_STYLE_PAINT_ORDER_NORMAL (which is what this code is assuming).  That assert might be a good idea anyway.

In nsCSSValue::AppendToString, should we be going for minimal serializations?  That is, given "paint-order: stroke", should we reserialize it as "stroke" or as "stroke fill markers" (as the patch does)?

The code in nsComputedDOMStyle::DoGetPaintOrder is basically identical to the code in nsCSSValue::AppendToString.  Can we share that code via some utility function as needed?

r=me with those fixed
Attachment #700863 - Flags: review?(bzbarsky) → review+
To be clear, the minimal serialization question is an open one...  "No" is an OK answer, as long as we understand why.  But normally we'd aim for minimal serialization...
(Assignee)

Comment 5

5 years ago
Hmm, well in the spec at the moment I've written "Computed value: as specified", which would mean minimal serialisation, assuming we don't have a separate "Serialize as:" line.

I can think of one potential advantage of maximal serialisation: it allows the author to determine the set of possible values for the property, in case we ever extend it.  Still, the main reason for filling in the unspecified components in their default order in the first place was to avoid the author having to know the complete set of components, so maybe it's not that necessary.

I think I'll adjust it to serialise minimally.


(In reply to Boris Zbarsky (:bz) from comment #3)
> Shouldn't there be some error-reporting here?

Will add some.

> >+        position += NS_STYLE_PAINT_ORDER_BITWIDTH;
> 
> There need to be some static asserts about the usable bits in "order" being
> enough for the maximal possible number of copies of BITWIDTH we might need.

There is assert in nsRuleNode.cpp, which isn't about "order" being big enough but nsStyleSVG::mPaintOrder.  I'll add one here for "order" too.

> In any case, please make seen and order unsigned?

OK, though CSSValue::SetIntValue takes a signed integer, so I'll need to cast it later anyway.

> >+    value.SetIntValue(order, eCSSUnit_Enumerated);
> 
> Either this needs to live inside the "component !=
> NS_STYLE_PAINT_ORDER_NORMAL" block or you need a static assert that 0 ==
> NS_STYLE_PAINT_ORDER_NORMAL (which is what this code is assuming).  That
> assert might be a good idea anyway.

Yes, good idea.
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfef9b308f92

Docs folks: note that this is behind a pref svg.paint-order.enabled.
Keywords: dev-doc-needed
Note that docs folks are swamped with b2g for the last many months and foreseeable future.  I recommend just documenting this yourself.
(Assignee)

Comment 8

5 years ago
Ah yep, fair enough.
(Assignee)

Comment 9

5 years ago
Landed a couple of followups:

https://hg.mozilla.org/integration/mozilla-inbound/rev/761992365553
https://hg.mozilla.org/integration/mozilla-inbound/rev/991938589ebe
(Assignee)

Comment 10

5 years ago
I added a page on MDN (browser compatibility stating that the property is available in Nightly should be valid once the next inbound -> central merge and nightly build happens!):

https://developer.mozilla.org/en-US/docs/SVG/Attribute/paint-order
Flags: in-testsuite+
Keywords: dev-doc-needed → dev-doc-complete
https://hg.mozilla.org/mozilla-central/rev/bfef9b308f92
https://hg.mozilla.org/mozilla-central/rev/761992365553
https://hg.mozilla.org/mozilla-central/rev/991938589ebe
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Comment 12

5 years ago
dont know where to report this, but  https://developer.mozilla.org/en-US/docs/SVG/Attribute/paint-order  page uses high CPU when scrolled.

http://people.mozilla.com/~bgirard/cleopatra/#report=2ea8407db63deb9b5c6b005e129abe3b2d2eb9bb

Comment 13

5 years ago
mayankleoboy1 please raise a new bug.
(In reply to Cameron McCormack (:heycam) from comment #5)
> Hmm, well in the spec at the moment I've written "Computed value: as
> specified", which would mean minimal serialisation, assuming we don't have a
> separate "Serialize as:" line.

Just one comment here; I don't see the relationship between the computed value lines and serialization at all; see http://lists.w3.org/Archives/Public/www-style/2009Mar/0329.html
(Assignee)

Comment 15

5 years ago
OK, I had kinda remembered that people were inferring serialisation from the "Computed value" line and that some new specs might have been adding explicit serialisations for properties, but didn't realise that inferring the serialisation this way was incorrect.
relnote-firefox: --- → ?

Comment 16

5 years ago
This is listed on https://developer.mozilla.org/en-US/docs/Firefox_21_for_developers.  I don't think this feature is big enough for a relnote, but I filed bug 842480 to have the relnote page link to the MDN page.

Updated

5 years ago
relnote-firefox: ? → -
Blocks: 1328534
You need to log in before you can comment on or make changes to this bug.