Fixes #516 and #515

[appreciated]

This change is 

[CLAassistant]

All committers have signed the CLA.

[pleku]

Thanks for the contribution @appreciated, I would have done the same changes myself but didn't yet get to it.

Also I had no idea (nor did @tomivirkki ) that this might get executed when the underlying iron list doens't have anything in the DOM.

[appreciated]

I would have done the same changes myself but didn't yet get to it.

@pleku I'm sure you would have 🙂.

Also I had no idea (nor did @tomivirkki ) that this might get executed when the underlying iron list doens't have anything in the DOM.

Maybe the causes some additional resize events which then cause the grid to update itself before it is initialized 🤔? Just a guess.

[denis-anisimov]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @denis-anisimov)

a discussion (no related file):
Is it possible to create an IT test for this ?

---

[denis-anisimov]

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @denis-anisimov)

[appreciated]

Is it possible to create an IT test for this ?

@denis-anisimov not sure since this only throws a client side exception. Note that I have pretty much no experience with Testbench.

[denis-anisimov]

Do you have a scenario when a client side exception is thrown ?

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @denis-anisimov)

[heruan]

@denis-anisimov There is an example project in #515 which should replicate the issue, is that what you need?

[denis-anisimov]

@denis-anisimov There is an example project in #515 which should replicate the issue, is that what you need?

OK, so it means that it's possible to make IT test for the fix.

[heruan]

@appreciated and @jflamy do you think you can add an IT to get this merged? You already have a sample project which replicates, I have no TestBench experience but I guess it should be pretty straighforward with the code you already have.

[heruan]

Worth noting this might (just a guess, not tested) be fixed in the Web Component, initializing _physicalItems as an empty array instead of null at vaadin/vaadin-grid/src/iron-list.html#L113.

[appreciated]

Any requirements for the integration-test? Can I find some coding guidelines for that anywhere?

[denis-anisimov]

No any specific requirements.
Let's see the result in review.

Just use existing code as an example.
E.g. https://github.com/vaadin/vaadin-grid-flow/blob/master/vaadin-grid-flow-integration-tests/src/main/java/com/vaadin/flow/component/grid/it/DetailsGridPage.java is a page which contains a usecase (UI control elements to reproduce the issue).
And https://github.com/vaadin/vaadin-grid-flow/blob/master/vaadin-grid-flow-integration-tests/src/test/java/com/vaadin/flow/component/grid/it/DetailsGridIT.java is an IT test code.

[heruan]

@appreciated, did you find out what is triggering the error? I'm not using any addon (just Grid) and I'm experiencing this in my app, but I cannot replicate in a simple project or IT test.

[heruan]

I still can't figure out what is causing this. Any chance the team will fix this for V13, if the author doesn't?

[appreciated]

I'm onto something. This issue only occurs if the grid is being wrapped inside a PolymerTemplate. I'm currently trying to put a mini-minimal example together.

[appreciated]

This is so wierd 😒.

An example project with the following code can be found here

Wrapping a non empty Grid:

GridTest

public class GridTest extends Grid<GridTest.Person> {

    List<Person> people = new ArrayList<>();

    public GridTest() {
        Random random = new Random();
        for (int i = 0; i < 300; i++) {
            people.add(new Person(UUID.randomUUID().toString(), random.nextInt(), i));
        }
        setItems(people);
        addColumn(person -> person.name);
        setSizeFull();
    }

    public class Person {
        private final String name;
        private final int id;
        private final int year;

        public Person(String name, int year, int id) {
            this.name = name;
            this.year = year;
            this.id = id;
        }

        public String getName() {
            return name;
        }

        public int getBirthYear() {
            return year;
        }

        public int getID() {
            return id;
        }
    }
}

in a Template as follows

MyTemplate

@Tag("a-template")
@HtmlImport("frontend://src/polymer-template.html")
public class MyTemplate extends PolymerTemplate<TemplateModel> implements HasComponents {
    @Id("test")
    Div div;
}

polymer-template.html

<link rel="import" href="../bower_components/polymer/polymer-element.html">

<dom-module id="a-template">
    <template>
        <style>
            #test {
                width: 100%;
                height: 100%;
            }
        </style>
        <div id="test">
        <slot></slot>
        </div>
    </template>
    <script>
        class ATemplate extends Polymer.Element {
            static get is() {
                return 'a-template'
            }
        }
        customElements.define(ATemplate.is, ATemplate);
    </script>
</dom-module>

Causes the issue.

But there is even more:

• If a PolymerTemple without a @Id binding is used this issue will not occur
• The issue only occurs in Chrome but not in Firefox

[appreciated]

@denis-anisimov I just found that I'm required to have a pro subscription to write Integration tests which I currently don't have. So I guess this need to be done by someone that has one 😓.

[heruan]

Thank you @appreciated for the thorough analysis! In my case the grid is in the lightdom of a component which is a PolymerTemplate, so maybe you're right.

I really hope the team can fix this for the upcoming platform release!

[heruan]

Worth noting that another issue in ComboBox connector happens under similar conditions: vaadin/vaadin-combo-box-flow#219

[denis-anisimov]

I cannot merge it without a test.
It's too hard to use your branch to write the test and the fix is trivial.
So it's better to close this PR and make a new one.

[denis-anisimov]

It's decided to merge this PR.
I will create another ticket for a test.

[denis-anisimov]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @appreciated and @denis-anisimov)

[denis-anisimov]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @appreciated and @denis-anisimov)

[denis-anisimov]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @appreciated)