CSS Variables in UA stylesheet trigger "TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug657143.html | SVG property list should be alphabetical"

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dholbert, Assigned: heycam)

Tracking

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.)
(Reporter)

Updated

5 years ago
Summary: CSS Variables with "moz" prefix trigger "TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug657143.html | SVG property list should be alphabetical" → CSS Variables in UA stylesheet trigger "TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug657143.html | SVG property list should be alphabetical"
(Reporter)

Updated

5 years ago
Flags: needinfo?(cam)
(Reporter)

Comment 1

5 years ago
(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))
(Reporter)

Comment 2

5 years ago
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".
Flags: needinfo?(cam)
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.
Depends on: 1043713
Posted patch patchSplinter Review
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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8462258 - Flags: review?(dholbert)
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.
(Reporter)

Comment 8

5 years ago
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
Attachment #8462258 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/4cc4fdbabae3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(Reporter)

Comment 11

5 years ago
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).
(Reporter)

Comment 12

5 years ago
heycam, if you agree with comment 11, would you mind adding a custom property to this test? (either here or in a followup bug)
Flags: needinfo?(cam)
QA Whiteboard: [qa-]
Blocks: 1062978
Blocks: 1062980
Blocks: 1062985
Blocks: 1062987
(Reporter)

Comment 13

5 years ago
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.)
No longer blocks: 1062978, 1062980, 1062985, 1062987
Flags: needinfo?(cam)
Attachment #8677828 - Flags: review?(dholbert)
(Reporter)

Comment 15

4 years ago
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!
Attachment #8677828 - Flags: review?(dholbert) → review+
You need to log in before you can comment on or make changes to this bug.