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

[Grid] Demo page renders improperly in IE11 #13639

Closed
2 tasks done
doncarron opened this issue Nov 19, 2018 · 12 comments
Closed
2 tasks done

[Grid] Demo page renders improperly in IE11 #13639

doncarron opened this issue Nov 19, 2018 · 12 comments
Assignees
Labels
docs Improvements or additions to the documentation test

Comments

@doncarron
Copy link

When visiting https://material-ui.com/layout/grid/ and comparing the resulting page in IE11 with what appears in chrome there are clear layout issues visible which undermine confidence in this library's ability to support IE11.

Particular issues occur in the top-right "page contents" area, the CSS Grid Layout example, and the white-space: nowrap; example.

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

Page should render correctly. See Chrome for example.

Current Behavior

Page renders improperly.

Steps to Reproduce

Compare https://material-ui.com/layout/grid/ in IE11 vs Chrome (latest)

Context

Our team is trying to use Material UI for a complex application that requires IE11 support and noticed that performance, in particular with the layout and the menu, was also unacceptably slow. We also noted that the Material UI team states they are supporting unit tests in the following browsers but IE11 is omitted:

Material-UI has 1200+ unit tests running on Chrome 49, Firefox 45, Safari 10 and Edge 14.

Can we expect these issues to be addressed in the future or does the Material UI team anticipate dropping support for IE11 entirely? This would help us plan for the future.

Your Environment

Tech Version
Material-UI v3.5.1
Browser IE11
@eps1lon
Copy link
Member

eps1lon commented Nov 19, 2018

There are 3 things that we need to address separately:

  1. run unit tests in IE11
  2. run visual regression tests on different browsers/render engines
  3. improve performance

The first point is held back by some incompatibility issues with the testing libraries we use. We should be able to make it work though.

Visual regression tests are fairly expensive and even if we would pass on the lowest supported versions we would still have no guarantee that something didn't break between e.g. chrome 50 and 51. So for something like testing if the demos render properly we would need visual regression test not only unit tests which might require using another testing library. Would like to know how and if other bigger component libraries do that.

The third point needs proper benchmarks. I guess @oliviertassinari started this effort with the @material-ui/benchmark package but as far as I know we don't automatically track the status via e.g. CI (or github actions) yet.

@doncarron
Copy link
Author

@eps1lon Thanks for your reply. Can I infer then from your statement on #1 that the Material UI team intends to continue supporting IE11 for the foreseeable future?

As for the IE11 specific performance issues / rendering problems would the Material UI team benefit from additional examples of these or is resolution simply a WIP and consumers just need to be patient?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 19, 2018

When visiting https://material-ui.com/layout/grid/ and comparing the resulting page in IE11 with what appears in chrome there are clear layout issues visible which undermine confidence in this library's ability to support IE11.

@doncarron We are using Material-UI in production at https://www.onepixel.com/. IE 11 has almost as much traffic as Firefox on our product, it's something we care about. IE 11 support of our core component has been handled a long time ago. In this case, I believe it's only impacting demos of our documentation, not the core components. IE 11 support of our documentation is a different story. We have made some progress recently. It used to be a blank page.

the CSS Grid Layout example

We explicitly warn about it, it's just a demo no using our core components. I guess we can add a ⚠️. As for the other demos, we should be able to sort that out.

  1. run unit tests in IE11

@eps1lon 💯 with that. I wish we could run the tests with Chrome 41 too, it's even more important from an SEO point of view. We track JavaScript errors at Onepixel with Sentry, so I know there is no JavaScript.

  1. run visual regression tests on different browsers/render engines

IMHO it's overkill (cost/value), we "rarely" have visual issue between browsers, most of them are quickly reported by our users and fixed.

  1. improve performance

How can we reproduce the issue? Material-UI performance is relatively good. Hook migration will help, emotion backend can help, benchmarking we should start with.

@eps1lon
Copy link
Member

eps1lon commented Nov 19, 2018

As for the IE11 specific performance issues / rendering problems would the Material UI team benefit from additional examples of these or is resolution simply a WIP and consumers just need to be patient?

We don't have a setup for benchmarks in IE11 but you can always file an issue with a repro and we can take a look. But we need to evaluate each of those individually. For example we are currently not using PureComponent anywhere (or React.memo) so if you rerender critical parts of your component tree just with the basic mui components then you would probably gain the most by wrapping them in a memoized component and making sure that the props are correctly memoized. But again all of this is pure speculation without a benchmark.

  1. run visual regression tests on different browsers/render engines

IMHO it's overkill (cost/value), we "rarely" have visual issue between browsers, most of them are quickly reported by our users and fixed.

Completely agree. Adding this would add quite a bit of extra time to our CI tasks. This would probably be a candidate for some long running background task that doesn't block merges but warns once it detects a regression.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 19, 2018

7d2070f solves the visual issue, you can verify on https://material-ui.netlify.com/layout/grid/.

@eps1lon Is working on the IE 11 test suite.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation test labels Nov 19, 2018
@doncarron
Copy link
Author

Thanks for your attentiveness. That said there are still issues remaining that I mentioned earlier.

The "page contents" area:

image

The "Material UI grid" (I get that the CSS Grid Layout won't work...)

image

Our organization would definitely like to leverage Material UI in a larger manner so we will work on producing examples that demonstrate the performance issues.

Thanks!

@eps1lon
Copy link
Member

eps1lon commented Nov 21, 2018

@doncarron We investigated running unit tests in IE 11 and this is currently not viable. See #13642 (comment) for an in-depth explanation. TL;DR: facebook components and react itself isn't unit tested in IE 11 either. But running unit tests in the supported browsers is only part of supporting a specific browser anyway.

@doncarron
Copy link
Author

@eps1lon Thanks for looking into it. I raised the unit testing concern only to reinforce my question as to whether or not a commitment was being made to IE11 - not because I personally felt it would make a difference identifying the kinds of issues that could occur. Based on your own statements as well as @oliviertassinari it is clear IE11 support is still a focus for the Material UI team.

@oliviertassinari
Copy link
Member

The "Material UI grid" (I get that the CSS Grid Layout won't work...)

It's expected, it's not supposed to work.

The "page contents" area:

Let's fix that.

@oliviertassinari oliviertassinari self-assigned this Nov 21, 2018
oliviertassinari added a commit that referenced this issue Nov 21, 2018
@oliviertassinari
Copy link
Member

@doncarron The table of contents issue should be fixed with 52e8204.

@doncarron
Copy link
Author

@oliviertassinari Thanks. Since, as you said, none of the core code actually changed these were all just cosmetic updates. But at least my team can't reference this page as an example why Material UI doesn't work properly in IE11 anymore.

@oliviertassinari
Copy link
Member

@doncarron Every detail counts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation test
Projects
None yet
Development

No branches or pull requests

3 participants