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

Compute value of float according to position value #8111

Merged
merged 2 commits into from Nov 3, 2015
Merged

Compute value of float according to position value #8111

merged 2 commits into from Nov 3, 2015

Conversation

@gilles-leblanc
Copy link
Contributor

gilles-leblanc commented Oct 21, 2015

According to CSS2 Section 9.7, if 'position' has a value of 'absolute'
or 'fixed' the computed value of 'float' should be 'none'.

This changes the float to a single_keyword_computed which checks the
positioned value of the element to compute the float value.

Fixes #8002

Review on Reviewable
@jdm
Copy link
Member

jdm commented Oct 21, 2015

There's a moratorium on creating new old-style reftests. Instead, you can use ./mach create-wpt --reftest tests/wpt/mozilla/tests/css/new_test.html --reference tests/wpt/mozilla/tests/css/new_test_reference.html to automatically create a new-style one. However, I think this would be clearer as a regular web-platform-test instead (./mach create-wpt tests/wpt/mozilla/tests/new_test.html) which would just call assert_equals on the result of getComputedStyle.
@gilles-leblanc gilles-leblanc force-pushed the gilles-leblanc:issue-8002 branch from 7d602dd to bd471a7 Oct 22, 2015
@gilles-leblanc
Copy link
Contributor Author

gilles-leblanc commented Oct 22, 2015

I updated the pull request.
Since the review hadn't begun proper I did a force push with the new version. I replaced the test witth a web-platform-test instead.
tests/wpt/mozilla/tests/css/float_relative_to_position.html Outdated
var computed_style = getComputedStyle(document.querySelector("div"))["float"];
assert_equals(computed_style, "none");

document.querySelector("div").setAttribute("position", "absolute");

This comment has been minimized.

Sign in to view
Copy link
@eefriedman

eefriedman Oct 22, 2015

Contributor

The position attribute on <div> has no meaning. Did you mean to set the style attribute?

The fact that you could make this mistake and have the test still pass indicates that it's not ideal... maybe add a stylesheet with a style like div { float: left }, have the test check that float is left, then change the position using the style attribute.
@gilles-leblanc
Copy link
Contributor Author

gilles-leblanc commented Oct 23, 2015

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved, some commit checks pending.

tests/wpt/mozilla/tests/css/float_relative_to_position.html, line 17 [r1] (raw file):
Good point. I updated the pull request with a new commit which addresses this issue.

Comments from the review on Reviewable.io
@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2015

☔ The latest upstream changes (presumably #8152) made this pull request unmergeable. Please resolve the merge conflicts.
@gilles-leblanc
Copy link
Contributor Author

gilles-leblanc commented Oct 23, 2015

@jdm and @eefriedman Would you rather I rebase now (because of the changes mentioned by bors) and loose history in the review or would you rather I wait until the review is finished?
@jdm
Copy link
Member

jdm commented Oct 23, 2015

Rebasing doesn't lose history. Reviewable is very permissive about these things.
@jdm
Copy link
Member

jdm commented Oct 23, 2015

r? @SimonSapin
@gilles-leblanc
Compute value of float according to position value
725484d 
According to CSS2 Section 9.7, if 'position' has a value of 'absolute'
or 'fixed' the computed value of 'float' should be 'none'.

This changes the float to a single_keyword_computed which checks the
positioned value of the element to compute the float value.

Fixes #8002
@gilles-leblanc
Modify wpt test following review comment
Loading status checks…
595f11d
@gilles-leblanc gilles-leblanc force-pushed the gilles-leblanc:issue-8002 branch from 87f8f71 to 595f11d Oct 24, 2015
@gilles-leblanc
Copy link
Contributor Author

gilles-leblanc commented Oct 27, 2015

I know you guys are busy and this isn't urgent but I'm just pinging this to make it doesn't fall into the cracks :)
@SimonSapin
Copy link
Member

SimonSapin commented Nov 3, 2015

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

bors-servo commented Nov 3, 2015

📌 Commit 595f11d has been approved by SimonSapin
@SimonSapin
Copy link
Member

SimonSapin commented Nov 3, 2015

(Sorry about the delay! I was away the last two weeks.)
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

⌛ Testing commit 595f11d with merge 97389d7...
bors-servo added a commit that referenced this pull request Nov 3, 2015
@bors-servo
Auto merge of #8111 - gilles-leblanc:issue-8002, r=SimonSapin
97389d7 
Compute value of float according to position value

According to CSS2 Section 9.7, if 'position' has a value of 'absolute'
or 'fixed' the computed value of 'float' should be 'none'.

This changes the float to a single_keyword_computed which checks the
positioned value of the element to compute the float value.

Fixes #8002

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8111)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

💔 Test failed - linux-rel
@jdm
Copy link
Member

jdm commented Nov 3, 2015

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

bors-servo commented Nov 3, 2015

⚡ Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-wpt are reusable. Rebuilding only linux-rel, mac-rel-css...
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

💔 Test failed - linux-rel
@gilles-leblanc
Copy link
Contributor Author

gilles-leblanc commented Nov 3, 2015

Since it failed twice, I'll run it locally to check the result.
@SimonSapin
Copy link
Member

SimonSapin commented Nov 3, 2015

@bors-servo retry

@gilles-leblanc, the failures are on different tests, both known to fail intermittently.
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

⌛ Testing commit 595f11d with merge 5070c87...
bors-servo added a commit that referenced this pull request Nov 3, 2015
@bors-servo
Auto merge of #8111 - gilles-leblanc:issue-8002, r=SimonSapin
Loading status checks…
5070c87 
Compute value of float according to position value

According to CSS2 Section 9.7, if 'position' has a value of 'absolute'
or 'fixed' the computed value of 'float' should be 'none'.

This changes the float to a single_keyword_computed which checks the
positioned value of the element to compute the float value.

Fixes #8002

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8111)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 595f11d into servo:master Nov 3, 2015
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable Review in progress: 0 of 3 files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SimonSapin SimonSapin

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Computed style for "float" doesn't respect position or display
7 participants
@gilles-leblanc @jdm @bors-servo @SimonSapin @eefriedman @nox @highfive
You can’t perform that action at this time.