Make Stylist::set_device check stylesheet media queries #14556

ghost · 2016-12-12T20:42:03Z

Fixes Stylist::set_device to check for media queries in stylesheets.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14279 (github issue number if applicable).

There are tests for these changes

This change is

highfive · 2016-12-12T20:42:07Z

Heads up! This PR modifies the following files:

@bholley: components/style/stylist.rs
@emilio: components/style/stylist.rs

highfive · 2016-12-12T20:42:08Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.

emilio · 2016-12-14T07:14:01Z

tests/wpt/mozilla/tests/css/stylesheet_media_queries.html

+<title></title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<iframe id="myframe" src="iframe_for_media_queries.html" height="500px" width="500px" onLoad="func();">


Func may not exist if the iframe loads too fast. It's unlikely, but please move the definition of func above.

emilio · 2016-12-14T07:14:01Z

tests/wpt/mozilla/tests/css/iframe_for_media_queries.html

+    </style>
+    </head>
+    <body>
+        <h1 id="head" >Header</h1>


nit: No space after id="head". also, another id could be better.

emilio · 2016-12-14T07:14:01Z

tests/wpt/mozilla/tests/css/stylesheet_media_queries.html

+    var head = document.getElementById("myframe").contentWindow.document.getElementById("head");
+    var before = getComputedStyle(head).getPropertyValue("background-color");
+    document.getElementById("myframe").width = "300";
+    setTimeout(function () {


Use requestAnimationFrame, and iframeTest.step_func.

emilio · 2016-12-14T07:14:01Z

components/style/stylist.rs

@@ -389,6 +389,7 @@ impl Stylist {
            false
        }
        self.is_device_dirty |= stylesheets.iter().any(|stylesheet| {
+            stylesheet.media.read().evaluate(&self.device) ||


This check is wrong, we need to check whether the media query changed, not only if it evaluates to true. Otherwise we might mark the device dirty when we shouldn't.

emilio · 2016-12-14T07:27:10Z

r? @SimonSapin

SimonSapin · 2016-12-23T16:22:42Z

Reviewed 1 of 3 files at r1, 3 of 3 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

tests/wpt/mozilla/tests/css/iframe_for_media_queries.html, line 10 at r3 (raw file):

    <style media="(max-width: 399px)">
    h1 {
        background: red;

Please change the colors so that red indicates failure, and green for success.

tests/wpt/mozilla/tests/css/stylesheet_media_queries.html, line 16 at r3 (raw file):

    setTimeout(iframeTest.step_func_done(function () {
        var after = getComputedStyle(testElement).getPropertyValue("background-color");
        assert_true(before != after, "Recomputed values");

Check both values with assert_equals, not just that they’re different.

Comments from Reviewable

SimonSapin · 2016-12-23T16:24:47Z

Although github shows it as "outdated", it looks like this comment from @emilio is still relevant:

Use requestAnimationFrame, and iframeTest.step_func.

Though to be honest I’m not sure how to use these. Can you give more details @emilio ?

emilio · 2016-12-23T16:38:31Z

Sure. I think we should get rid of setTimeout, and ideally requestAnimationFrame too.

@iamrohit7 does the test written this way work?

<!doctype html>
<meta charset="utf-8">
<title>CSS Test: Media query correctly forces style invalidation</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<iframe id="myframe" src="iframe_for_media_queries.html" height="500" width="500">
</iframe>
<script>
var test = async_test("Media queries within stylesheets");
window.onload = test.step_func_done(function() {
  var frame = document.getElementById("myframe");
  var frameDoc = frame.contentWindow.document;
  var element = frameDoc.getElementById("testelement");
  assert_equals(getComputedStyle(element).backgroundColor, "xxx");
  frame.width = "300";
  frameDoc.documentElement.offsetWidth; // Force layout
  assert_equals(getComputedStyle(element).backgroundColor, "yyy");
})
</script>

If it doesn't you may need to replace that test.step_func_done for test.step_func, and assert_equals(getComputedStyle(element).backgroundColor, "yyy"); for:

requestAnimationFrame(test.step_func_done(function() {
   assert_equals(getComputedStyle(element).backgroundColor, "yyy");
}))

(Obviously replacing xxx and yyy for the proper values).

ghost · 2016-12-24T02:37:29Z

@emilio I tried forcing layout by accessing the offset as you suggested but it didn't work. The property value isn't being updated to the new value(though it changes visually). I still needed to setTimeout before checking for the second value. I should've commented so, sorry.

emilio · 2016-12-24T13:32:32Z

@iamrohit7 In that case, I think using requestAnimationFrame should be enough, and is less flaky that setTimeout, have you tried with that?

ghost · 2016-12-24T15:16:53Z

@emilio Do you mean something like this?

var test = async_test("Media queries within stylesheets");
window.onload = test.step_func(function() {
  var frame = document.getElementById("myframe");
  var frameDoc = frame.contentWindow.document;
  var element = frameDoc.getElementById("testelement");
  assert_equals(getComputedStyle(element).backgroundColor, "rgb(255, 0, 0)");
  frame.width = "300";
  frameDoc.documentElement.offsetWidth; // Force layout
  window.requestAnimationFrame(test.step_func_done(function () {
	  assert_equals(getComputedStyle(element).backgroundColor, "rgb(0, 255, 0)");
  }));
});

The second assert still fails.

bzbarsky · 2016-12-24T20:14:40Z

I tried forcing layout by accessing the offset as you suggested but it didn't work.

That sounds like a servo bug. Please let's just fix that instead of adding flakiness to web platform tests for everyone to work around it.

bzbarsky · 2016-12-24T20:17:27Z

There's one other bug in the test: it's using an element from one document, but a getComputedStyle call on a window from another document. The behavior of that is somewhere between "not specified very well and not very interoperable" and "lol". You really want to use frame.contentWindow.getComputedStyle(element).

emilio · 2016-12-24T21:29:56Z

On Sat, Dec 24, 2016 at 12:14:47PM -0800, Boris Zbarsky wrote: That sounds like a servo bug. Please let's just fix that instead of adding flakiness to web platform tests for everyone to work around it.

Yes, but it's not even a single bug I think. I think there are at least two different bugs here, that due to Servo's architecture aren't easy to fix, and I think they should be fixed in different PRs. The first one is, as you noted, that changing the layout in a same-origin iframe needs to flush all the same-origin parent chain, and that's something Servo doesn't do (and isn't trivial because in Servo layout is separated per _document_, not per origin). I think we should change that and I'll fill a bug about it. Also, I think there's a bug in our setter for the iframe width, which doesn't mark the content document as needing layout, which is also wrong. I'll dig a bit more (probably tomorrow), and file issues/fix if I have time. In any case, I think we should land the proper test for this, and marking it as failing/intermittent with a link to the proper issues.

bors-servo · 2017-01-18T19:00:19Z

☔ The latest upstream changes (presumably #14839) made this pull request unmergeable. Please resolve the merge conflicts.

emilio · 2017-01-28T15:07:16Z

@bors-servo r=SimonSapin

bors-servo · 2017-01-28T15:07:16Z

📌 Commit cda2995 has been approved by SimonSapin

bors-servo · 2017-01-28T15:14:25Z

⌛ Testing commit cda2995 with merge d551a36...

bors-servo · 2017-01-28T15:28:21Z

💔 Test failed - linux-dev

emilio · 2017-01-28T15:40:16Z

Tidy is failing, I think the way to update it is ./mach update-wpt, but I'm not totally sure:

./tests/wpt/mozilla/meta/MANIFEST.json:6795: tab on line

./tests/wpt/mozilla/meta/MANIFEST.json:6796: tab on line

jdm · 2017-01-28T16:52:57Z

./mach manifest-update

ghost · 2017-01-29T03:21:26Z

@bors-servo retry

emilio · 2017-01-29T08:21:02Z

Travis seems still unhappy, please run the manifest-update command (or ./mach test-wpt --manifest-update --binary= SKIP_TESTS).

emilio · 2017-01-29T08:23:10Z

$ bash etc/ci/manifest_changed.sh
diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json
index a6092c071..372a9f46a 100644
--- a/tests/wpt/mozilla/meta/MANIFEST.json
+++ b/tests/wpt/mozilla/meta/MANIFEST.json
@@ -6788,18 +6788,18 @@
             "url": "/_mozilla/css/offset_properties_inline.html"
	                }
			         ],
				 -        "css/test_font_family_parsing.html": [
				 -          {
				 -            "path": "css/test_font_family_parsing.html",
				 -            "url": "/_mozilla/css/test_font_family_parsing.html"
				 -          }
				 -        ],
				          "css/stylesheet_media_queries.html": [
					             {
						                  "path": "css/stylesheet_media_queries.html",
								               "url": "/_mozilla/css/stylesheet_media_queries.html"
									                  }
											           ],
												   +        "css/test_font_family_parsing.html": [
												   +          {
												   +            "path": "css/test_font_family_parsing.html",
												   +            "url": "/_mozilla/css/test_font_family_parsing.html"
												   +          }
												   +        ],
												            "css/test_variable_legal_values.html": [
													               {
														                    "path": "css/test_variable_legal_values.html",
																    The command "bash etc/ci/manifest_changed.sh" exited with 1.


        Adds test for checking media queries in stylesheets


        Changes Stylist::set_device to check for mediaqueries in Stylesheets

ghost · 2017-01-29T11:43:51Z

@emilio Done :)

emilio · 2017-01-29T12:06:00Z

@bors-servo r=SimonSapin,Emilio

Thanks Rohit! Sorry this took so long to get landed.

bors-servo · 2017-01-29T12:06:00Z

📌 Commit 325271e has been approved by SimonSapin,Emilio

bors-servo · 2017-01-29T12:06:04Z

⌛ Testing commit 325271e with merge 1c1aaa5...


        Auto merge of #14556 - iamrohit7:fix-set-device, r=SimonSapin,Emilio

Make Stylist::set_device check stylesheet media queries Fixes Stylist::set_device to check for media queries in stylesheets.  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14279 (github issue number if applicable).  - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14556)

bors-servo · 2017-01-29T13:00:30Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned nox Dec 12, 2016

highfive added the S-awaiting-review label Dec 12, 2016

emilio requested changes Dec 14, 2016

View changes

highfive assigned SimonSapin and unassigned nox Dec 14, 2016

SimonSapin changed the title ~~Fix set device~~ Make Stylist::set_device check stylesheet media queries Dec 14, 2016

SimonSapin added S-needs-code-changes and removed S-awaiting-review labels Dec 14, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 22, 2016

SimonSapin added S-needs-code-changes and removed S-awaiting-review labels Dec 23, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 24, 2016

emilio mentioned this pull request Dec 24, 2016

Our resize story for iframes is not great. #14719

Open

highfive added the S-needs-rebase label Jan 18, 2017

jdm added the S-needs-code-changes label Jan 19, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jan 28, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 28, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Jan 29, 2017

Rohit Burra added 2 commits Dec 12, 2016

Adds test for checking media queries in stylesheets

c1eb6ae

Changes Stylist::set_device to check for mediaqueries in Stylesheets

Loading status checks…

325271e

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 29, 2017

bors-servo merged commit 325271e into servo:master Jan 29, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 29, 2017

ghost deleted the fix-set-device branch Jan 29, 2017

servo / servo

Make Stylist::set_device check stylesheet media queries #14556

Make Stylist::set_device check stylesheet media queries #14556

ghost commented Dec 12, 2016 •

edited by ghost

highfive commented Dec 12, 2016

highfive commented Dec 12, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Dec 14, 2016

SimonSapin commented Dec 23, 2016

SimonSapin commented Dec 23, 2016

emilio commented Dec 23, 2016

ghost commented Dec 24, 2016 •

edited by ghost

emilio commented Dec 24, 2016

ghost commented Dec 24, 2016

bzbarsky commented Dec 24, 2016

bzbarsky commented Dec 24, 2016

emilio commented Dec 24, 2016

bors-servo commented Jan 18, 2017

emilio commented Jan 28, 2017

bors-servo commented Jan 28, 2017

bors-servo commented Jan 28, 2017

bors-servo commented Jan 28, 2017

emilio commented Jan 28, 2017

jdm commented Jan 28, 2017 •

edited

ghost commented Jan 29, 2017

emilio commented Jan 29, 2017

emilio commented Jan 29, 2017

ghost commented Jan 29, 2017

emilio commented Jan 29, 2017

bors-servo commented Jan 29, 2017

bors-servo commented Jan 29, 2017

bors-servo commented Jan 29, 2017

servo / servo

Join GitHub today

Make Stylist::set_device check stylesheet media queries #14556

Make Stylist::set_device check stylesheet media queries #14556

Conversation

ghost commented Dec 12, 2016 • edited by ghost

highfive commented Dec 12, 2016

highfive commented Dec 12, 2016

This comment has been minimized.

emilio Dec 14, 2016

This comment has been minimized.

emilio Dec 14, 2016

This comment has been minimized.

emilio Dec 14, 2016

This comment has been minimized.

emilio Dec 14, 2016

emilio commented Dec 14, 2016

SimonSapin commented Dec 23, 2016

SimonSapin commented Dec 23, 2016

emilio commented Dec 23, 2016

ghost commented Dec 24, 2016 • edited by ghost

emilio commented Dec 24, 2016

ghost commented Dec 24, 2016

bzbarsky commented Dec 24, 2016

bzbarsky commented Dec 24, 2016

emilio commented Dec 24, 2016

bors-servo commented Jan 18, 2017

emilio commented Jan 28, 2017

bors-servo commented Jan 28, 2017

bors-servo commented Jan 28, 2017

bors-servo commented Jan 28, 2017

emilio commented Jan 28, 2017

jdm commented Jan 28, 2017 • edited

ghost commented Jan 29, 2017

emilio commented Jan 29, 2017

emilio commented Jan 29, 2017

ghost commented Jan 29, 2017

emilio commented Jan 29, 2017

bors-servo commented Jan 29, 2017

bors-servo commented Jan 29, 2017

bors-servo commented Jan 29, 2017

ghost commented Dec 12, 2016 •

edited by ghost

ghost commented Dec 24, 2016 •

edited by ghost

jdm commented Jan 28, 2017 •

edited