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

Jump to bottom

Absolutely positioned block box are collaping margin with its previous sibling #12676

Closed
shinglyu opened this issue Aug 1, 2016 · 5 comments
Closed

Absolutely positioned block box are collaping margin with its previous sibling #12676

shinglyu opened this issue Aug 1, 2016 · 5 comments
Labels
A-layout/block C-has-manual-testcase

Comments

@shinglyu
Copy link
Member

@shinglyu shinglyu commented Aug 1, 2016

According to the spec, a block element with position:absolute and NO top, bottom, left, right defined should NOT collapse with any margin.

Though absolutely positioned boxes may have margins, those margins do not collapse with any other margins.

But the following test case shows that that red box is collapsing with the #margin-box's bottom margin.

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
<html>
    <head>
        <style type="text/css">
            #margin-box {
                background-color:blue;
                width: 100px;
                height: 10px;
                margin-bottom: 50px;
            }

            #reference-overlapped-red {
                position: absolute;
                background-color: red;
                width: 100px;
                height: 100px;
                z-index:-1;
            }

            #test-overlapping-green {
                background-color: green;
                width: 100px;
                height: 100px;
            }
        </style>
    </head>
    <body>
        <div id="margin-box"></div>
        <div id="reference-overlapped-red"></div>
        <div id="test-overlapping-green"></div>
    </body>
</html>

Screenshot: (left=servo, right=gecko)
selection_015
@shinglyu
Copy link
Member Author

@shinglyu shinglyu commented Aug 1, 2016

This blocks #12610 in some sense because all the tests/wpt/css-tests/css-flexbox-1_dev/html/flex-minimum-width-flex-items-00*.html tests is broken because of this.
@shinglyu shinglyu changed the title Absolutely positioned block box are collaping it top margin Absolutely positioned block box are collaping margin with its previous sibling Aug 1, 2016
@stshine stshine mentioned this issue Aug 3, 2016
Merged
3 of 3 tasks complete
@shinglyu
Copy link
Member Author

@shinglyu shinglyu commented Aug 4, 2016

I've already identify the problem, but my naive patch caused some regression. Need to Double check with the spec to fix the regression.
@shinglyu
Copy link
Member Author

@shinglyu shinglyu commented Aug 10, 2016

The problem is this comment in block.rs:

                    // Assume that the *hypothetical box* for an absolute flow starts immediately
                    // after the block-end border edge of the previous flow.

The hypothetical box has to consider the previous flow's bottom margin if it has no top, bottom, left, right property.
@shinglyu
Copy link
Member Author

@shinglyu shinglyu commented Aug 10, 2016

I think I fixed it. My patch fixed 99 reftests but made one fail: /css21_dev/html4/left-offset-position-fixed-001.htm.

I found out that the test was a false positive, both the test and reference are rendered incorrectly, so the test passes, not the reference are fixed by the patch, but the test file failed for other reason. I'll check weather the bug is related, otherwise I should land my patch and create another bug for the left-offset-position-fixed-001.htm case

This is a proof that the reference file was rendered incorrectly:
selection_027
@shinglyu
Copy link
Member Author

@shinglyu shinglyu commented Aug 12, 2016

The regressed test case was caused by #12824 , which is not really related to this, so I'll merge this first.
This was referenced Aug 15, 2016
Open
Merged
bors-servo added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-layout/block C-has-manual-testcase
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Fix absolute-flow's auto positioning
2 participants
@jdm @shinglyu
You can’t perform that action at this time.