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

IE11 compatibility #1

Closed
dozed opened this issue Feb 9, 2017 · 17 comments
Closed

IE11 compatibility #1

dozed opened this issue Feb 9, 2017 · 17 comments
Labels

Comments

@dozed
Copy link

dozed commented Feb 9, 2017

The library is currently not compatible with IE11.

There are two issues:

  • postMessage supports only string payload
  • getOriginFromUrl seems broken, e.g. protocol and port was missing

Possible URI fix var childOrigin = new URI(window.location.href).path(url).origin(); with https://medialize.github.io/URI.js/

A way for postMessage IE compatibility:

let Compat = {

  init: () => {
    var ieVersion = Compat.detectIE();
    Compat.stringifyPostMessages = (ieVersion !== false && Compat.detectIE() <= 11);
  },

  detectIE: () => {
    var ua = window.navigator.userAgent;

    var msie = ua.indexOf('MSIE ');
    if (msie > 0) {
      // IE 10 or older => return version number
      return parseInt(ua.substring(msie + 5, ua.indexOf('.', msie)), 10);
    }

    var trident = ua.indexOf('Trident/');
    if (trident > 0) {
      // IE 11 => return version number
      var rv = ua.indexOf('rv:');
      return parseInt(ua.substring(rv + 3, ua.indexOf('.', rv)), 10);
    }

    var edge = ua.indexOf('Edge/');
    if (edge > 0) {
      // Edge (IE 12+) => return version number
      return parseInt(ua.substring(edge + 5, ua.indexOf('.', edge)), 10);
    }

    // other browser
    return false;
  },

  postMessage: (target, data, remoteOrigin) => {

    if (Compat.stringifyPostMessages) {
      return Compat.postMessageCompat(target, data, remoteOrigin);
    } else {
      target.postMessage(data, remoteOrigin);
    }

  },

  addMessageListener: (target, listener) => {
    if (Compat.stringifyPostMessages) {
      return Compat.addMessageListenerCompat(target, listener);
    } else {
      target.addEventListener(MESSAGE, listener, false);

      return {
        remove: () => {
          target.removeEventListener(MESSAGE, listener);
        }
      };
    }
  },

  postMessageCompat: (target, data, remoteOrigin) => {

    let str = JSON.stringify(data);
    target.postMessage(str, remoteOrigin);

  },

  addMessageListenerCompat: (target, listener) => {

    let wrapped = (e) => {

      if (e.data && e.data.startsWith("{")) {
        let data = JSON.parse(e.data);
        let ec = {
          data: data,
          source: e.source,
          origin: e.origin
        };
        listener(ec);
      } else {
        listener(e);
      }

    };

    target.addEventListener(MESSAGE, wrapped);

    return {
      remove: () => {
        target.removeEventListener(MESSAGE, wrapped);
      }
    };

  }

};

Compat.init();
@Aaronius
Copy link
Owner

Aaronius commented Feb 9, 2017

Thanks for reporting! I wasn't aware of these issues. I'll try to get them fixed as soon as possible.

@Aaronius
Copy link
Owner

@dozed Are you sure you aren't running IE in compatibility mode? I'm pretty sure IE9 and older required a string payload but I think everything after that supports it. My tests pass on IE11 and I can't reproduce the problems you're seeing. Can you provide me with a test or some specific code that is failing? Thanks.

@dozed
Copy link
Author

dozed commented Feb 13, 2017

You are right, IE10 and IE11 allow non-string payloads. So only IE9 and below are affected. Do you want to support those?

The issue with the URL appears in IE10/11 as well as in Edge, event.origin === childOrigin fails. Here is a test case: http://output.jsbin.com/yebucecibo Would it make sense to discuss this in a separate ticket?

Source jsbin:

@Aaronius
Copy link
Owner

I don't intend to support IE9 and below, but I do need to document what the supported browsers are. Thanks so much for creating the example jsbin. I'll take a closer look when I get a moment. No need to create a new ticket.

@dozed
Copy link
Author

dozed commented Feb 13, 2017

When providing url: http://output.jsbin.com/vovudekosi the childOrigin is http://output.jsbin.com/vovudekosi:80.

@Aaronius
Copy link
Owner

@dozed I think you're referring to childOrigin at the end of these lines:

var child = iframe.contentWindow || iframe.contentDocument.parentWindow;
var childOrigin = getOriginFromUrl(url);

If I put console.log(childOrigin); after those lines and run it, I see this in the browser console:

http://output.jsbin.com:80

Which is accurate. You should be able to see that here: http://jsbin.com/wenosinoxe/1/edit?html,js,output

Do you see something different?

@dozed
Copy link
Author

dozed commented Feb 14, 2017

Can you check if the handshake succeeds or if you can invoke a method on the parent? Also check the value of event.origin.

@Aaronius
Copy link
Owner

It doesn't succeed, but I wouldn't expect it to in the context of jsbin. The URL http://output.jsbin.com/vovudekosi, which is what the parent is trying to load, redirects to http://jsbin.com/vovudekosi/1/edit?html,js,output, which is jsbin itself and not the html page that has the connectToParent call. I think you'd have to pull this code out of jsbin to successfully test the full flow.

@dozed
Copy link
Author

dozed commented Feb 16, 2017

This is since the jsbin is expired. You would need to setup a new jsbin with the code to reproduce the issue with the URL.

@Aaronius
Copy link
Owner

Aaronius commented Feb 20, 2017

Okay, I think I have this figured out and fixed in 2.4.1. You can see my changes here: f04c490

Basically, in browsers where we have to compute the childOrigin, we need to not add the port to the computed string if the port is the default for the protocol. Because we were always adding the port, it wasn't matching the origin reported by the browser on message events.

The approach I've taken appears to match what's done in the whatwg-url library here.

You might be able to view the working jsbin here: http://output.jsbin.com/leqovofite

Thanks again for reporting this issue and writing up the jsbin. Please let me know if you see any problems.

@dozed
Copy link
Author

dozed commented Feb 20, 2017

On Microsoft Edge a.port || location.port is the empty string. So it does not work there.

@Aaronius
Copy link
Owner

:/

@Aaronius Aaronius reopened this Feb 21, 2017
@Aaronius
Copy link
Owner

Fixed in 2.4.3. Currently viewable at: http://output.jsbin.com/jucovogugi/1

I tested in IE 10 and 11, Edge, and latest versions of Chrome, Firefox, and Safari. My automated tests don't use the default ports or it would have caught this. I don't really want to use the default ports though because it may not catch the problems with custom ports. I might need to run them on both default and custom ports. :(

Thanks for testing this for me.

@dozed
Copy link
Author

dozed commented Feb 23, 2017

Cool, thanks for fixing.

@Aaronius
Copy link
Owner

@dozed Out of curiosity, are you actively using Penpal or just checking it out? If you're using it, how is it working out for you?

@dozed
Copy link
Author

dozed commented Feb 23, 2017

I use it actively in a private project, it is working fine so far. I like the simple functional approach.

@Aaronius
Copy link
Owner

Good to hear. Thanks!

@Aaronius Aaronius added the bug label Feb 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants