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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix preserve3d's IE11/Win10 false positive #1920

Merged
merged 3 commits into from Aug 4, 2016
Merged

Fix preserve3d's IE11/Win10 false positive #1920

merged 3 commits into from Aug 4, 2016

Conversation

@paulirish
Copy link
Member

@paulirish paulirish commented Mar 9, 2016

This is @aFarkas's patch. I'm just helping 馃樅

fixes #1748

@paulirish paulirish force-pushed the paulirish-patch-1 branch from ba8226c to 92aed6c Mar 10, 2016
@paulirish
Copy link
Member Author

@paulirish paulirish commented Mar 10, 2016

test page: https://output.jsbin.com/bajubol/1/quiet

win7 ie11 - should be false
image

win10 ie11 - should be true
image

lookin good.

@ryanseddon
Copy link
Member

@ryanseddon ryanseddon commented Mar 10, 2016

Isn't this the preserve3d test looks false on both in your screenshot still?

@paulirish
Copy link
Member Author

@paulirish paulirish commented Mar 10, 2016

Isn't this the preserve3d test looks false on both in your screenshot still?

it does appear that is true... :/

@ryanseddon
Copy link
Member

@ryanseddon ryanseddon commented Mar 10, 2016

@aFarkas is this test missing anything? Seem to be still getting a false negative in win10?

@aFarkas
Copy link
Member

@aFarkas aFarkas commented Mar 11, 2016

Not sure, but I think your expectations are wrong. IE11 should always return false (in both cases).

The problem was, that IE11 on Win10 returned true although it should return false. Now it returns always false.

This is also what the title says.

@paulirish Thank you very much to add the modified test (it's so important to avoid layout in modern browsers.)

@ryanseddon
Copy link
Member

@ryanseddon ryanseddon commented Aug 3, 2016

so if we rebase or merge in master this should be god to go right @paulirish @aFarkas?

@paulirish
Copy link
Member Author

@paulirish paulirish commented Aug 3, 2016

Rebased and updated. @aFarkas can you review my logic flip in f9ddfa2 ?

@aFarkas
Copy link
Member

@aFarkas aFarkas commented Aug 4, 2016

LGTM

@ryanseddon
Copy link
Member

@ryanseddon ryanseddon commented Aug 4, 2016

Build failing was a linting issue merged to master so this is good to go, merging.

@ryanseddon ryanseddon merged commit 2a7ddcb into master Aug 4, 2016
0 of 4 checks passed
0 of 4 checks passed
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
@ryanseddon ryanseddon deleted the paulirish-patch-1 branch Aug 4, 2016
@paulirish
Copy link
Member Author

@paulirish paulirish commented Aug 4, 2016

Ace. Thanks for the followup, @ryanseddon !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants