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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: mihaelav, Assigned: mihaelav)
References
Details
Attachments
(3 files, 3 obsolete files)
9.92 KB,
patch
|
Details | Diff | Splinter Review | |
8.75 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
2.42 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
We should have some reftests for using variables in external style sheets, to support the new "CSS3 variables" feature.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8367355 -
Flags: review?(cam)
Assignee | ||
Updated•10 years ago
|
Summary: Add reftests for using variables in external style sheets → Add tests for using variables in external style sheets
Comment 2•10 years ago
|
||
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+
Comment 3•10 years ago
|
||
Also that external style sheet should be in the supports/ directory, so that the base URL is different.
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8367355 -
Attachment is obsolete: true
Attachment #8367891 -
Flags: review?(cam)
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8367891 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9d3acbb028
Flags: in-testsuite+
Keywords: checkin-needed
Comment 8•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(mihaela.velimiroviciu)
Flags: needinfo?
Comment 11•10 years ago
|
||
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.
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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 14•10 years ago
|
||
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-
Comment 15•10 years ago
|
||
Ah good, thanks.
Attachment #8383350 -
Attachment is obsolete: true
Attachment #8385070 -
Flags: review?(ted)
Updated•10 years ago
|
Attachment #8385070 -
Flags: review?(ted) → review+
Comment 17•10 years ago
|
||
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 → ---
Comment 20•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=dc537b6434b8
Attachment #8393943 -
Flags: review?(dbaron) → review+
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/44bea44e2f61
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•