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

Fix absolute-flow's auto positioning #12873

Merged
merged 1 commit into from Aug 15, 2016
Merged

Fix absolute-flow's auto positioning #12873

merged 1 commit into from Aug 15, 2016

Conversation

@shinglyu
Copy link
Member

shinglyu commented Aug 15, 2016 •
edited by larsbergstrom

If an absolute positioned flow has no top, bottom, left, right property, its hypothetical box position should be the margin-end of its previous sibling, not the border-end.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #12676 (github issue number if applicable).
  • There are tests for these changes

This change is Reviewable
tests/wpt/metadata-css/css21_dev/html4/left-offset-position-fixed-001.htm.ini
@@ -0,0 +1,3 @@
[left-offset-position-fixed-001.htm]

This comment has been minimized.

Sign in to view
Copy link
@shinglyu

shinglyu Aug 15, 2016

Author Member

This test case was a false positive. Both the test and ref were rendered wrongly. This patch fixed the ref, but the test part need to be fixed by #12824
@shinglyu
Copy link
Member Author

shinglyu commented Aug 15, 2016

r? @larsbergstrom
@notriddle
Copy link
Contributor

notriddle commented Aug 15, 2016

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

bors-servo commented Aug 15, 2016

📌 Commit 9752f7e has been approved by notriddle
@highfive highfive assigned notriddle and unassigned larsbergstrom Aug 15, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2016

⌛ Testing commit 9752f7e with merge f4f8b10...
bors-servo added a commit that referenced this pull request Aug 15, 2016
@bors-servo
Auto merge of #12873 - shinglyu:abs-margin-patch, r=notriddle
Loading status checks…
f4f8b10 
Fix absolute-flow's auto positioning

<!-- Please describe your changes on the following line: -->
If an absolute positioned flow has no top, bottom, left, right property, its hypothetical box position should be the margin-end of its previous sibling, not the border-end.

---
<!-- 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] These changes fix #12676 (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes

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

<!-- 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/12873)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2016

💔 Test failed - linux-rel
@highfive
Copy link

highfive commented Aug 15, 2016 

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm
</span><span class="stdout">  └   → /css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm 549dc2995e7de786feb6065fc8fc76b53746d920
/css-flexbox-1_dev/html/reference/flex-flexitem-percentage-prescation-ref.htm 516cbcf7b447cf7d5a568a4f8b57f030555e6525
Testing 549dc2995e7de786feb6065fc8fc76b53746d920 == 516cbcf7b447cf7d5a568a4f8b57f030555e6525

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flex-minimum-width-flex-items-002.htm
  └   → /css-flexbox-1_dev/html/flex-minimum-width-flex-items-002.htm db87f72145a3ccddded634bc8588902375b2937d
/css-flexbox-1_dev/html/reference/ref-filled-green-100px-square.htm d4aa213e3e31e41d0d8e84aec79e94f860f27178
Testing db87f72145a3ccddded634bc8588902375b2937d == d4aa213e3e31e41d0d8e84aec79e94f860f27178

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/perspective-origin-001.htm
  └   → /css-transforms-1_dev/html/perspective-origin-001.htm 719003ff0f2c5efe0b678a757a454870ca362e2c
/css-transforms-1_dev/html/reference/ref-filled-green-100px-square.htm d4aa213e3e31e41d0d8e84aec79e94f860f27178
Testing 719003ff0f2c5efe0b678a757a454870ca362e2c == d4aa213e3e31e41d0d8e84aec79e94f860f27178

  ▶ FAIL [expected PASS] /css21_dev/html4/run-in-listitem-between-002.htm
  └   → /css21_dev/html4/run-in-listitem-between-002.htm ef608dda75ea29fbec3a0a32f64cc80292e62d82
/css21_dev/html4/reference/run-in-text-ref.htm d068883ccfce1b3fdf24f9ea6b401c9a047b593c
Testing ef608dda75ea29fbec3a0a32f64cc80292e62d82 == d068883ccfce1b3fdf24f9ea6b401c9a047b593c

  ▶ FAIL [expected PASS] /css21_dev/html4/run-in-listitem-between-001.htm
  └   → /css21_dev/html4/run-in-listitem-between-001.htm ef608dda75ea29fbec3a0a32f64cc80292e62d82
/css21_dev/html4/reference/run-in-text-ref.htm d068883ccfce1b3fdf24f9ea6b401c9a047b593c
Testing ef608dda75ea29fbec3a0a32f64cc80292e62d82 == d068883ccfce1b3fdf24f9ea6b401c9a047b593c
@shinglyu shinglyu force-pushed the shinglyu:abs-margin-patch branch from 9752f7e to 58f0e83 Aug 15, 2016
@shinglyu
Copy link
Member Author

shinglyu commented Aug 15, 2016

I checked the 5 reftest using reftest analyzer. They should be failing all for different reasons, I mark them as FAIL again. Not sure why they passes on my computer.
@heycam
Copy link
Member

heycam commented Aug 15, 2016

@bors-servo retry
@emilio
Copy link
Member

emilio commented Aug 15, 2016

@bors-servo: r=notriddle,emilio
@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2016

📌 Commit 58f0e83 has been approved by notriddle,emilio
@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2016

⌛ Testing commit 58f0e83 with merge 3f945c9...
bors-servo added a commit that referenced this pull request Aug 15, 2016
@bors-servo
Auto merge of #12873 - shinglyu:abs-margin-patch, r=notriddle,emilio
Loading status checks…
3f945c9 
Fix absolute-flow's auto positioning

<!-- Please describe your changes on the following line: -->
If an absolute positioned flow has no top, bottom, left, right property, its hypothetical box position should be the margin-end of its previous sibling, not the border-end.

---
<!-- 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] These changes fix #12676 (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes

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

<!-- 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/12873)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2016

💔 Test failed - linux-rel
@highfive
Copy link

highfive commented Aug 15, 2016 

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/perspective-origin-001.htm
@emilio emilio closed this Aug 15, 2016
@emilio emilio reopened this Aug 15, 2016
@emilio
Copy link
Member

emilio commented Aug 15, 2016

@bors-servo: retry (#9087)
bors-servo added a commit that referenced this pull request Aug 15, 2016
@bors-servo
Auto merge of #12873 - shinglyu:abs-margin-patch, r=notriddle,emilio
Loading status checks…
198c43c 
Fix absolute-flow's auto positioning

<!-- Please describe your changes on the following line: -->
If an absolute positioned flow has no top, bottom, left, right property, its hypothetical box position should be the margin-end of its previous sibling, not the border-end.

---
<!-- 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] These changes fix #12676 (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes

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

<!-- 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/12873)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2016

⌛ Testing commit 58f0e83 with merge 198c43c...
@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2016

💔 Test failed - mac-rel-css
@highfive
Copy link

highfive commented Aug 15, 2016 

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/perspective-origin-001.htm
@shinglyu
Absolute positioned flow should be placed at the margin-end of its pr…
Loading status checks…
332cdd0 
…evious flow
@shinglyu shinglyu force-pushed the shinglyu:abs-margin-patch branch from 58f0e83 to 332cdd0 Aug 15, 2016
@shinglyu
Copy link
Member Author

shinglyu commented Aug 15, 2016

@emilio: I repeated it 20 time on my computer and it passes 20/20. So I'm marking it as expected PASS again. Looks like the first failure is an intermittent.
@notriddle
Copy link
Contributor

notriddle commented Aug 15, 2016

@bors-servo: r=notriddle,emilio
@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2016

📌 Commit 332cdd0 has been approved by notriddle,emilio
@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2016

⌛ Testing commit 332cdd0 with merge d687f77...
bors-servo added a commit that referenced this pull request Aug 15, 2016
@bors-servo
Auto merge of #12873 - shinglyu:abs-margin-patch, r=notriddle,emilio
Loading status checks…
d687f77 
Fix absolute-flow's auto positioning

<!-- Please describe your changes on the following line: -->
If an absolute positioned flow has no top, bottom, left, right property, its hypothetical box position should be the margin-end of its previous sibling, not the border-end.

---
<!-- 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] These changes fix #12676 (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes

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

<!-- 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/12873)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 15, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev
@bors-servo bors-servo merged commit 332cdd0 into servo:master Aug 15, 2016
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 mentioned this pull request Aug 17, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@notriddle notriddle

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Absolutely positioned block box are collaping margin with its previous sibling
8 participants
@shinglyu @notriddle @bors-servo @highfive @heycam @emilio @larsbergstrom @Manishearth
You can’t perform that action at this time.