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

getBoundingClientRect Error when loading data to observable list #83

Closed
hmarcelodn opened this Issue Sep 15, 2016 · 31 comments

Comments

Projects
None yet
@hmarcelodn
Copy link

hmarcelodn commented Sep 15, 2016

I'm submitting a bug report

Library Version: 1.0.0-beta.3.0.1
Please tell us about your environment:

Operating System: Windows Server

Node Version: 5.0.0

NPM Version: 3.5.2

JSPM: 0.16.35

Browser: all

Language: ECMA6

Current behavior:

When loading a list from a http request the virtual-repear.for="item of data" where data is the observable list loaded from the http request displays the following error and does not render full content.

Uncaught (in promise) TypeError: Cannot read property 'getBoundingClientRect' of null

Demo: http://screencast.com/t/oM6WzL63Ih

Expected/desired behavior:

Virtualize the content when the observable list changes without throwing a JS error.

@AStoker Thanks!

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented Sep 16, 2016

Can you format your question according to the issue template?
Can you also provide code samples of what you're doing?

@hmarcelodn

This comment has been minimized.

Copy link

hmarcelodn commented Sep 16, 2016

@AStoker Updated

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented Sep 16, 2016

How are you loading the data? Can you show me your view model for where you're loading up data?

@hmarcelodn

This comment has been minimized.

Copy link

hmarcelodn commented Sep 16, 2016

@AStoker just a simple http request like below then assign the array to the observable.

this.caseService .getCasesList(this.app.getOffice(), statusIds, typesIds, this.caseName, this.caseNumber) .then(response => { this.data = response.rows; });

It happens in the attached lifecycle

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented Sep 16, 2016

Sorry, I need to see a bit more :) Can you show me the view you have set up for this too?
Does this.data exist before the attached lifecycle?

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented Sep 16, 2016

And is there a reason that it's in the attached instead of the activate? Seems you'd probably want that kind of get logic in the activate

@hmarcelodn

This comment has been minimized.

Copy link

hmarcelodn commented Sep 16, 2016

`import {inject} from 'aurelia-framework';
import {CustomMessages} from 'shared/custom-messages';
import {EventAggregator} from 'aurelia-event-aggregator';
import {CaseService} from './case-service';
import {AppService} from 'services/app-service';
import {FormatFunctions} from 'shared/format-functions';

@Inject(CustomMessages, EventAggregator, CaseService, AppService, FormatFunctions)
export class List{
constructor(message, ea, caseService, app, formatFunc){
this.message = message;
this.ea = ea;
this.caseService = caseService;
this.app = app;
this.caseName;
this.caseNumber;
this.renders = [];
this.data = [];
this.statusList = [];
this.typesList = [];
this.checkDefaultStatus = "checkDefaultCaseStatus";
this.checkDefaulType = "checkDefaultCaseType";
this.defaultStatus = "Open";
this.placeholder = this.message.datagrid.noResults;
this.outputDefault = { id: 2 };

    this.circleBalance;
    this.permissions = 'banking_balances_view';
    this.formatFunc = formatFunc.hideBalance;

    this.columns = [
        { columnName: "Case #", name: "caseNumber", width: 100, template: "link" },
        { columnName: "Case Name", name: "caseName", width: 200, template: "link"  },
        { columnName: "Status", name: "status", width: 100, template: "default"  },
        { columnName: "Type", name: "type", width: 150, template: "default"  },
        { columnName: "Open Date", name: "openStatusDate", width: 150, template: "default"  },
        { columnName: "Est. Distribution Date", name: "distributionDate", width: 220, template: "default" },
        { columnName: "Balance", name: "balance", width: 200, template: "default"  }
    ];     

    this.caseService
        .getOpenedCases(this.app.getOffice())
        .then(response => {
            this.openedCases = response;
        });
}

show(){

}

activate(){
    $("body").css("background", "#EEEEEE");
    $(document).scrollTop(0);
}

attached(){
    this.search();
}

search() {

    let statusIds = [];
    let typesIds  = [];

    this.statusList
        .filter(item => item.checked)
        .forEach(item => 
        { 
            statusIds.push(item.id);
        });       

    if(statusIds.length == 0){
        this.ea.publish(`${this.checkDefaultStatus}`);
        statusIds.push(this.outputDefault.id);
    }

    this.typesList
       .filter(item => item.checked)
       .forEach(item => 
       { 
           typesIds.push(item.id);
       });       

    if(typesIds.length == 0){
        this.ea.publish(`${this.checkDefaulType}`);
    }

    this.caseService
        .getCasesList(this.app.getOffice(), statusIds, typesIds, this.caseName, this.caseNumber)
        .then(response => 
        {
            this.resizeLayer(response.rows.length);

            this.data = response.rows;
        });
}

reset(){
    this.caseName   = "";
    this.caseNumber = "";
    this.ea.publish(`${this.checkDefaultStatus}`);
    this.ea.publish(`${this.checkDefaulType}`);
}

resizeLayer(resultsGratherThanZero){
    if(resultsGratherThanZero){
        $(".appMainContainer").css("height", "initial");
    }else{
        $(".appMainContainer").css("height", "inherit");
    }
}

}`

@AStoker here is the full code for such viewmodel. You mean placing the code in activate so we can block the rest of the licycle so the virtualizer already find a list loaded, right?

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented Sep 16, 2016

You shouldn't have to block the lifecycle (there's a case for it, but I try not to do too many blocking actions like that).
For kicks and giggles, what happens when you comment out your search() call in the attached, so that the api call isn't even made? Does it still throw an error? I know it won't be making any api calls.

@hmarcelodn

This comment has been minimized.

Copy link

hmarcelodn commented Sep 16, 2016

@AStoker I tried that. The same error happens when the list is empty (I created the variable in the constructor).

What I did now, was to return a promise from the activate method. That thing worked. What do you suggest to me to do? Sounds like a workaround since it is still failing when its empty.

@hmarcelodn

This comment has been minimized.

Copy link

hmarcelodn commented Sep 16, 2016

How about if you modify the list? will that recognize the changes in the observable list?

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented Sep 16, 2016

It should. And it should also work with an empty list, so it sounds like something is breaking where it clearly shouldn't :) Can you reproduce this in the skeleton project and an empty data list?

@hmarcelodn

This comment has been minimized.

Copy link

hmarcelodn commented Sep 16, 2016

@AStoker I am going to send you the Skeleton soon. Thanks for helping on this.

@hmarcelodn

This comment has been minimized.

Copy link

hmarcelodn commented Sep 16, 2016

@AStoker should I upload a pull request for that? Thanks.

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented Sep 16, 2016

Pull requests are for if you have a fix for a problem. If you have a repo skeleton, just publish that to your own github account, and send me a link :)

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented Sep 20, 2016

Further clarifying this issue. Correct me if I'm wrong here @hmarcelodn.
This issue seems to only happen when using a table. and it is related to trying to get the first element of an empty tbody (

this.distanceToTop = this.domHelper.getElementDistanceToTopOfDocument(this.templateStrategy.getFirstElement(this.topBuffer));
); When we try getting the first element of a repeated div, the next element is an empty buffer div. When we try and get the first element of a table, since the buffer divs are outside the table, and the element it's trying to get the first child of is the tbody, we get nothing, causing errors down the line.
This should happen any time you have an empty array of elements at the time of attaching the virtual repeater to the dom, and are using a table.
Can you confirm this @hmarcelodn?

@EisenbergEffect EisenbergEffect added the bug label Sep 28, 2016

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Sep 28, 2016

How is this coming?

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented Sep 28, 2016

@EisenbergEffect, I can reproduce the issue.
Basic repro steps are for you to create a table with a virtual repeater on it, and then give it an empty list. The way that the template strategy is trying to find elements is causing it to not return any elements to render to, and therefore causing this issue.
It does seem related to an issue that was solved before by moving the table buffer elements outside the table, and I need to hear from @martingust about this issue (#84)
A fix for this would be to do null checks on elements everywhere that we try and compute values... But that has the potential to mess up a display if we try and compute the proper heights of elements before we give the repeater anything (we get nulls and 0's for places that would normally contain values).

@Nepoxx

This comment has been minimized.

Copy link

Nepoxx commented May 17, 2017

Is there any progress on it or a workaround? As far as I understand this seems to be an issue on empty lists, which should get "resolved" once the list is filled/replace?

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented May 17, 2017

@Nepoxx, some development was done recently with sizing elements. I don't recall if it directly solved this or not, can you confirm that it's still broken?

@Nepoxx

This comment has been minimized.

Copy link

Nepoxx commented May 17, 2017

I am still seeing the issue, but upon digging a bit deeper, I don't think it is caused by the same issue. My list is composed of (non-virtual) sub-lists, so I don't think virtualization can know how big the items of the list will be since they do not "exist" yet.

@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented May 17, 2017

So right now we have it set up so that it doesn't compute heights until at least the list is visible. So I guess the situation (sorry, it's been a while) is that when you have your async stuff, you need a callback to recalculate the heights?

@Nepoxx

This comment has been minimized.

Copy link

Nepoxx commented May 17, 2017

I'm not sure. Interestingly enough, the list does render correctly (but breaks my routing for some reason). Not a huge deal as this is not my main use case for virtualization.

I know a lot of similar libraries for other frameworks require that the elements of your list have the same height, is this also a requirement here? I am definitely breaking this assumption, the elements of my lists have varying height which are not predetermined until they are rendered. Basically what I have is something like this:

<div repeat.for="projectGroup of projectGroups" class="project-group" class.bind="projectGroup.id" if.bind="projects.length > 0">

      <h2>${projectGroup.title}</h2>

      <ul class="project-block-list">
        <li repeat.for="project of projects | filter:projectGroup.filter" >
          <ul>
            <li repeat.for="buildGroup of project.relevantBuildGroups"
                click.trigger="onBuildGroupClick(buildGroup)">
            </li>
          </ul>
        </li>
      </ul>
</div>

Stack (probably not useful, but still):

"TypeError: element.getBoundingClientRect is not a function
    at calcOuterHeight (http://localhost:9001/jspm_packages/npm/aurelia-ui-virtualization@1.0.0-beta.3.1.0/utilities.js?v=1495054898931:17:22)
    at VirtualRepeat._calcInitialHeights (http://localhost:9001/jspm_packages/npm/aurelia-ui-virtualization@1.0.0-beta.3.1.0/virtual-repeat.js?v=1495054898931:515:56)
    at VirtualRepeat.itemsChanged (http://localhost:9001/jspm_packages/npm/aurelia-ui-virtualization@1.0.0-beta.3.1.0/virtual-repeat.js?v=1495054898931:221:12)
    at BehaviorPropertyObserver.selfSubscriber (http://localhost:9001/jspm_packages/npm/aurelia-templating@1.4.2/aurelia-templating.js?v=1495054898931:3703:48)
    at BehaviorPropertyObserver.call (http://localhost:9001/jspm_packages/npm/aurelia-templating@1.4.2/aurelia-templating.js?v=1495054898931:3569:14)
    at BehaviorPropertyObserver.setValue (http://localhost:9001/jspm_packages/npm/aurelia-templating@1.4.2/aurelia-templating.js?v=1495054898931:3549:18)
    at VirtualRepeat.descriptor.set [as items] (http://localhost:9001/jspm_packages/npm/aurelia-templating@1.4.2/aurelia-templating.js?v=1495054898931:3658:43)
    at Object.setValue (http://localhost:9001/jspm_packages/npm/aurelia-binding@1.2.1/aurelia-binding.js?v=1495054898931:3622:25)
    at Binding.updateTarget (http://localhost:9001/jspm_packages/npm/aurelia-binding@1.2.1/aurelia-binding.js?v=1495054898931:4863:27)
    at Binding.call (http://localhost:9001/jspm_packages/npm/aurelia-binding@1.2.1/aurelia-binding.js?v=1495054898931:4878:16)
    at SetterObserver.callSubscribers (http://localhost:9001/jspm_packages/npm/aurelia-binding@1.2.1/aurelia-binding.js?v=1495054898931:373:19)
    at SetterObserver.call (http://localhost:9001/jspm_packages/npm/aurelia-binding@1.2.1/aurelia-binding.js?v=1495054898931:3695:12)
    at TaskQueue.flushMicroTaskQueue (http://localhost:9001/jspm_packages/npm/aurelia-task-queue@1.2.0/aurelia-task-queue.js?v=1495054898931:151:16)
    at MutationObserver.eval (http://localhost:9001/jspm_packages/npm/aurelia-task-queue@1.2.0/aurelia-task-queue.js?v=1495054898931:79:24)"
@AStoker

This comment has been minimized.

Copy link
Member

AStoker commented May 18, 2017

In regards to the height, yes, this library assumes that all heights are the same. I have a few ideas floating around on how to improve this to allow dynamic height. The issue becomes more complex with dynamic heights as you need to at least somewhat reliably recalculate buffer heights to simulate a proper scroll bar height.
With dynamic heights, what we'll have to give up is a fully "scaled" scroll bar. That is to say as you scroll and elements become visible, that scroll bar may jump in size/height as the virtual list recalculates appropriate sizes.

@Nepoxx

This comment has been minimized.

Copy link

Nepoxx commented May 18, 2017

I missed this part in the readme: The repeated rows need to have equal height throughout the list,

My apologies.

As to your comment, I think the only correct way would be to do something like a virtual (virtual) dom render so that you know exactly what the list looks like, assuming that the list is generated once. If the list changes dynamically I don't think there's much you can do to tell the browser its height.

@nisargrthakkar

This comment has been minimized.

Copy link

nisargrthakkar commented Sep 15, 2017

Any update on this bug? I got this error while using virtual-repeat for table

@elitemike

This comment has been minimized.

Copy link

elitemike commented Sep 16, 2017

I too just ran into this issue while creating a demo scenario for a performance fix.

@JordyvA

This comment has been minimized.

Copy link

JordyvA commented Dec 6, 2017

I'm getting the GetBoundingClientRect error when I'm using the "with.bind" structure from Aurelia in the repeater. I think this library can't handle that. See the linked issue: #110

@salminio

This comment has been minimized.

Copy link

salminio commented Apr 18, 2018

Just tried upgrading to 3.3.0 from 3.0.2 and this issue just appeared - same stack as previously mentioned. Interestingly enough the code around the error doesn't look very much different. What I noticed is the VirtualRepeat.prototype.attached getting called before the list is even valued in the later code where the prior code (where it works) - it isn't called until valued and the 'element' then has the function getBoundingClientRect

@st-it

This comment has been minimized.

Copy link

st-it commented Jul 26, 2018

In my case (tr element with virtual-repeat attribute). It seems the attached function has a comment node declared at the element property. The comment node is a child node of the tr element.
Just created this module with a piggyback which works around the issue by overwriting the element to the parent element.
This can be loaded as a global resource from the main module.

Hope this helpes fixing the bug.

import {VirtualRepeat} from "aurelia-ui-virtualization";

VirtualRepeat.prototype.attached = (function (oldFn) {
    return function () {
        let element = this.element;
        if (element.nodeType === 8) {
            this.element = element.parentElement;
        }
        oldFn.apply(this);
    };
})(VirtualRepeat.prototype.attached);
@roddharris

This comment has been minimized.

Copy link

roddharris commented Nov 6, 2018

I'm getting the same error Object doesn't support property or method 'getBoundingClientRect'. I'm doing the following:

async validateFile() {
   this.dialogService.open(…) //Allows user to select a file
      .whenClosed(async response => {
          if(!response.wasCancelled){
               this.isProcessing = true;
              //below line populates billingValidations array of dataService object
              //declared as: billingValidations = [];
               await this.dataService.validateBillingFile(response.output.file); 
               this.sortResults();
               this.isProcessing = false;
               return;
       }
}


<div if.bind="!isProcessing" style="height:100%;overflow-y:scroll;margin: 10px 20px;">
    <table class="table table-sm">
         <thead>…</thead>
         <tbody>
               <tr virtual-repeat.for = "item of dataService.billingValidations">
                     <td>…</td>
               </tr>
          </tbody>
    </table>
</div>

This renders the first 30+ items in the DOM and the size of the scrollbar reflects that there are 2000+ items total. But when I scroll to the last of the 30+ items, everything after that is blank -- it scrolls but there is nothing rendered below the last of the rendered items.

Incidentally - the error appears in the console before I do any scrolling -- essentially as soon as this.isProcessing = false occurs and the div is rendered.

Also, the project I'm working in was based on the Navigation Skeleton -- not exactly sure what version of Aurelia that was based on.

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Jan 15, 2019

I think this issue has been fixed in latest commits. @EisenbergEffect I think it can be closed for now

@hmarcelodn Please try with latest version after new version has been released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment