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

Polyfill for mediaQuery breaks in IE11 #11358

Closed
4 tasks done
kb3eua opened this issue Jun 27, 2018 · 11 comments · Fixed by #11393
Closed
4 tasks done

Polyfill for mediaQuery breaks in IE11 #11358

kb3eua opened this issue Jun 27, 2018 · 11 comments · Fixed by #11393

Comments

@kb3eua
Copy link

kb3eua commented Jun 27, 2018

What should happen?

Foundation should load all of its JavaScript (in this case, trying to initialize an accordion) in IE11 without throwing any exceptions.

What happens instead?

In IE11, get error SCRIPT65535: Invalid Calling object in the console window and the accordion does not work.

Possible Solution

The cause seems to be the polyfill for window.matchMedia. There is a local variable matchMedia, to which window.matchMedia is being assigned, and calling this as a function in foundation.util.mediaQuery.js is what seems to trigger the error.

The preferable solution would be to remove the polyfill from foundation altogether (just use the native window.matchMedia), and leave a note in the setup guide specifying that for older browsers a third-party polyfill will be required (https://github.com/paulirish/matchMedia.js)... Foundation should not be baking in its own polyfills.

Steps to Reproduce

How to reproduce:

  1. Define markup for an accordion.
  2. Call $(document).foundation()
  3. Navigate to site in IE11
  4. Observe that clicking on the accordion does not cause it to expand or collapse, and there is an error in the console.

Context

I'm just trying to use the Accordion plugin. It works in Chrome, but not IE11 b/c of the aforementioned polyfill.

Your Environment

  • Foundation version(s) used: 6.4.3
  • Browser(s) name and version(s): Internet Explorer 11
  • Operating System and version (desktop or mobile): Windows 10 Enterprise (desktop)

Checklist (all required):

  • I have read and follow the CONTRIBUTING.md document.
  • There are no other issues similar to this one.
  • The issue title is descriptive.
  • The template is correctly filled.
@DanielRuf
Copy link
Contributor

Never had this issue in current projects which use accordions.

Can you test Foundation Sites 6.5.0 RC1?

@DanielRuf
Copy link
Contributor

Could reproduce it with a project here.
It seems atLeast() is the cause, at least here. Can you test if is() works?

And can you share the used markup and code?

@DanielRuf
Copy link
Contributor

Probably related: tus/tus-js-client#77 (comment)

@DanielRuf
Copy link
Contributor

@kb3eua
Copy link
Author

kb3eua commented Jul 5, 2018

My UI/UX team member decided to go with an alternate solution, so I'm afraid I didn't save the code or markup changes and have since moved on to other things. But I can tell you that I used webpack to bundle the code (as did the person who reported the issue in your aforementioned "probably related" link... perhaps that's the common denominator?), and for me the error bubbled up from within the _getCurrentSize() function. And the markup I used was just copied & pasted from the first sample here.

@DanielRuf
Copy link
Contributor

We had a similar issue and could also trace it back to the _getCurrentSize() function. The issue is that the matchMedia is not mapped back to the window.matchMedia property which is likely causing an error in IE due to strict mode.

patch-package
--- a/node_modules/foundation-sites/js/foundation.util.mediaQuery.js
+++ b/node_modules/foundation-sites/js/foundation.util.mediaQuery.js
@@ -63,6 +63,8 @@ let matchMedia = window.matchMedia || (function() {
}
})();

+window.matchMedia = matchMedia;
+
var MediaQuery = {
queries: [],

@@ -109,7 +111,7 @@ var MediaQuery = {
var query = this.get(size);

if (query) {
- return matchMedia(query).matches;
+ return window.matchMedia(query).matches;
}

return false;
@@ -160,7 +162,7 @@ var MediaQuery = {
for (var i = 0; i < this.queries.length; i++) {
var query = this.queries[i];

- if (matchMedia(query.value).matches) {
+ if (window.matchMedia(query.value).matches) {
matched = query;
}
}

@DanielRuf
Copy link
Contributor

@DanielRuf
Copy link
Contributor

I also think we should also use polyfills from polyfill.io by the Financial Times

https://github.com/Financial-Times/polyfill-service

@kb3eua
Copy link
Author

kb3eua commented Jul 5, 2018

@DanielRuf I don't think you want to lock users into using any particular polyfill solution. I'd rather Foundation not use any polyfills at all because some people may only be targeting evergreen browsers (in which case a polyfill only adds unnecessarily to the download size), and not everyone is going to want to create an external dependency on the polyfill.io site (some may prefer to bundle the polyfill with their own code). It should be documented that Foundation has a dependency on this or other newer features of JavaScript/ECMAScript which are only supported in newer browsers, and you can suggest 3rd-party polyfill solutions but it should ultimately be up to the developer as to what polyfill, if any, they want to use.

@DanielRuf
Copy link
Contributor

on the polyfill.io site

Please check the repository and see package.json, this is on npm.

@DanielRuf
Copy link
Contributor

DanielRuf commented Jul 5, 2018

Well, currently we internally use a matchMedia polyfill (which has bugs) and is a thirdparty code snippet.

ncoden added a commit that referenced this issue Jul 11, 2018
…ow-fix-ie11

fix: set matchMedia on window to fix issue on IE11, fixes #11358
ncoden added a commit to ncoden/foundation-sites that referenced this issue Aug 25, 2018
…ill-window-fix-ie11 for v6.5.0

e7554d6 fix: set matchMedia on window to fix issue on IE11, fixes foundation#11358
1a36a44 chore: update MatchMedia polyfill to v0.3.1

Co-Authored-By: Nicolas Coden <nicolas@ncoden.fr>
Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
@ncoden ncoden mentioned this issue Sep 10, 2018
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants