Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for getComputedStyle() to layout_2020 #26477

Merged
merged 1 commit into from May 11, 2020
Merged

Add support for getComputedStyle() to layout_2020 #26477

merged 1 commit into from May 11, 2020

Conversation

@mrobinson
Copy link
Member

mrobinson commented May 9, 2020 •
edited

These changes add support for getComputedStyle() to layout_2020.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes
@mrobinson mrobinson requested review from SimonSapin and emilio May 9, 2020
@highfive
Copy link

highfive commented May 9, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/node.rs, components/script/dom/cssstyledeclaration.rs
  • @jgraham: tests/wpt/include-layout-2020.ini
  • @KiChjang: components/script/dom/node.rs, components/script/dom/cssstyledeclaration.rs
  • @emilio: components/style/values/specified/box.rs
@highfive
Copy link

highfive commented May 9, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@mrobinson mrobinson changed the title Add support for `getComputedStyle` to layout_2020 and add a fast path Add support for getComputedStyle() to layout_2020 and add a fast path May 9, 2020
@mrobinson mrobinson force-pushed the mrobinson:layout-2020-get-computed-value branch from 9bd53a7 to de34be8 May 9, 2020
@emilio
emilio reviewed May 9, 2020
View changes
components/script/dom/cssstyledeclaration.rs Outdated

if !node.is_connected() {
// TODO: Node should be matched against the style rules of this window.
// Firefox is currently the only browser to implement this.

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio May 9, 2020

Member

This is no longer true, and spec was changed to reflect this. So we can remove this.

This comment has been minimized.

Sign in to view
Copy link
@mrobinson

mrobinson May 10, 2020

Author Member

Thanks for pointing that out. I've opened #26481 for this.
components/script/dom/cssstyledeclaration.rs Outdated
// we can calculate the used value without looking at layout data structures.
// Some properties resovled values compute are their computed values.
// See: https://drafts.csswg.org/cssom/#dom-window-getcomputedstyle
fn try_to_get_resolved_style(&self, node: &Node, property: &PropertyId) -> Option<String> {

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio May 9, 2020

Member

This doesn't look good if there are pending style changes. What prevents you from using an outdated style in that case?

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio May 9, 2020

Member

Answering myself: .style() / .style_for_pseudo do update style.

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio May 9, 2020

Member

Why is this optimization valuable then, though? Is the idea that it avoids going through the layout thread if there are no pending changes? If so it should be documented.
components/script/dom/cssstyledeclaration.rs Outdated
}

let positioned = style.get_box().position != Position::Static;
match longhand_id {

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio May 9, 2020

Member

It's a bit unfortunate to have this list of properties duplicated in multiple places :/
@mrobinson mrobinson force-pushed the mrobinson:layout-2020-get-computed-value branch from de34be8 to e5abe7d May 10, 2020
@mrobinson mrobinson changed the title Add support for getComputedStyle() to layout_2020 and add a fast path Add support for getComputedStyle() to layout_2020 May 10, 2020
@mrobinson
Copy link
Member Author

mrobinson commented May 10, 2020

Thank you both for the reviews. I think the second commit here (adding a fast path for getComputedSyle) was not well thought-out on my part, so I'm removing it and sticking with adding support to layout 2020. The fast path only makes sense when the node isn't dirty, but the change I proposed did an unconditional reflow (🙃 ), so it was a bit pointless in the end. I'll think about that part a bit more deeply and hopefully later come up with a better change that avoids the duplication of the list of "special" properties as well.
@jdm
Copy link
Member

jdm commented May 10, 2020

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented May 10, 2020

📌 Commit e5abe7d has been approved by jdm
@highfive highfive assigned jdm and unassigned SimonSapin May 10, 2020
@bors-servo
Copy link
Contributor

bors-servo commented May 10, 2020

⌛ Testing commit e5abe7d with merge cca83f5...
bors-servo added a commit that referenced this pull request May 10, 2020
@bors-servo
Auto merge of #26477 - mrobinson:layout-2020-get-computed-value, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
cca83f5 
Add support for getComputedStyle() to layout_2020

These changes add support for `getComputedStyle()` to layout_2020.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented May 10, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented May 11, 2020 •
edited

From the layout-2020 test results:

4 unexpected results that are NOT known-intermittents:
  â–¶ Unexpected subtest result in /_mozilla/mozilla/calc.html:
  â”” PASS [expected FAIL] calc(0ch + 0px + 0pt + 0pc + 0in + 0cm + 0mm + 0rem + 0em + 0ex + 0% + 0vw + 0vh + 0vmin + 0vmax)


  â–¶ Unexpected subtest result in /_mozilla/mozilla/localeCompare.html:
  │ FAIL [expected PASS] localeCompare should return the same as other browsers, even though it's implementation-dependent
  │   → assert_equals: expected -1 but got 1
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/localeCompare.html:8:16
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1975:25
  │ test@http://web-platform.test:8000/resources/testharness.js:535:30
  â”” @http://web-platform.test:8000/_mozilla/mozilla/localeCompare.html:7:5


  â–¶ Unexpected subtest result in /css/css-ui/box-sizing-027.html:
  │ FAIL [expected PASS] Check the resolved value of 'height' when box-sizing is  border-box.
  │   → assert_equals: expected "100px" but got "60px"
  │ 
  │ @http://web-platform.test:8000/css/css-ui/box-sizing-027.html:30:17
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1975:25
  │ test@http://web-platform.test:8000/resources/testharness.js:535:30
  â”” @http://web-platform.test:8000/css/css-ui/box-sizing-027.html:27:5

  â–¶ Unexpected subtest result in /css/css-ui/box-sizing-027.html:
  │ FAIL [expected PASS] Check the resolved value of 'width' when box-sizing is  border-box.
  │   → assert_equals: expected "100px" but got "60px"
  │ 
  │ @http://web-platform.test:8000/css/css-ui/box-sizing-027.html:25:17
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1975:25
  │ test@http://web-platform.test:8000/resources/testharness.js:535:30
  â”” @http://web-platform.test:8000/css/css-ui/box-sizing-027.html:22:5


  â–¶ Unexpected subtest result in /_mozilla/mozilla/getComputedStyle.html:
  â”” PASS [expected FAIL] Element's resolved values
@mrobinson mrobinson force-pushed the mrobinson:layout-2020-get-computed-value branch from e5abe7d to 39a9322 May 11, 2020
@mrobinson mrobinson mentioned this pull request May 11, 2020
Merged
4 of 4 tasks complete
@jdm
Copy link
Member

jdm commented May 11, 2020

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2020

📌 Commit 39a9322 has been approved by jdm
bors-servo added a commit that referenced this pull request May 11, 2020
@bors-servo
Auto merge of #26477 - mrobinson:layout-2020-get-computed-value, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
70d1f6a 
Add support for getComputedStyle() to layout_2020

These changes add support for `getComputedStyle()` to layout_2020.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2020

⌛ Testing commit 39a9322 with merge 70d1f6a...
@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2020

💔 Test failed - status-taskcluster
@jdm
jdm reviewed May 11, 2020
View changes
tests/wpt/mozilla/meta-layout-2020/mozilla/localeCompare.html.ini Outdated
@@ -1,4 +1,2 @@
[localeCompare.html]
bug: https://github.com/servo/servo/issues/25802
[localeCompare should return the same as other browsers, even though it's implementation-dependent]
expected: FAIL

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm May 11, 2020

Member

You will need to revert the changes to this file.
@mrobinson
layout_2020: Add initial support for getComputedStyle
Loading status checks…
9c7b1ae 
This implementation is more-or-less on par with the one from layout_2013
and in some cases better. There are still some cases where we don't
return the correct "resolved value," but this is enough to test
animations and transitions.
@mrobinson mrobinson force-pushed the mrobinson:layout-2020-get-computed-value branch from 39a9322 to 9c7b1ae May 11, 2020
@mrobinson
Copy link
Member Author

mrobinson commented May 11, 2020

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2020

📌 Commit 9c7b1ae has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2020

⌛ Testing commit 9c7b1ae with merge ff71ab0...
bors-servo added a commit that referenced this pull request May 11, 2020
@bors-servo
Auto merge of #26477 - mrobinson:layout-2020-get-computed-value, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
ff71ab0 
Add support for getComputedStyle() to layout_2020

These changes add support for `getComputedStyle()` to layout_2020.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2020

💔 Test failed - status-taskcluster
@mrobinson
Copy link
Member Author

mrobinson commented May 11, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2020

⌛ Testing commit 9c7b1ae with merge aa9f16c...
@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing aa9f16c to master...
@bors-servo bors-servo merged commit aa9f16c into servo:master May 11, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request May 11, 2020
Merged
@mrobinson mrobinson deleted the mrobinson:layout-2020-get-computed-value branch May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

@emilio emilio

@SimonSapin SimonSapin

Assignees

@jdm jdm

Labels
A-layout/2020
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@mrobinson @highfive @jdm @bors-servo @emilio @SimonSapin
You can’t perform that action at this time.