Avoid `Window::GetComputedStyle` when checking for `display: none` #20012

alexfjw · 2018-02-10T08:48:04Z

Refactored Window::GetComputedStyle to use Element::Style.

Not sure which tests are relevant, but I've ran the dom, fetch & 2dcontext wpt tests. They don't seem to give errors.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Avoid Window::GetComputedStyle when checking for display: none #19885.

These changes do not require tests because it's a refactoring task

This change is

highfive · 2018-02-10T08:48:10Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

highfive · 2018-02-10T08:48:10Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/script_thread.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/element.rs
@fitzgen: components/script/script_thread.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/element.rs
@KiChjang: components/script/script_thread.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script/dom/element.rs

highfive · 2018-02-10T08:48:12Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

CYBAI · 2018-02-10T10:58:16Z

components/script/dom/canvasrenderingcontext2d.rs


                    if element_not_rendered {
                        Ok(RGBA::new(0, 0, 0, 255))
                    } else {
-                        self.parse_color(&style.GetPropertyValue(DOMString::from("color")))
+                        Ok(style.unwrap().get_color().color)
                    }


I'm wondering will it be better to use pattern matching for style here?

Ex.

let style = canvas.upcast::<Element>().style(); match (style) { Some(s) if canvas.upcast::<Node>().is_in_doc() => Ok(s.get_color().color), _ => Ok(RGBA::new(0, 0, 0, 255)) }

I was pondering on that as well but couldn't think of a good way to write it. Thanks for the suggestion, I'll try it out.

jdm · 2018-02-10T13:52:42Z

@bors-servo try

bors-servo · 2018-02-10T13:52:47Z

⌛ Trying commit 32fae57 with merge ff97245...

…cking_for_display-none, r=<try> Avoid `Window::GetComputedStyle` when checking for `display: none`  Refactored Window::GetComputedStyle to use Element::Style. Not sure which tests are relevant, but I've ran the dom, fetch & 2dcontext wpt tests. They don't seem to give errors. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19885.  - [x] These changes do not require tests because it's a refactoring task    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20012)

bors-servo · 2018-02-10T14:27:09Z

💔 Test failed - linux-rel-wpt

alexfjw · 2018-02-10T14:31:33Z

Apologies, looks like I should have ran through all the css tests. I'll get the code fixed.

emilio

Looks great with that tiny bit fixed, thanks!

emilio · 2018-02-12T11:34:58Z

components/script/dom/canvasrenderingcontext2d.rs

-                    } else {
-                        self.parse_color(&style.GetPropertyValue(DOMString::from("color")))
+                    match style {
+                        Some(ref s) if canvas.upcast::<Node>().is_in_doc() => Ok(s.get_color().color),


This also needs to check whether s.get_box().display is not Display::None.

Also, you should be able to assert that is_in_doc() if the style is Some(..). But checking it explicitly doesn't hurt I guess.

Thanks emilio! I realised about s.get_box().display from the error I made previously. Was wondering about is_in_doc() as well but wasn't too sure & decided to spend some time reading up.

emilio · 2018-02-12T13:19:41Z

components/script/dom/canvasrenderingcontext2d.rs

-                    } else {
-                        self.parse_color(&style.GetPropertyValue(DOMString::from("color")))
+                    match canvas_element.style() {
+                        Some(ref s) if canvas_element.has_css_layout_box() => Ok(s.get_color().color),


The has_css_layout_box check whole new style query, which is somewhat unfortunate, but can be fixed in a followup I guess.

Thanks a lot!

Man, didn't realize that style queries were expensive. Thanks for the help as well! I've learnt quite a bit from this PR.

emilio · 2018-02-12T13:20:57Z

@bors-servo r+

bors-servo · 2018-02-12T13:20:58Z

📌 Commit c448e09 has been approved by emilio

bors-servo · 2018-02-12T13:45:54Z

⌛ Testing commit c448e09 with merge aa59ad9...

…cking_for_display-none, r=emilio Avoid `Window::GetComputedStyle` when checking for `display: none`  Refactored Window::GetComputedStyle to use Element::Style. Not sure which tests are relevant, but I've ran the dom, fetch & 2dcontext wpt tests. They don't seem to give errors. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19885.  - [x] These changes do not require tests because it's a refactoring task    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20012)

bors-servo · 2018-02-12T13:46:49Z

💔 Test failed - windows-msvc-dev

emilio · 2018-02-12T14:13:38Z

@bors-servo retry

jdm · 2018-02-12T14:35:06Z

@bors-servo r-

  ▶ TIMEOUT [expected OK] /_mozilla/mozilla/transitionend_safety.html
  │ 
  │ VMware, Inc.
  │ softpipe
  └ 3.3 (Core Profile) Mesa 17.3.0-devel

  ▶ Unexpected subtest result in /_mozilla/mozilla/transitionend_safety.html:
  │ TIMEOUT [expected PASS] Nodes cannot be GCed while a CSS transition is in effect.
  └   → Test timed out

These tests still fail consistently.

alexfjw · 2018-02-13T04:34:15Z

@emilio
I'm sorry but I'm not really sure why the test fails. Hope you could help me out.
It looks to me like checking Window.GetComputedStyle(element).Display() == 'none' is the same as checking !element.has_css_layout_box(). However, they return different values in the failing test, the former being false and latter true.

The wiki's article on layout styling mentions Window.getComputedStyle() method sometimes returns used values and sometimes computed values, but I'm not sure if that point is relevant here.

//Edit, missed out a !

emilio · 2018-02-13T15:51:35Z

I think that test is wrong, and I suspect it'll fail on other browsers. has_css_layout_box is definitely not the same as GetComputedStyle().Display() == "none". For example, if the node is outside of the document, the element would not have a layout box, regardless of the computed value of the Display property. Similarly if the element is under a display: none parent, it'll have no layout box, even though its display will not be none.

Not firing transitionend for an element outside of the document seems what other browsers do and what the spec mandates, too. @jdm would you be fine with removing that test-case, or maybe moving the step_func_done to a timeout? The test looks invalid to me.

jdm · 2018-02-14T15:44:00Z

A timeout seems like the best choice to me, rather than losing test coverage of a safety issue.

emilio · 2018-02-24T19:36:45Z

Hi @alexfjw are you planning to finish this up? It should be a matter of marking the test as an expected timeout. Let me know if you have any question on how to do that, happy to help!

alexfjw · 2018-02-25T01:18:44Z

Apologies, was a little busy the past week. Yep, I'll try to finish it up soon

emilio · 2018-02-25T02:10:37Z

No problem, thank you very much!

alexfjw · 2018-02-26T09:54:03Z

@emilio, would something like this be fine? Or did you mean updating the test expectation to accept a timeout?

emilio

I meant to just let the harness expect a timeout, but this wfm too. Thanks!

emilio · 2018-02-26T10:32:35Z

Looks like you need to update the manifest:

diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json
index 892d423d8..2cd4e0050 100644
--- a/tests/wpt/mozilla/meta/MANIFEST.json
+++ b/tests/wpt/mozilla/meta/MANIFEST.json
@@ -70817,7 +70817,7 @@
    "testharness"
   ],
   "mozilla/transitionend_safety.html": [
-   "778e43b049aa421bad7f86eb03d0955576a84ce0",
+   "16d238e94b2cc2843c9aee4210db43d496bb3114",
    "testharness"
   ],
   "mozilla/union.html": [

emilio · 2018-02-27T09:52:01Z

@bors-servo r+

Thanks!

bors-servo · 2018-02-27T09:52:01Z

📌 Commit 8d09398 has been approved by emilio

bors-servo · 2018-02-27T09:52:08Z

⌛ Testing commit 8d09398 with merge ac162e8...

…cking_for_display-none, r=emilio Avoid `Window::GetComputedStyle` when checking for `display: none`  Refactored Window::GetComputedStyle to use Element::Style. Not sure which tests are relevant, but I've ran the dom, fetch & 2dcontext wpt tests. They don't seem to give errors. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19885.  - [x] These changes do not require tests because it's a refactoring task    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20012)

bors-servo · 2018-02-27T10:47:25Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing ac162e8 to master...

highfive assigned metajack Feb 10, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 10, 2018

CYBAI reviewed Feb 10, 2018

View reviewed changes

alexfjw changed the title ~~Avoid Window::GetComputedStyle when checking for display: none~~ [WIP] Avoid Window::GetComputedStyle when checking for display: none Feb 10, 2018

alexfjw changed the title ~~[WIP] Avoid Window::GetComputedStyle when checking for display: none~~ Avoid Window::GetComputedStyle when checking for display: none Feb 10, 2018

highfive added the S-tests-failed The changes caused existing tests to fail. label Feb 10, 2018

alexfjw changed the title ~~Avoid Window::GetComputedStyle when checking for display: none~~ [WIP] Avoid Window::GetComputedStyle when checking for display: none Feb 10, 2018

highfive removed the S-tests-failed The changes caused existing tests to fail. label Feb 11, 2018

emilio approved these changes Feb 12, 2018

View reviewed changes

alexfjw changed the title ~~[WIP] Avoid Window::GetComputedStyle when checking for display: none~~ Avoid Window::GetComputedStyle when checking for display: none Feb 12, 2018

emilio approved these changes Feb 12, 2018

View reviewed changes

highfive assigned emilio and unassigned metajack Feb 12, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 12, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Feb 12, 2018

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Feb 26, 2018

emilio approved these changes Feb 26, 2018

View reviewed changes

Avoid Window::GetComputedStyle when checking for display: none

8d09398

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 27, 2018

bors-servo merged commit 8d09398 into servo:master Feb 27, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid `Window::GetComputedStyle` when checking for `display: none` #20012

Avoid `Window::GetComputedStyle` when checking for `display: none` #20012

alexfjw commented Feb 10, 2018 •

edited

highfive commented Feb 10, 2018

highfive commented Feb 10, 2018

highfive commented Feb 10, 2018

CYBAI Feb 10, 2018 •

edited

alexfjw Feb 10, 2018

jdm commented Feb 10, 2018

bors-servo commented Feb 10, 2018

bors-servo commented Feb 10, 2018

alexfjw commented Feb 10, 2018

emilio left a comment

emilio Feb 12, 2018

emilio Feb 12, 2018

alexfjw Feb 12, 2018

emilio Feb 12, 2018

alexfjw Feb 12, 2018

emilio commented Feb 12, 2018

bors-servo commented Feb 12, 2018

bors-servo commented Feb 12, 2018

bors-servo commented Feb 12, 2018

emilio commented Feb 12, 2018

jdm commented Feb 12, 2018

alexfjw commented Feb 13, 2018 •

edited

emilio commented Feb 13, 2018

jdm commented Feb 14, 2018

emilio commented Feb 24, 2018

alexfjw commented Feb 25, 2018

emilio commented Feb 25, 2018

alexfjw commented Feb 26, 2018

emilio left a comment

emilio commented Feb 26, 2018

emilio commented Feb 27, 2018

bors-servo commented Feb 27, 2018

bors-servo commented Feb 27, 2018

bors-servo commented Feb 27, 2018

Avoid Window::GetComputedStyle when checking for display: none #20012

Avoid Window::GetComputedStyle when checking for display: none #20012

Conversation

alexfjw commented Feb 10, 2018 • edited

highfive commented Feb 10, 2018

highfive commented Feb 10, 2018

highfive commented Feb 10, 2018

CYBAI Feb 10, 2018 • edited

Choose a reason for hiding this comment

alexfjw Feb 10, 2018

Choose a reason for hiding this comment

jdm commented Feb 10, 2018

bors-servo commented Feb 10, 2018

bors-servo commented Feb 10, 2018

alexfjw commented Feb 10, 2018

emilio left a comment

Choose a reason for hiding this comment

emilio Feb 12, 2018

Choose a reason for hiding this comment

emilio Feb 12, 2018

Choose a reason for hiding this comment

alexfjw Feb 12, 2018

Choose a reason for hiding this comment

emilio Feb 12, 2018

Choose a reason for hiding this comment

alexfjw Feb 12, 2018

Choose a reason for hiding this comment

emilio commented Feb 12, 2018

bors-servo commented Feb 12, 2018

bors-servo commented Feb 12, 2018

bors-servo commented Feb 12, 2018

emilio commented Feb 12, 2018

jdm commented Feb 12, 2018

alexfjw commented Feb 13, 2018 • edited

emilio commented Feb 13, 2018

jdm commented Feb 14, 2018

emilio commented Feb 24, 2018

alexfjw commented Feb 25, 2018

emilio commented Feb 25, 2018

alexfjw commented Feb 26, 2018

emilio left a comment

Choose a reason for hiding this comment

emilio commented Feb 26, 2018

emilio commented Feb 27, 2018

bors-servo commented Feb 27, 2018

bors-servo commented Feb 27, 2018

bors-servo commented Feb 27, 2018

Avoid `Window::GetComputedStyle` when checking for `display: none` #20012

Avoid `Window::GetComputedStyle` when checking for `display: none` #20012

alexfjw commented Feb 10, 2018 •

edited

CYBAI Feb 10, 2018 •

edited

alexfjw commented Feb 13, 2018 •

edited