html-next / vertical-collection

Infinite Scroll and Occlusion at > 60FPS
https://html-next.github.io/vertical-collection/
MIT License
176 stars 76 forks source link

Vertical collection does not work very well with IE11 and Edge #74

Closed jevanlingen closed 7 years ago

jevanlingen commented 7 years ago

The plugin is just great in Chrome and Firefox. Sadly if I watch the table demo in the Windows browsers, not so much.

Internet Explorer 11 First entries are loaded, but when scrolling down no new entries are drawn: image

Edge Only first entry row is drawn (when scrolling up and down, all entries are drawn, so it seems some initialization error):

image

runspired commented 7 years ago

Does this only happen on the table demo? Or on the other demos as well?

jevanlingen commented 7 years ago

Internet Explorer

Edge

pzuraq commented 7 years ago

The issues with Edge are definitely related to the way tables render. Specifically, something to do with the way that we're adding VCs on initial render. If we populate the VC pool when we create the Radar, it renders like it should.

I can't seem to exactly reproduce the IE11 issues. For me, it renders the items, but doesn't update them on scroll.

Gotta handle some of the other issues, but edge and IE support are definitely a priority so will come back to this asap.

pzuraq commented 7 years ago

IE11 should have been fixed by #92, still working on Edge so keeping this issue open

kumkanillam commented 7 years ago

In IE-11 , I get the error Unable to get property 'scrollTop' of undefined or null reference.

image

pzuraq commented 7 years ago

It looks like you're using an older version, can you update to the latest master and see if it still happens? We should be releasing a new beta soon

kumkanillam commented 7 years ago

@pzuraq I updated using this command ember install git+https://github.com/html-next/vertical- collection.git#master.

Receiving the below error in IE, image

Now I am receiving the below error in chrome


app.js:106 Ember.onerror handler - error.message _verticalCollectionPrivate.Token is not a constructor  error TypeError: _verticalCollectionPrivate.Token is not a constructor
    at Class.init (component.js:235)
    at Class.superWrapper [as init] (ember.debug.js:40428)
    at new Class (ember.debug.js:36121)
    at Function._ClassMixinProps.create (ember.debug.js:36309)
    at CurlyComponentManager.create (ember.debug.js:12591)
    at OpenComponentOpcode.evaluate (ember.debug.js:47198)
    at VM.execute (ember.debug.js:53789)
    at VM.resume (ember.debug.js:53768)
    at TryOpcode.handleException (ember.debug.js:54279)
    at UpdatingVMFrame.handleException (ember.debug.js:54459) ```.
pzuraq commented 7 years ago

That looks like a babel issue to me, we do some custom babel transforms to make everything faster when possible. What version of ember-cli are you using?

runspired commented 7 years ago

ember-cli needs to be ~2.4 or later I believe

pzuraq commented 7 years ago

For reference, we're running Ember-CLI 2.8 with Ember 1.8, so it's possible to use a much later version of CLI with earlier versions of Ember. I'd definitely be willing to help you figure out what's happening and fix it @kumkanillam, especially if it's on a more recent version of Ember-CLI

kumkanillam commented 7 years ago

@pzuraq Sorry for the late reply. I am using ember-cli: 2.11.0 node: 6.3.1 os: win32 x64 "ember-source": "~2.11.0",

pzuraq commented 7 years ago

yeah, the error you're getting on beta.3 has likely been fixed in master. The error you're getting on master is very different, it looks like something strange is happening with your build that's causing our -private package to not exist. Can you post your ember-cli-build, and the other addons that you're using? If not, can you reproduce in a minimal example app so that we can try to debug this? Could also setup a screensharing session to dive in and take a look.

kumkanillam commented 7 years ago

I will try it in brand new app, and let you know the status now.

kumkanillam commented 7 years ago

@pzuraq I created new app called vc-test, its working fine. it comes with "ember-cli": "2.12.1" but then when I change this to "ember-cli": "2.11.0", version by running the npm install ember-cli@2.11.0 --save-dev. I got the error _verticalCollectionPrivate.Token is not a constructor error TypeError: _verticalCollectionPrivate.Token is not a constructor in chrome. Is there any difference if I update ember-cli version alone without following proper update I haven't done ember init too.. I am ok to screensharing session if you want.

kumkanillam commented 7 years ago

@pzuraq I can confirm that ember-cli version 2.11.0 is causing chrome error.

pzuraq commented 7 years ago

Ok, I opened #96 to track this issue, looks like we have a bug with that specific version of ember-cli

pzuraq commented 7 years ago

So now that beta.4 is released I'm revisiting this, IE is throwing an error because something doesn't support an includes method, and Edge is failing on some examples due to measuring errors. The other issues mentioned seem to be resolved with the new rendering strategy, so going to dig into those and close this once we fix them.

pzuraq commented 7 years ago

So it looks like IE/Edge do not have the same results for Range.getBoundingClientRect as every other browser. In IE, the blank text nodes surrounding each Virtual Component are seen as if they are immediately surrounding the text content of the component, not the DOM nodes. The heights we get back are inaccurate because of this.

This is very problematic for our current measuring strategy, basically makes IE/Edge unusable with DynamicRadar. We'll have to figure out a workaround.

pzuraq commented 7 years ago

So I'm going to attempt a refactor where we find and measure the root elements of each VC directly, and throw errors if any text nodes are included that aren't wrapped in an element and contain content of any kind. The alternative is to enforce one single root element, which may end up being what we have to do.

pzuraq commented 7 years ago

Should be fixed now! Let me know if there are any more issues with IE/Edge. I'm going to try to get Saucelabs set asap so we are actually running the full test suite against all browsers.

jevanlingen commented 7 years ago

@pzuraq: At the moment I used beta.7 in combination with Ember 2.12. Just upgraded to Ember 2.14.

Everthing went fine, but stumbled at an IE error: TypeError: Unable to get property 'source' of undefined or null reference.

Are Ember 2.13 and 2.14 already fully supported btw? If I check the demo pages, Ember 2.12 is used there as well!

pzuraq commented 7 years ago

They should be supported, I haven't encountered that issue with any version of Ember before, and we run all of our tests against the latest release, beta, and canary versions, although not in IE. Can you post steps to reproduce?

jevanlingen commented 7 years ago

Ah just found the problem, I was missing some polyfills for IE. Just by adding

'ember-cli-babel': {
    includePolyfill: true
}

to the ember-cli-build.js fixes the problems!

Apparently you need full polyfills to let vertical-collection work with the latest Ember versions. @pzuraq: Maybe this should be noted somewhere in the docs?

pzuraq commented 7 years ago

The polyfills shouldn't be required, if we're relying on polyfilled APIs then we should probably figure out what APIs those are and fix them. Can you post reproduction steps so we can do that?

pzuraq commented 7 years ago

I tried using the error message you posted above, I can't find anywhere in the source where we access a source property

localpcguy commented 6 years ago

@pzuraq I ran into the TypeError: Unable to get property 'source' of undefined or null reference and it isn't actually in the VerticalCollection code, but it is being thrown BY the VerticalCollection code (if I remove the VerticalCollection the error disappears). It is in the meta function in Ember's meta.js file

The check is for undefined specifically but maybeMeta is null when getting this error in IE11.

  // remove this code, in-favor of explicit parent
  if (maybeMeta !== undefined) {
    if (maybeMeta.source === obj) {
      return maybeMeta;
    }
    parent = maybeMeta;
  }

I'm not sure if it's actually a bug in Ember, or if VerticalCollection is somehow causing it to trigger.

localpcguy commented 6 years ago

Adding the includePolyfill also fixed the issue for my case, so I went looking in VerticalCollection, and I think the issue in IE11 might be the use of .find in DynamicRadar (line 246). IE11 doesn't support .find which has bitten me a number of times, and Babel doesn't seem to transform it (at least not by default). Either directly including a find polyfill or using something like filter(...)[0] (with appropriate checking for zero length) could work.

pzuraq commented 6 years ago

The SkipList is just a class though, so it's unlikely that that's causing the issue: https://github.com/html-next/vertical-collection/blob/master/addon/-private/data-view/skip-list.js#L112

There's definitely something that needs to be polyfilled. Could be an array function? Or maybe we're using maps or something else there?

localpcguy commented 6 years ago

Whoops, that my bad for assuming that .find was just the array function. Sorry.

Looking around some more, I also saw code which sets __ember_meta_ to null - could that be where the null is coming from that errors in the meta.js file in Ember?

this.__ember_meta__ = null; in radar.js

pzuraq commented 6 years ago

ah, that is probably causing the issue. The reason for that is in Ember 1.11 we attempt to freeze the objects to make sure their shape is unchanged for perf, we can only do that in legacy Ember.

localpcguy commented 6 years ago

Could it do something like the following? If that would work, I can probably do small PR for it if you'd like.

import { IS_GLIMMER_2, GTE_EMBER_1_13 } from 'ember-compatibility-helpers';
...
if (!GTE_EMBER_1_13) { 
   this.__ember_meta__ = null;
}

Or it might be as simple as assigning it to undefined instead of null?

pzuraq commented 6 years ago

I think that may work, or IS_EMBER_2 possibly