MD Settings: Ensure iron-lists are populated and sized correctly.

MD Settings: Ensure iron-lists are populated and sized correctly,

without using setInterval(). Instead rely on a call to this.async() to
ensure that the iron-list's contents have been updated before firing
an 'iron-resize' event.

This fixes the problem of an infinite setInterval() when a list is rendered
in the background because user initiated a search query.

BUG=657336
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[dpapad, 2016-10-21 23:12:02]

Description was changed from

==========
MD Settings: Ensure iron-lists are populated and sized correctly.

BUG=657336
==========

to

==========
MD Settings: Ensure iron-lists are populated and sized correctly.

BUG=657336
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-10-21 23:19:53]

Description was changed from

==========
MD Settings: Ensure iron-lists are populated and sized correctly.

BUG=657336
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Ensure iron-lists are populated and sized correctly,

without using setInterval. Instead rely on a call to this.async() to
ensure that the iron-list's contents have been updated before firing
an 'iron-resize' event.

BUG=657336
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-10-21 23:20:38]

Description was changed from

==========
MD Settings: Ensure iron-lists are populated and sized correctly,

without using setInterval. Instead rely on a call to this.async() to
ensure that the iron-list's contents have been updated before firing
an 'iron-resize' event.

BUG=657336
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Ensure iron-lists are populated and sized correctly,

without using setInterval(). Instead rely on a call to this.async() to
ensure that the iron-list's contents have been updated before firing
an 'iron-resize' event.

This fixes the problem of an infinite setInterval() when a list is rendered
in the background because user initiated a search query.

BUG=657336
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2016-10-24 21:35:34]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2016-10-24 21:35:35]

https://codereview.chromium.org/2441173002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right):

https://codereview.chromium.org/2441173002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:72: }.bind(this));
Assuming this works, then sweet, lgtm! See comment in issue.

[dpapad, 2016-10-24 22:24:34]

@Steven: I removed the scroll-container class (which had the 1px min-height),
and I was still not able to break the iron-list (it always displayes the correct
items on startup). Also scroll-container CSS class was only used in 2 cases,
whereas iron-list is used in more cases.

I am going to wait for a re-LGTM before landing.

https://codereview.chromium.org/2441173002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right):

https://codereview.chromium.org/2441173002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:72: }.bind(this));
On 2016/10/24 at 21:35:35, stevenjb wrote:
> Assuming this works, then sweet, lgtm! See comment in issue.

Yes, the bind() call works as expected.

[stevenjb, 2016-10-24 22:25:15]

On 2016/10/24 22:24:34, dpapad wrote:
> @Steven: I removed the scroll-container class (which had the 1px min-height),
> and I was still not able to break the iron-list (it always displayes the
correct
> items on startup). Also scroll-container CSS class was only used in 2 cases,
> whereas iron-list is used in more cases.
> 
> I am going to wait for a re-LGTM before landing.
> 
>
https://codereview.chromium.org/2441173002/diff/20001/ui/webui/resources/cr_e...
> File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right):
> 
>
https://codereview.chromium.org/2441173002/diff/20001/ui/webui/resources/cr_e...
> ui/webui/resources/cr_elements/cr_scrollable_behavior.js:72: }.bind(this));
> On 2016/10/24 at 21:35:35, stevenjb wrote:
> > Assuming this works, then sweet, lgtm! See comment in issue.
> 
> Yes, the bind() call works as expected.

Hmm. I am suspicious then. Let me test it later today.

[dpapad, 2016-10-25 22:21:40]

On 2016/10/24 at 22:25:15, stevenjb wrote:
> On 2016/10/24 22:24:34, dpapad wrote:
> > @Steven: I removed the scroll-container class (which had the 1px
min-height),
> > and I was still not able to break the iron-list (it always displayes the
correct
> > items on startup). Also scroll-container CSS class was only used in 2 cases,
> > whereas iron-list is used in more cases.
> > 
> > I am going to wait for a re-LGTM before landing.
> > 
> >
https://codereview.chromium.org/2441173002/diff/20001/ui/webui/resources/cr_e...
> > File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right):
> > 
> >
https://codereview.chromium.org/2441173002/diff/20001/ui/webui/resources/cr_e...
> > ui/webui/resources/cr_elements/cr_scrollable_behavior.js:72: }.bind(this));
> > On 2016/10/24 at 21:35:35, stevenjb wrote:
> > > Assuming this works, then sweet, lgtm! See comment in issue.
> > 
> > Yes, the bind() call works as expected.
> 
> Hmm. I am suspicious then. Let me test it later today.

@stevenjb: Did you get a chance to test it locally?

[stevenjb, 2016-10-26 00:03:49]

OK, I tested this locally and was unable to repro the issues we were seeing
before (http://crbug.com/639795). So, yay, still lgtm.

[dpapad, 2016-10-26 00:19:05]

On 2016/10/26 at 00:03:49, stevenjb wrote:
> OK, I tested this locally and was unable to repro the issues we were seeing
before (http://crbug.com/639795). So, yay, still lgtm.

Unfortunately I was able to repro the case that makes the iron-list incorrectly
populated (only 3 items). It is very consistent. Here are the repro steps:

1) Visit chrome://md-settings
2) Search for "manage" (this triggers rendering of iron-lists while they are not
visible).
3) Click on "Manage search engines". Observe that the list has only 3 items.
Resizing the window even by 1px triggers the list to be fully populated.

At least this gives us a greater understanding of when populating iron-lists
breaks, so will keep exploring whether a solution not involving timeouts exists.
For the record, restoring the 1px min height does not fix this case.

[stevenjb, 2016-10-26 00:26:33]

That makes sense I guess since the search render doesn't actually stamp
things?

Does the current setInterval hack work with your repro?


On Tue, Oct 25, 2016 at 5:19 PM, dpapad@chromium.org via
chromiumcodereview-hr.appspot.com <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/10/26 at 00:03:49, stevenjb wrote:
> > OK, I tested this locally and was unable to repro the issues we were
> seeing
> before (http://crbug.com/639795). So, yay, still lgtm.
>
> Unfortunately I was able to repro the case that makes the iron-list
> incorrectly
> populated (only 3 items). It is very consistent. Here are the repro steps:
>
> 1) Visit chrome://md-settings
> 2) Search for "manage" (this triggers rendering of iron-lists while they
> are not
> visible).
> 3) Click on "Manage search engines". Observe that the list has only 3
> items.
> Resizing the window even by 1px triggers the list to be fully populated.
>
> At least this gives us a greater understanding of when populating
> iron-lists
> breaks, so will keep exploring whether a solution not involving timeouts
> exists.
> For the record, restoring the 1px min height does not fix this case.
>
> https://codereview.chromium.org/2441173002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dpapad, 2016-10-26 00:32:36]

On 2016/10/26 at 00:26:33, stevenjb wrote:
> That makes sense I guess since the search render doesn't actually stamp
> things?
Everything is stamped as if you visited the page, that's not the problem. I am
currently tracing iron-list's logic to figure out what is different. At first
glance, it knows whether it is visible or not and follows different codepath [1]
when first being rendered. Then, when it actually becomes visible at repro step
3, the container has only 2px height, and for some reason it chooses the minimal
default number of contents which is 3, see [2]. Still not sure why if
physicalCount == 0, it only grows it by 3 slots.

[1]
https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...
[2]
https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...


> 
> Does the current setInterval hack work with your repro?

[dpapad, 2016-10-26 00:38:14]

> > Does the current setInterval hack work with your repro?

Kind of. It causes the iron-list to be fully populated, although it is clearly
visible (flickers) that it has 3 elements first, then the rest get painted. Also
if you never visit the page that holds the iron-list, the setInterval() executes
indefinitely, which is the original bug I am trying to fix.

[dpapad, 2016-10-26 01:44:10]

On 2016/10/26 at 00:38:14, dpapad wrote:
> > > Does the current setInterval hack work with your repro?
> 
> Kind of. It causes the iron-list to be fully populated, although it is clearly
visible (flickers) that it has 3 elements first, then the rest get painted. Also
if you never visit the page that holds the iron-list, the setInterval() executes
indefinitely, which is the original bug I am trying to fix.

@stevenjb: After investigating iron-list's internals (and with @dbeam's help),
it seems there is a bug that if fixed would eliminate the need to setInterval()
or this.async(). I still have not heard back from iron-list's authors though, so
not 100% sure yet. Sent a PR with the fix
https://github.com/PolymerElements/iron-list/pull/337 (could not find your
github ID to cc you there).