Skip to content
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

style: Adjust float if the element is positioned per CSS 2.1 section 9.7 #15730

Merged
merged 2 commits into from Feb 25, 2017

Conversation

@emilio
Copy link
Member

commented Feb 24, 2017

We've found crashes related to this in Gecko.

r? @SimonSapin or @heycam


This change is Reviewable

We've found crashes related to this in Gecko.
@highfive

This comment has been minimized.

Copy link

commented Feb 24, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/properties.mako.rs
@highfive

This comment has been minimized.

Copy link

commented Feb 24, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@emilio

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2017

@bors-servo try

  • I hope we have tests for this.
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

⌛️ Trying commit e67f5c7 with merge 9598f23...

bors-servo added a commit that referenced this pull request Feb 24, 2017
style: Adjust float if the element is positioned per CSS 2.1 section 9.7

We've found crashes related to this in Gecko.

r? @SimonSapin or @heycam
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

💔 Test failed - linux-rel-wpt

@stshine

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

⌛️ Trying commit e67f5c7 with merge 642908b...

bors-servo added a commit that referenced this pull request Feb 25, 2017
style: Adjust float if the element is positioned per CSS 2.1 section 9.7

We've found crashes related to this in Gecko.

r? @SimonSapin or @heycam

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15730)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

💔 Test failed - linux-rel-wpt

@emilio

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2017

So we don't seem to, given those failures seem intermittent.

I'll write a wpt test. Also, I guess I'll look at any code that could deal with this in layout to simplify it.

@emilio emilio force-pushed the emilio:adjust-float branch from 84c9c9c to d051612 Feb 25, 2017
@emilio

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2017

Seems like the layout code that deals with this (in components/layout/construct.rs) ignores the float value for absolutely positioned elements already, so I don't think I can simplify it further.

This should be ready for review.

@emilio

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

⌛️ Trying commit d051612 with merge 051a727...

bors-servo added a commit that referenced this pull request Feb 25, 2017
style: Adjust float if the element is positioned per CSS 2.1 section 9.7

We've found crashes related to this in Gecko.

r? @SimonSapin or @heycam

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15730)
<!-- Reviewable:end -->
@SimonSapin

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

@bors-servo r+


Reviewed 1 of 1 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

📌 Commit d051612 has been approved by SimonSapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

💔 Test failed - mac-dev-unit

@SimonSapin

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

bash ./etc/ci/manifest_changed.sh
diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json
index 63e821e..9ecaa48 100644
--- a/tests/wpt/mozilla/meta/MANIFEST.json
+++ b/tests/wpt/mozilla/meta/MANIFEST.json
@@ -20902,7 +20902,7 @@
    "support"
   ],
   "css/float-abspos.html": [
-   "d5e1649c88102f27da5fe1ac16715cfee7f70f84",
+   "d34a9ce0be290bc3c46aa80eb136a91460957346",
    "testharness"
   ],
   "css/float_clearance_a.html": [
program finished with exit code 1
…ioned.
@emilio emilio force-pushed the emilio:adjust-float branch from d051612 to 19e4f72 Feb 25, 2017
@emilio

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2017

@bors-servo r=SimonSapin

Arg, forgot that I can't do last-minute fixups to the tests without updating the manifest, sigh.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

📌 Commit 19e4f72 has been approved by SimonSapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

⌛️ Testing commit 19e4f72 with merge eb28153...

bors-servo added a commit that referenced this pull request Feb 25, 2017
style: Adjust float if the element is positioned per CSS 2.1 section 9.7

We've found crashes related to this in Gecko.

r? @SimonSapin or @heycam

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15730)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

💔 Test failed - arm32

@emilio

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2017

Fun:

IOError: CRC check failed 0xc78a87b != 0x61f360b5L

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

⚡️ Previous build results for android, mac-dev-unit are reusable. Rebuilding only arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2017

☀️ 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
Approved by: SimonSapin
Pushing eb28153 to master...

@bors-servo bors-servo merged commit 19e4f72 into servo:master Feb 25, 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
@emilio emilio deleted the emilio:adjust-float branch Feb 25, 2017
@bzbarsky

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

@emilio I assume we don't need mOriginalFloat bits because these structs will never land in nsRuleNode::ComputeDisplayData or nsStyleDisplay::CalcDifference?

@emilio

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@bzbarsky yeah, that was my analysis (arguably post-landing, I just realised we had that bit in Gecko) at https://bugzilla.mozilla.org/show_bug.cgi?id=1342738#c3

@heycam

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

I assume we don't need mOriginalFloat bits because these structs will never land in nsRuleNode::ComputeDisplayData or nsStyleDisplay::CalcDifference?

The structs will land in CalcDifference, but we only compare mOriginalFloat to return a NeutralChange hint, which stylo doesn't need to care about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.