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

Z-index should be ignored for non-positioned stacking contexts #8023

Merged
merged 1 commit into from Oct 16, 2015

Conversation

@mrobinson
Copy link
Member

mrobinson commented Oct 14, 2015

When a stacking-context is not positioned, its z-index should be
ignored. This is per CSS 2 9.9.1. The only exception to this is when
the z-index is applied to an element with display: flex | inline-flex.
inline-flex does not appear to be implemented at this time so we only
do this for flex.

Review on Reviewable

@mrobinson
Copy link
Member Author

mrobinson commented Oct 14, 2015

@pcwalton
Copy link
Contributor

pcwalton commented Oct 14, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 14, 2015

📌 Commit 9da8265 has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Oct 15, 2015

Testing commit 9da8265 with merge e7d2dfa...

bors-servo pushed a commit that referenced this pull request Oct 15, 2015
Z-index should be ignored for non-positioned stacking contexts

When a stacking-context is not positioned, its z-index should be
ignored. This is per CSS 2 9.9.1. The only exception to this is when
the z-index is applied to an element with display: flex | inline-flex.
inline-flex does not appear to be implemented at this time so we only
do this for flex.

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

bors-servo commented Oct 15, 2015

💔 Test failed - mac-dev-ref-unit

@mrobinson
Copy link
Member Author

mrobinson commented Oct 15, 2015

@mrobinson mrobinson force-pushed the mrobinson:zindex branch from 9da8265 to f186957 Oct 15, 2015
@mrobinson
Copy link
Member Author

mrobinson commented Oct 15, 2015

I accidentally uploaded an old version of the PR. I have canceled the previous bors test run and have uploaded the complete version. @pcwalton, would you mind taking a quick look at this again? Very sorry for the confusion.

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 15, 2015

@bors-servo r-

Stop adding new tests to test-ref.

@jdm
Copy link
Member

jdm commented Oct 15, 2015

I feel like adding test coverage to existing old-style reftests is a grey area, since the changes will be ported along with the rest of the test.

@mrobinson
Copy link
Member Author

mrobinson commented Oct 15, 2015

@Ms2ger Sorry. I wasn't sure exactly what to do in this in-between time. I'm happy to use the WPT infrastructure if that is better. Do you have any guidance for how I can add tests for this issue?

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 15, 2015

Oh, misread the diff. I guess this is fine for now.

@mrobinson
Copy link
Member Author

mrobinson commented Oct 15, 2015

@Ms2ger and @jdm thanks! I'm totally willing to help out with the migration too.

@samlh
Copy link
Contributor

samlh commented Oct 15, 2015

The reftest doesn't seem to match the reference in Firefox. Am I missing something?

@mrobinson
Copy link
Member Author

mrobinson commented Oct 15, 2015

@samlh There is an unrelated bug with the placement of absolute blocks which may be causing this. I'll take a look.

@mrobinson
Copy link
Member Author

mrobinson commented Oct 15, 2015

@samlh This difference isn't due to the positioning bug. I'm tracking down the issue now. Thanks for letting me know.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2015

The latest upstream changes (presumably #8045) made this pull request unmergeable. Please resolve the merge conflicts.

@mrobinson
Copy link
Member Author

mrobinson commented Oct 16, 2015

@samlh The issue here seems to be that Blink and WebKit don't consider elements with transforms to be "positioned" for the purposes of the CSS 2.1 z-index specification, but Gecko does.

When a stacking-context is not positioned, its z-index should be
ignored. This is per CSS 2 9.9.1. The only exception to this is when
the z-index is applied to an element with display: flex | inline-flex.
inline-flex does not appear to be implemented at this time so we only
do this for flex.
@mrobinson mrobinson force-pushed the mrobinson:zindex branch from f186957 to f2a66af Oct 16, 2015
@mrobinson
Copy link
Member Author

mrobinson commented Oct 16, 2015

I think that, in the end, the Blink/WebKit behavior is incorrect. I notice that they seem to include blocks with transformations into the list of positioned content for stacking purposes, but ignore the z-index value. Given that, I'm going to follow the Firefox behavior here. I've updated the patch to not ignore z-index for blocks with transformations and also to integrate the test with the newly wpt-ized version.

@pcwalton
Copy link
Contributor

pcwalton commented Oct 16, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2015

📌 Commit f2a66af has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Oct 16, 2015

Testing commit f2a66af with merge 90dd3cd...

bors-servo pushed a commit that referenced this pull request Oct 16, 2015
Z-index should be ignored for non-positioned stacking contexts

When a stacking-context is not positioned, its z-index should be
ignored. This is per CSS 2 9.9.1. The only exception to this is when
the z-index is applied to an element with display: flex | inline-flex.
inline-flex does not appear to be implemented at this time so we only
do this for flex.

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

bors-servo commented Oct 16, 2015

@bors-servo bors-servo merged commit f2a66af into servo:master Oct 16, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@mrobinson mrobinson deleted the mrobinson:zindex branch Oct 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.