Spinning off from bug 763671 comment 61: In that bug, wesj is adding some CSS variables to mobile/android/themes/core/content.css (See top of attachment 8458340 [details] [diff] [review]) Those CSS variable-names end up appearing in the list of "properties" that we check for sortedness in test_bug657143.html, which triggers this test-failure: { TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug657143.html | SVG property list should be alphabetical - got "clip-path,clip-rule,color-interpolation,color-interpolation-filters,dominant-baseline,fill,fill-opacity,fill-rule,filter,flood-color,flood-opacity,image-rendering,lighting-color,marker-end,marker-mid,marker-start,mask,mask-type,paint-order,shape-rendering,stop-color,stop-opacity,stroke,stroke-dasharray,stroke-dashoffset,stroke-linecap,stroke-linejoin,stroke-miterlimit,stroke-opacity,stroke-width,text-anchor,text-rendering,vector-effect,moz-form-textcolor,moz-form-border-focused,moz-form-background-disabled,moz-form-border-invalid,moz-highlight-color,moz-form-button-background,moz-range-progress-disabled,moz-form-textcolor-placeholder,moz-form-border,moz-form-border-radius,moz-form-textcolor-disabled", expected "clip-path,clip-rule,color-interpolation,color-interpolation-filters,dominant-baseline,fill,fill-opacity,fill-rule,filter,flood-color,flood-opacity,image-rendering,lighting-color,marker-end,marker-mid,marker-start,mask,mask-type,moz-form-background-disabled,moz-form-border,moz-form-border-focused,moz-form-border-invalid,moz-form-border-radius,moz-form-button-background,moz-form-textcolor,moz-form-textcolor-disabled,moz-form-textcolor-placeholder,moz-highlight-color,moz-range-progress-disabled,paint-order,shape-rendering,stop-color,stop-opacity,stroke,stroke-dasharray,stroke-dashoffset,stroke-linecap,stroke-linejoin,stroke-miterlimit,stroke-opacity,stroke-width,text-anchor,text-rendering,vector-effect" } In the "got" string there, the (moz-prefixed) CSS variables are all listed at the end, instead of being in sorted order. The test (incorrectly) considers this to be a problem with the "SVG property list" because (I think) it assumes that everything in getComputedStyle() after "clip-path" is a SVG property. (So, these entries at the very end for CSS variables appear to be out of order SVG properties, from the test's perspective.) heycam: do you think anything is actually wrong here, or do we just need to fix the test? (Is there any way it can distinguish a CSS variable from a property-name, to detect when we hit the end of the properties in getComputedStyle()?) (Note: I'm marking this as blocking bug 763671, because currently that bug can't land without causing this test-failure.)

(In reply to Daniel Holbert [:dholbert] from comment #0) > Those CSS variable-names end up appearing in the list of "properties" that > we check for sortedness in test_bug657143.html, which triggers this > test-failure: (To be clear, the variable-names are (quoted from the failure message in comment 0): moz-form-textcolor,moz-form-border-focused,moz-form-background-disabled,moz-form-border-invalid,moz-highlight-color,moz-form-button-background,moz-range-progress-disabled,moz-form-textcolor-placeholder,moz-form-border,moz-form-border-radius,moz-form-textcolor-disabled. They happen to be moz-prefixed, but I don't think that matters for this bug -- all that matters is that (1) they're in the UA stylesheet, and (2) their names sort alphabetically before "vector-effect" (the last SVG property))

FWIW: the CSS variables themselves don't seem to be in any particular order (as exposed to the test via getComputedStyle). The first one in the test-failure message (i.e. the first one enumerated via getComputedStyle) is "moz-form-textcolor", which is in the middle of wesj's list and which (if we were sorting) would comes after the "moz-form-border*" CSS variables. (I'd initially thought that wesj might be able to work around this test-bug by naming his variables "--z-moz-[whatever]", but given that the variables get appended to the list in a random order, they'd still be unsorted amongst themselves & trigger a test-failure.) Anyway -- if we end up needing a quick hackaround, one option would be to make this test stop when it hits "vector-effect". But that's not really future-proof, since it would prevent this test from sanity-checking the sortedness of any CSS properties that we add to the bottom of our COMPUTED_STYLE_MAP_ENTRY list (which currently lives in http://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStylePropertyList.h?rev=dcbd6f52128c ).

The test needs updating to not consider those custom properties as part of the SVG properties. But there is a bug in the getComputedStyle return value too: the "--" prefix should not be chopped off from the item() return value. If we fix that, then the test can easily look for names that begin with "--", rather than "vector-effect".

The CSSOM spec doesn't actually require the custom properties to be exposed through the CSSStyleDeclaration returned by getComputedStyle, but I think it should. I've brought it up here: http://www.w3.org/mid/53D19EBD.1070000@mcc.id.au

Filed bug 1043713 for the incorrect names being exposed.

I rewrote the test a bit to be easier to follow; I found the booleans for managing the state while going along the property list a bit confusing. This relies on the bug 1043713 patch.

Although I'm actually not sure whether the UA style sheet should be exposing custom properties to authors; I've commented in bug 763671 comment 64.

Comment on attachment 8462258 [details] [diff] [review] patch r=me. Just two optional nits: >diff --git a/layout/style/test/test_bug657143.html b/layout/style/test/test_bug657143.html > // Checks ordering of CSS properties in nsComputedDOMStyle.cpp >-// by splitting the getComputedStyle() into the three sublists >+// by splitting the getComputedStyle() into four sublists > // then cloning and sort()ing the lists and comparing with originals There's a space character at the end of this comment's final line -- maybe fix (and replace with a period) while you're here. >+ok(!mozA.find(isNotPrefixed), 'Experimental -moz- CSS property list should not contain mature properties'); >+ok(!svgA.find(isPrefixed), 'Experimental -moz- CSS properties should not be in the SVG property list'); >+ok(!customA.find(isNotCustom), 'Non-custom CSS properties should not be in the custom property list'); Maybe also check for: - prefixed properties in the cssA list - custom properties in each of the non-custom lists

One thing I just thought of here: Unless we add custom properties to the UA stylesheet in e.g. bug 763671 -- and it's looking like we won't -- then we won't actually be exercising the custom-properties (css variables) part of this test. Maybe it'd be worth adding a custom property or two *in this test's own stylesheet* (e.g. in a <style> block), to make sure that part of the test is exercised and does the right thing? Otherwise, the test's assumptions about those properties are just going un-tested, until we add some css variables in a UA stylesheet, which may be a while from now (and this test's assumptions might be violated, intentionally or accidentally, in the meantime).

heycam, if you agree with comment 11, would you mind adding a custom property to this test? (either here or in a followup bug)

I don't think wesj meant to make all those new bugs block this one. (just a side effect of using "clone" on the bug that triggered this bug.)

Comment on attachment 8677828 [details] [diff] [review] test tweak followup >Bug 1043461 - Followup to ensure we still test custom property position when the UA style sheet doesn't have custom properties in it. > > >diff --git a/layout/style/test/test_bug657143.html b/layout/style/test/test_bug657143.html > <p id="display"></p> >+<style> >+:root { --test: some value; } >+</style> > <div id="content" style="display: none"> This "--test: some value" is a bit mysterious in this test right now. (I didn't understand why it was there until I read your commit message.) Maybe include a comment alongside it saying "Make sure we've got at least one custom property", or similar? r=me with that clarified - thanks!