Skip to content

Core: Remove IE-specific support tests, rely on document.documentMode #4387

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.

Sign up for GitHub

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
Merged
mgol merged 1 commit into from
May 13, 2019
Merged

Core: Remove IE-specific support tests, rely on document.documentMode #4387

mgol merged 1 commit into from
May 13, 2019

Conversation

@mgol
Copy link
Member

@mgol mgol commented May 4, 2019 •
edited
Loading

Summary

To make sure none of the IE-targeted workarounds depends on IE bugs we also
have a new special test run on Chrome Headless with document.documentMode
set to 11 to force all those workarounds to be applied in Chrome & make sure
tests still pass.

Edit:

  1. After recent changes we're back at -412 bytes
  2. Also, update some tests to IE-sniff when deciding whether to skip a test.

Fixes gh-4386

Checklist

@mgol mgol added the Core label May 4, 2019
@mgol mgol mentioned this pull request May 4, 2019
Closed
mgol
mgol commented May 4, 2019
View reviewed changes
mgol
mgol commented May 4, 2019
View reviewed changes
test/unit/fake-document-mode.js Outdated
@@ -0,0 +1,7 @@
QUnit.module( "fake-document-mode", { afterEach: moduleTeardown } );
Copy link
Member Author

@mgol mgol May 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't added this test to the list where all others are specified as it only makes sense in the mode with faked documentMode.

mgol
mgol commented May 5, 2019
View reviewed changes
@mgol mgol force-pushed the ie-support branch 2 times, most recently from 213f86f to c8c152b Compare May 5, 2019 12:50
@mgol
Copy link
Member Author

mgol commented May 5, 2019

It turns out my workaround for Chrome with documentMode emulation failing the "SVG dimensions (border-box)" test was incorrect; IE doesn't support offset* properties on SVGs either. The workaround really depends on IE incorrectly reporting content-box dimensions for border-box elements. I reverted this change (we're down to -415 bytes), you can see the failure on Travis now.

There are two ways we can work around this:

  1. We accept that in this case we depend on the bug existing in IE and on it not getting fixed there. That means the special "Chrome with documentMode emulation" test run doesn't make sense anymore.
  2. We restore the boxSizingReliable test but guard it in the additional documentMode check to avoid triggering layout outside of IE. This will reduce size gains from -415 bytes to -201 bytes, although I suspect this can be reduced by removing some now redundant CSS from the support test.

Note that in the past we've been bitten by an assumption that IE won't fix a bug when IE had a bug with dimensions in fullscreen mode inside iframes (see #1764) and we applied a workaround that multiplied the dimensions by 100 based on the documentMode check which we had to revert later (see #3041) when IE fixed it contrary to earlier claims. The probability of IE fixing something now is perhaps lower than 3 years ago, especially when it comes to something more complex than this simple iframe sizing bug but there's still some risk.

@dmethvin
Copy link
Member

dmethvin commented May 5, 2019 •
edited
Loading

I prefer the first option. It seems like the "fullscreen in iframes" bug was trivial to fix but something tells me that IE11 isn't going to fix its unreliable box sizing ever. If that seems like too much risk we could just guard it with documentMode to avoid forcing layout anywhere but IE. The sad thing about that solution is that IE11 will probably suffer the most performance hit from forcing layout.

@gibson042
Copy link
Member

gibson042 commented May 6, 2019

I also prefer the first option. As I mentioned in #4386 (comment) , if anyone ever does release an update or fork of IE that fixes the issue, we can re-add support tests then.

@mgol
Copy link
Member Author

mgol commented May 6, 2019

PR updated and is ready for final review.

I removed the special Chrome with fake docMode run, I also modified some tests to IE-sniff when deciding whether to skip instead of performing sometimes complex (& easier to do wrong) support tests where possible.

gibson042
gibson042 approved these changes May 6, 2019
View reviewed changes
@mgol mgol mentioned this pull request May 6, 2019
Merged
2 tasks
@mgol
Copy link
Member Author

mgol commented May 6, 2019

@gibson042 PR updated (changes are in the new fixup commit). I also added a support comment for e0d9411 that was missing.

dmethvin
dmethvin approved these changes May 6, 2019
View reviewed changes
Copy link
Member

@dmethvin dmethvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@mgol
Core: Remove IE-specific support tests, rely on document.documentMode
4ca19f8 
Also, update some tests to IE-sniff when deciding whether
to skip a test.

Fixes jquerygh-4386
@timmywil timmywil added this to the 4.0.0 milestone May 13, 2019
@mgol mgol merged commit 3527a38 into jquery:master May 13, 2019
@mgol mgol deleted the ie-support branch May 13, 2019 19:40
@mgol mgol mentioned this pull request May 19, 2019
Open
4 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Nov 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dmethvin dmethvin dmethvin approved these changes

@gibson042 gibson042 gibson042 approved these changes

Assignees

No one assigned

Labels

Core

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

Rely on documentMode for most IE workarounds?

4 participants

@mgol @dmethvin @gibson042 @timmywil