MD Settings: Use scrollable behavior for all iron-list containers

MD Settings: Use scrollable behavior for all iron-list containers

This  adds a .scroll-container class which adds a 1px min-height
to ensure that scrollable iron-list elements update.

It also updates search_engines_page to use CrScrollableBehavior.

Note: Even though we are now using scrollable-behavior for search
engines, since we do not enforce a max height on the page or the
container, the containers will never actually scroll (but if we
do set a maximum page height they will, which is nice).

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

Committed: https://crrev.com/2c697de24370a2156fe3a6d616d75105005d4029
Cr-Commit-Position: refs/heads/master@{#421232}

[stevenjb, 2016-09-16 15:53:13]

Description was changed from

==========
MD Settings: Search Engines: Add scrollable behavior

This also adds a .scroll-container class which adds a 1px min-height
to ensure that scrollable iron-list elements update.

Note: Even though we are now using scrollable-behavior for search
engines, since we do not enforce a max width on the page or the
container, the containers will never actually scroll (but if we
do set a maximum page height they will, which is nice).

BUG=639795
==========

to

==========
MD Settings: Search Engines: Add scrollable behavior

This also adds a .scroll-container class which adds a 1px min-height
to ensure that scrollable iron-list elements update.

Note: Even though we are now using scrollable-behavior for search
engines, since we do not enforce a max width on the page or the
container, the containers will never actually scroll (but if we
do set a maximum page height they will, which is nice).

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

[stevenjb, 2016-09-16 15:54:28]

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

[stevenjb, 2016-09-16 15:54:29]

This fix will address the iron-list refresh issue for search engines. There are
other approaches, but this one seems to be the most robust (and is shared across
other iron lists).

[stevenjb, 2016-09-16 15:54:50]

On 2016/09/16 15:54:29, stevenjb wrote:
> This fix will address the iron-list refresh issue for search engines. There
are
> other approaches, but this one seems to be the most robust (and is shared
across
> other iron lists).

(search engines are the only iron-list that doesn't actually scroll).

[dpapad, 2016-09-17 00:05:51]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org

[dpapad, 2016-09-17 00:05:53]

+dbeam

@dbeam, @stevenjb: I was OOO while the iron-list was introduced more widely in
Settings, so please pardon me for not being on the same page, I am probably
missing context from previous discussions.

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
(left):

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.html:22:
<div id="container" class="layout vertical flex" scrollable>
This change should probably be on a different CL, or the CL description should
not mention only Search Engines.

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:38:
<div id="container" class="scroll-container" scrollable>
Is the id necessary? I don't see it used here, or in the behavior.

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.js
(right):

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engines_list.js:25:
this.updateScrollableContents();
I am not familiar with CrScrollableBehavior. By reading the documentation,
sounds useful (consistent styling, keyboard behavior), but it also seems it is
forcing the use of iron-list.

iron-list (per its own documentation) is useful for "infinite" lists. I think it
might be too big of a hammer to apply everywhere, instead of the simpler chunked
dom-repeat. The contents of updateScrollableContens() involves setInterval and I
have to admit that it scares me.

So my question is whether using iron-list for search engines is the best path
forward, and in general for any list that in the 90th percentile has at most 100
entries (those numbers are per my expectation, we can probably get actual
numbers on this). Same question holds for the "startup URLs". Do we really need
an infinite list for those?

[Dan Beam, 2016-09-17 00:54:27]

i don't really have much context on scrollablebehavior (haven't really followed
along) and don't know what benefit it offers over the dom-repeat or iron-list
polymer "primitives" it might wrap.

i do know that i have ~150 "other engines" in my search engines in my synced,
internal profile.  that's more than i expected and quite a few pages of
scrollback.  sure, it's way less than the number of cookies, but it might still
take a long time to render with a dom-repeat w/chunking.  we should get stats
with more significance.

generally: our team sure does want rules lately; i.e. when X when Y.

rules are helpful when they're really really clear; they remove thought /
cognitive load and increase cohesion (and I hate thinking).

rules are bad because assumptions might change and dogma may decrease
flexibility.

if scrollable behavior (which wraps iron-list) is like WAY heavier than
dom-repeat, we should think about this more.  do we have evidence for that? 
using a central implementation is nice for consistency and ease of updates.

if there's a big difference between an empty iron-list and empty dom-repeat (for
example), maybe we should only use the iron-list version for cases that have >N
items at 50%ile?  maybe we could compose the behavior such that you could create
an InfiniteScrollable and ShortScrollable?

either way, I don't think it's worthwhile to updating existing code without
clear benefit; i think it adds churn.  if you end up fixing the same bug
multiple times in multiple, duplicate implementations... that's usually the
impetus for me to combine.

^ my opinion

[Dan Beam, 2016-09-17 00:54:57]

On 2016/09/17 00:54:27, Dan Beam wrote:
> generally: our team sure does want rules lately; i.e. when X when Y.

when X then* Y

[stevenjb, 2016-09-19 16:19:15]

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.js
(right):

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engines_list.js:25:
this.updateScrollableContents();
On 2016/09/17 00:05:53, dpapad wrote:
> I am not familiar with CrScrollableBehavior. By reading the documentation,
> sounds useful (consistent styling, keyboard behavior), but it also seems it is
> forcing the use of iron-list.
> 
> iron-list (per its own documentation) is useful for "infinite" lists. I think
it
> might be too big of a hammer to apply everywhere, instead of the simpler
chunked
> dom-repeat. The contents of updateScrollableContens() involves setInterval and
I
> have to admit that it scares me.
> 
> So my question is whether using iron-list for search engines is the best path
> forward, and in general for any list that in the 90th percentile has at most
100
> entries (those numbers are per my expectation, we can probably get actual
> numbers on this). Same question holds for the "startup URLs". Do we really
need
> an infinite list for those?

We switched to iron-list primarily for its keyboard navigation behavior which is
fairly subtle and complex and not encapsulated into a separate behavior. I did
play with other options with the existing dom-repeat but did not have much
success with getting the behavior we desire.

We can discuss this more at today's standup.

[stevenjb, 2016-09-26 21:52:34]

Description was changed from

==========
MD Settings: Search Engines: Add scrollable behavior

This also adds a .scroll-container class which adds a 1px min-height
to ensure that scrollable iron-list elements update.

Note: Even though we are now using scrollable-behavior for search
engines, since we do not enforce a max width on the page or the
container, the containers will never actually scroll (but if we
do set a maximum page height they will, which is nice).

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

to

==========
MD Settings: Use scrollable behavior for all iron-list containers

This  adds a .scroll-container class which adds a 1px min-height
to ensure that scrollable iron-list elements update.

It also updates search_engines_page to use CrScrollableBehavior.

Note: Even though we are now using scrollable-behavior for search
engines, since we do not enforce a max height on the page or the
container, the containers will never actually scroll (but if we
do set a maximum page height they will, which is nice).

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

[stevenjb, 2016-09-26 21:58:19]

Ugh, I'd forgotten I had put this CL up.

I re-reviewed and tested this and I am still convinced this is the best approach
for updating iron-list.

PTAL

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
(left):

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.html:22:
<div id="container" class="layout vertical flex" scrollable>
On 2016/09/17 00:05:53, dpapad wrote:
> This change should probably be on a different CL, or the CL description should
> not mention only Search Engines.

The change is related so I will update the CL description.

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):

https://codereview.chromium.org/2338133011/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:38:
<div id="container" class="scroll-container" scrollable>
On 2016/09/17 00:05:53, dpapad wrote:
> Is the id necessary? I don't see it used here, or in the behavior.

Next line: 'scroll-target="container"'

[dpapad, 2016-09-26 22:02:14]

lgtm

[stevenjb, 2016-09-26 22:06:06]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-09-26 22:06:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 23:20:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 23:20:12]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2016-09-27 15:58:15]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-09-27 15:58:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 16:34:14]

Description was changed from

==========
MD Settings: Use scrollable behavior for all iron-list containers

This  adds a .scroll-container class which adds a 1px min-height
to ensure that scrollable iron-list elements update.

It also updates search_engines_page to use CrScrollableBehavior.

Note: Even though we are now using scrollable-behavior for search
engines, since we do not enforce a max height on the page or the
container, the containers will never actually scroll (but if we
do set a maximum page height they will, which is nice).

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

to

==========
MD Settings: Use scrollable behavior for all iron-list containers

This  adds a .scroll-container class which adds a 1px min-height
to ensure that scrollable iron-list elements update.

It also updates search_engines_page to use CrScrollableBehavior.

Note: Even though we are now using scrollable-behavior for search
engines, since we do not enforce a max height on the page or the
container, the containers will never actually scroll (but if we
do set a maximum page height they will, which is nice).

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

[commit-bot: I haz the power, 2016-09-27 16:34:16]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-27 16:36:30]

Description was changed from

==========
MD Settings: Use scrollable behavior for all iron-list containers

This  adds a .scroll-container class which adds a 1px min-height
to ensure that scrollable iron-list elements update.

It also updates search_engines_page to use CrScrollableBehavior.

Note: Even though we are now using scrollable-behavior for search
engines, since we do not enforce a max height on the page or the
container, the containers will never actually scroll (but if we
do set a maximum page height they will, which is nice).

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

to

==========
MD Settings: Use scrollable behavior for all iron-list containers

This  adds a .scroll-container class which adds a 1px min-height
to ensure that scrollable iron-list elements update.

It also updates search_engines_page to use CrScrollableBehavior.

Note: Even though we are now using scrollable-behavior for search
engines, since we do not enforce a max height on the page or the
container, the containers will never actually scroll (but if we
do set a maximum page height they will, which is nice).

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

Committed: https://crrev.com/2c697de24370a2156fe3a6d616d75105005d4029
Cr-Commit-Position: refs/heads/master@{#421232}
==========

[commit-bot: I haz the power, 2016-09-27 16:36:31]

Patchset 2 (id:??) landed as
https://crrev.com/2c697de24370a2156fe3a6d616d75105005d4029
Cr-Commit-Position: refs/heads/master@{#421232}