New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 657336 link

Starred by 1 user

Issue metadata

Status: ExternalDependency
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[MD Settings] CrScrollableBehavior fires a timer every 10ms after doing a search

Project Member Reported by tsergeant@chromium.org, Oct 19 2016

Issue description

Steps to reproduce:

1. Load MD Settings
2. Perform a search for anything (search term 'the' works for me)
3. Open devtools and record a short timeline trace while idle

Expected behavior:

Timeline should be idle

Actual behavior:

Timeline is full of tiny calls to cr_scrollable_behavior:70 (https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scrollable_behavior.js)

It seems like something to do with performing a search causes the scrollable-behavior to get stuck in this half-initialized state forever.
 
Screen Shot 2016-10-19 at 11.51.17 AM.png
27.8 KB View Download
Cc: steve...@chromium.org
Labels: Hotlist-MD-Settings-SearchBox
Owner: dpa...@chromium.org
Status: Assigned (was: Untriaged)
<triage>@dpapad, can you look into this?</triage>

Comment 2 by dpa...@chromium.org, Oct 19 2016

Cc: dbeam@chromium.org
I am able to reproduce this. It is caused by the setInterval() call at https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scrollable_behavior.js?l=70.

@stevenjb: Was this call supposed to be setTimeout() instead of setInterval()?

Comment 3 by dpa...@chromium.org, Oct 19 2016

It seems that the code assumes that at some point the scrollHeight of the iron-list's container will get a non-zero value (see [1]), but that is not the case when a section is rendered in the background (not visible), for the purposes of searching.

Notice that if the user navigates to the password section after triggering search, the calls to the setInterval() callback are finally stopped.

[1] https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scrollable_behavior.js?l=73
Cc: dschuyler@chromium.org michae...@chromium.org
Right. So, this is an inherent problem with iron-list. I don't know what the best solution is. What we really need is an event indicating that the container has been stamped, but I am unaware of any such event.

Maybe we just need to disable this (and similar loops) during a search?

Comment 5 by dpa...@chromium.org, Oct 19 2016

@stevenjb: Can you elaborate a bit on what exactly is the inherent problem of iron-list? I understand it is related with sizing but don't have the full context. Perhaps we can find a different fix that does not involve a timer/interval.

Disabling this logic during search feels more like a work-around than a fix, so I prefer to do this only if we exhaust other approaches.

From reading the comment above the updateScrollableContents() method, it seems related to https://github.com/PolymerElements/iron-list/issues/21, but not 100% sure.
It's all more of a workaround than a fix frankly.

I feel like I understood the problem at the time, but I would have to
re-immerse myself to be able to explain it.

Dave or Michael may have it in their brain cache.

I believe it is something like "iron-list contents are populated before
it's stamped, but the container height is 0 so nothing is rendered, so the
list height is 0, so when the container is stamped its height is 1 (the min
height). At that point we need to tell the iron-list to resize itself".

I think that it is related to the bug you referenced.

Comment 7 by dpa...@chromium.org, Oct 19 2016

I see, thanks for the info. So IIUC basically the updateScrollableContents() fixes the problem of erroneously empty iron-lists that get populated if the window is manually resized (hence the call to  ironList.notifyResize() inside that method).
Correct. Basically we need to force a resize at some point after the
container is guaranteed to be stamped.
Yes to comments #0 to #8 above, aside some extra explanation for:
>@stevenjb: Was this call supposed to be setTimeout() instead of setInterval()?

The trade-off here is that using setTimeout() means that we'd need to choose a timeout long enough that it would catch it even on a very slow desktop/laptop (which may be unpleasantly long on fast machines). Using setInterval allows us to have a much shorter timeout, so that it's more responsive in more cases (and works in fast and slow machines where it will eventually render).

The expectation is that the check won't be done more than a handful of times and then we'll be all set (the interval will stop). Never actually rendering wasn't considered when the workaround was made. Afaik, the workaround is still the most correct solution we have knowledge of. If we are not normally going to search within iron-lists (are we going to do that?), then it seems reasonable to avoid starting this interval if we know it will never succeed.
I have a candidate fix, that seems to work, without involving any timeouts, see https://codereview.chromium.org/2441173002.

Basically it relies on this.async() call after the array that backs up a list is modified, to ensure that the list has been rendered (similar to a dom-repeat), before firing an 'iron-resize' event that causes the list to be properly displayed. I've verified that it works for the onStartup and searchEngines iron-lists (they are always populated with the correct number of elements).

@stevenjb, @dschuyler: Do you know/remember if relying on this.async() is not reliable (meaning that it fires before the list's <template> has been expanded)?
I seem to recall that we had some issues with relying on async(), although I don't recall the specifics.

I know that there were a lot of changes that happened roughly in parallel however. One of which was ensuring that the container has a min height of 1. So it is possible that after all of the other changes were added, we can now replace setInterval() with async()? If so that would but lovely...



Status: ExternalDependency (was: Assigned)
Status update: https://github.com/PolymerElements/iron-list/pull/337 fixes the issue in iron-list such that calling neither setTimeout() nor this.async() is necessary. Waiting to hear back from iron-list authors on whether the fix is OK and can land in iron-list proper sometime soon.
Labels: -Pri-2 Pri-3
what's the status here?
Not much progress.
1) My original PR which fixes the issue (https://github.com/PolymerElements/iron-list/pull/337), was dropped because it seemingly was not aligned with iron-list's original design (see long discussion in the PR).
2) A different PR was landed which was supposed to fix this (https://github.com/PolymerElements/iron-list/pull/345), but last time I checked it was not addressed by that PR (which is why we have not rolled iron-list in our local copy since).
3) A meeting was arranged with iron-list authors to clarify the situation (you were present in that meeting), and the conclusion was that we will try more things to try making it work.

Among the things that were suggested was dropping iron-list for small "dumb" lists, and just create our own keyboard-accessible list container (IIRC someone was working on that for chrome://extensions, but have not heard back).

Sign in to add a comment