Closed Bug 959973 Opened 10 years ago Closed 10 years ago

Add tests for using variables in external style sheets

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mihaelav, Assigned: mihaelav)

References

Details

Attachments

(3 files, 3 obsolete files)

We should have some reftests for using variables in external style sheets, to support the new "CSS3 variables" feature.
Attached patch tests (obsolete) — Splinter Review
Attachment #8367355 - Flags: review?(cam)
Summary: Add reftests for using variables in external style sheets → Add tests for using variables in external style sheets
Comment on attachment 8367355 [details] [diff] [review]
tests

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

Thanks for the tests.  r=me with the change below.

::: layout/style/test/test_variables.html
@@ +16,5 @@
>  p { border-left-style: inset; padding: 1px; var-decoration: line-through; }
>  </style>
>  
> +<style id="test4">
> +div { var-a: url("image.png"); background-image: var(a); }

Can you put the var-a declaration in an external style sheet?  I think we should be testing that the tokens from the external style sheet get interpreted as a url() and resolved against the base URL of the style sheet where you use it (i.e., the local style sheet here that declares background-image).
Attachment #8367355 - Flags: review?(cam) → review+
Also that external style sheet should be in the supports/ directory, so that the base URL is different.
Attached patch updated tests (obsolete) — Splinter Review
Attachment #8367355 - Attachment is obsolete: true
Attachment #8367891 - Flags: review?(cam)
Comment on attachment 8367891 [details] [diff] [review]
updated tests

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

Oh, I'm sorry.  I forgot the external variable was referenced from the mochitest rather than a reftest.  So I think it would be better to put it off a subdirectory of layout/style/test/.  You can call that directory "support" too.  r=me if you move it there, and then I don't need to see the patch again before it lands.
Attachment #8367891 - Flags: review?(cam) → review+
Attachment #8367891 - Attachment is obsolete: true
Keywords: checkin-needed
Backed out for bustage. Please make sure this at least compiles locally before requesting checkin. A green Try run would be even better.
https://hg.mozilla.org/integration/mozilla-inbound/rev/78bbd153072a

https://tbpl.mozilla.org/php/getParsedLog.php?id=33888899&tree=Mozilla-Inbound
Mihaela will you look into this?
Flags: needinfo?(mihaela.velimiroviciu)
I'm not sure what I'm supposed to do for this. I get no errors when building the mozilla-central source + my tests locally (with ./mach build).
I don't have access (yet) to build on Try.
What else should I try?
Flags: needinfo?
Flags: needinfo?(mihaela.velimiroviciu)
Flags: needinfo?
I don't think you need to do those Makefile.in changes.  Also, test_variables.html refers to external-variable-url.css, which should be supports/external-variable-url.css (it needs to be in a different directory for the test to make sense, as we're testing how URLs are resolved) and that file appears to be missing from the patch.
Attached patch updated patch (obsolete) — Splinter Review
There were a few things that were causing the test to fail:

1. The background-image property needed to be set in the document itself, with the variable set in the external style sheet.

2. Some Makefile changes were needed to cause external-variable-url.css to be copied into the right place in the test environment.  (And using MOCHITEST_FILES won't do, since that only copies files into layout/style/test/; it won't keep things in subdirectories.)

3. The <link> element needed to be inserted into the document after the pref has been set, just like all the <style> elements are populated after the pref has been set.

r?gps for the Makefile.in change.
Attachment #8383350 - Flags: review?(gps)
Comment on attachment 8383350 [details] [diff] [review]
updated patch

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

Ted is currently neck deep converting mochitests to use .ini manifests. I wouldn't be surprised if he's already bit rotted this patch.

Deferring to him for review.

Hint: He'll probably tell you to move things to support-files in an .ini file.
Attachment #8383350 - Flags: review?(gps) → review?(ted)
Comment on attachment 8383350 [details] [diff] [review]
updated patch

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

::: layout/style/test/Makefile.in
@@ +26,5 @@
>  INSTALL_TARGETS += VISITED_REFTEST
>  
> +EXTERNAL_VARIABLE_MOCHITEST_FILES = support/external-variable-url.css
> +EXTERNAL_VARIABLE_MOCHITEST_DEST = $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)/support/
> +INSTALL_TARGETS += EXTERNAL_VARIABLE_MOCHITEST

Nope. You just want to stick this in mochitest.ini nowadays:
http://mxr.mozilla.org/mozilla-central/source/layout/style/test/mochitest.ini#1

support-files =
  support/external-variable-url.css

will do what you want.
Attachment #8383350 - Flags: review?(ted) → review-
Attached patch updated patch v2Splinter Review
Ah good, thanks.
Attachment #8383350 - Attachment is obsolete: true
Attachment #8385070 - Flags: review?(ted)
Attachment #8385070 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/7a5b2bdcfacb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
This added a bunch of reftests without adding those tests to the reftest.list, which means they're not being run.  The tests should be added, right?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patchSplinter Review
Good catch.
Attachment #8393943 - Flags: review?(dbaron)
https://hg.mozilla.org/mozilla-central/rev/44bea44e2f61
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: