828805 - implement paint-order property from SVG 2

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: 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.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: 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)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Cameron McCormack (:heycam)]

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.

Comment 6

[Cameron McCormack (:heycam)]

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

Docs folks: note that this is behind a pref svg.paint-order.enabled.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note that docs folks are swamped with b2g for the last many months and foreseeable future.  I recommend just documenting this yourself.

Comment 8

[Cameron McCormack (:heycam)]

Ah yep, fair enough.

Comment 9

[Cameron McCormack (:heycam)]

Landed a couple of followups:

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

Comment 10

[Cameron McCormack (:heycam)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bfef9b308f92
https://hg.mozilla.org/mozilla-central/rev/761992365553
https://hg.mozilla.org/mozilla-central/rev/991938589ebe

Comment 12

[Mayank Bansal]

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

[Robert Longson [:longsonr]]

mayankleoboy1 please raise a new bug.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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

Comment 15

[Cameron McCormack (:heycam)]

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.

Comment 16

[Jesse Ruderman]

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.