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.

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

Improves cross-browser compatibility #275

Closed
wants to merge 2 commits into from
Closed

Improves cross-browser compatibility #275

wants to merge 2 commits into from

Conversation

amarvin
Copy link

@amarvin amarvin commented Jun 21, 2018 •
edited
Loading

Adds meta tags recommended by Bootstrap 3.3 basic template for cross-browser compatibility. Relates to #170

@amarvin
Improves cross-browser compatibility
b317a28 
Adds meta tags recommended by Bootstrap 3.3 basic template for cross-browser compatibility
@ngnpope
Copy link
Contributor

ngnpope commented Jun 25, 2018

Would it be better to use those recommended for Bootstrap v4.1?

https://getbootstrap.com/docs/4.1/getting-started/introduction/#starter-template

@amarvin
Copy link
Author

amarvin commented Jun 25, 2018

I think the Bootstrap 4 template doesn't support older browsers, and assumes HTML5 compatibility. I was hoping that Dash would support old browsers like IE11.

@chriddyp
Copy link
Member

Here's a reference on this tag: https://stackoverflow.com/a/6771584/4142536

I still need to take some time to thoroughly think this one through (e.g. make sure it's the right move for Dash in the future, make sure that it won't break anything)

@ngnpope
Copy link
Contributor

ngnpope commented Jun 25, 2018

I was hoping that Dash would support old browsers like IE11.

IE11 does support HTML5. Bootstrap 4 support IE10+...

https://getbootstrap.com/docs/4.1/getting-started/browsers-devices/#supported-browsers

@amarvin
Copy link
Author

amarvin commented Jun 25, 2018

Ah good catch @pope1ni. I guess the real reason that I need the X-UA-Compatible tag in IE11 is that my Dash apps are hosted on the Intranet, and IE11 displays Intranet pages in compatibility mode by default (unless overridden by X-UA-Compatible. Not sure how common this specific problem is, but I bet a lot of Dash users want to build internal apps on their company's Intranet.

@ngnpope
Copy link
Contributor

ngnpope commented Jun 25, 2018

IE11 displays Intranet pages in compatibility mode by default

A quick search shows that using meta tags may not solve this problem for you: https://stackoverflow.com/a/9338959

@amarvin
Copy link
Author

amarvin commented Jun 25, 2018

It does work in my case. I overrode dash.Dash.index() with a modified version that includes <meta http-equiv="X-UA-Compatible" content="IE=edge">, and the Intranet Dash app now renders fine in IE11.

@pkallos
Copy link

pkallos commented Jun 27, 2018

Would it be possible to adjust this patch to allow arbitrary of either

  • arbitrary<meta> tags
  • arbitrary content to be rendered in the <head>

@chriddyp
Copy link
Member

Would it be possible to adjust this patch to allow arbitrary of either

See the proposal in #265

billdoors
billdoors reviewed Jul 9, 2018
View reviewed changes
dash/dash.py
@@ -300,6 +300,8 @@ def index(self, *args, **kwargs): # pylint: disable=unused-argument
<html>
<head>
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
Copy link

Choose a reason for hiding this comment

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

My dash app doesn't work with IE. Only after I added these two lines, it work with all browsers. It will be nice to include this in the Dash by default.

This was referenced Jul 9, 2018
Closed
Closed
@chriddyp
Copy link
Member

I'm still interested in this but I'm afraid I don't have enough information. Could someone help me verify a few things?

  • Will adding this tag negatively effect any Dash apps on any browsers or is it safe to add to our default index? For example, will this break some of our user's apps that are embedding a certain version of bootstrap?
  • When we say that Dash "Doesn't work in IE11", what in particular isn't working? When I render the app in browserstack on IE11, Dash renders for me:
    image
  • In particular, is it that you are appending external CSS stylesheets that need the tag? i.e. bootstrap 3
  • Or is there JavaScript functions that are failing? If its a JavaScript issue, is the real root of the problem that we are not embedding some polyfill that we need to be?
  • For those IE users, are you able to visit the dash docs? If so, could you find another public Dash app that doesn't render correctly that I can use for testing?

@chriddyp
Copy link
Member

This popular project also includes it by default: https://github.com/h5bp/html5-boilerplate/blob/master/dist/index.html#L6.

Seems like we should give it a go. I'm still interested in answers to those questions above but given that adding this tag makes Dash apps work for some users in IE, seems like something worth putting in. Any other thoughts @plotly/dash ?

In order to add this, this PR's conflicts will need to be resolved

@ngnpope
Copy link
Contributor

ngnpope commented Aug 1, 2018

I'm still interested in this but I'm afraid I don't have enough information.

So see #275 (comment) and my response #275 (comment) above for the reasoning. Dash works fine in IE11, but apparently in certain configurations, e.g. when hosted on an intranet, IE11 will default to one of it's compatibility modes which causes it to behave like an older version of IE. I don't know what compatibility version was used for @amarvin's intranet, but it sounds like Dash doesn't work with it (and indeed Dash should not make any efforts to support IE<11 as Microsoft no longer support it).

Setting the X-UA-Compatible HTTP header to IE=edge will ensure that each version of IE which supports the header will render and behave at the current version, i.e. disabling compatibility modes. In the case of IE11, it will be forced to behave like IE11. This is often done in-page using <meta> tags.

Will adding this tag negatively effect any Dash apps on any browsers or is it safe to add to our default index?

The X-UA-Compatible header will only affect IE. Microsoft named it in such a way that it could be used by other vendors. (Once upon a time Google had Chrome Frame which was a plugin for IE that sort of embedded Chrome. It also used the header so that developers could force their users to use the Chrome Frame plugin for their site.)

For example, will this break some of our user's apps that are embedding a certain version of bootstrap?

The X-UA-Compatible header is unlikely to break anything. It is more likely that things are already broken as in @amarvin's case.

The <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no"> tag is more likely to break things for existing users, but is probably quite a sensible default. There are two options here - add it by default and document the change - or don't add it and allow people to add it easily. This is now possible with #286 merged.

Or is there JavaScript functions that are failing? If its a JavaScript issue, is the real root of the problem that we are not embedding some polyfill that we need to be?

This, I imagine, was the problem, but only because of IE11 being in compatibility mode and behaving as an older version of IE. You could actually simulate the problem by adding something like <meta http-equiv="X-UA-Compatible" content="IE=8"> and testing again.

I hope that this clears things up for you @chriddyp.

@chriddyp
Copy link
Member

chriddyp commented Aug 1, 2018

Alright, I think I'm convinced. @T4rk1n - could you wire this in? It looks like this tag needs to be the first meta tag (see https://stackoverflow.com/a/9624500/4142536). And we should allow users to override this tag with meta_tags, in a similar way to how we allow users to override the default encoding.

Many thanks everyone for the feedback and discussion ❤️

@T4rk1n
Copy link
Contributor

T4rk1n commented Aug 1, 2018

Alright I'll add this.

@T4rk1n T4rk1n mentioned this pull request Aug 1, 2018
Merged
@T4rk1n
Copy link
Contributor

T4rk1n commented Aug 1, 2018

Implemented in #316

@T4rk1n T4rk1n closed this Aug 1, 2018
@chriddyp chriddyp mentioned this pull request Aug 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@billdoors billdoors billdoors left review comments

Assignees

@T4rk1n T4rk1n

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@amarvin @ngnpope @chriddyp @pkallos @T4rk1n @billdoors