Closed Bug 828805 Opened 12 years ago Closed 12 years ago

implement paint-order property from SVG 2

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
relnote-firefox --- -

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

Attached patch WIP (v0.1) (obsolete) — Splinter Review
Here's a WIP. This works, but I haven't written any tests yet. Also the property probably needs to go behind a pref.
Attached patch patch (v1.0)Splinter Review
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...
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.
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.
Ah yep, fair enough.
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+
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
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.
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.
Blocks: svg2
Blocks: 1437267
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: