MD Settings: Introduce CrScrollableBehavior

MD Settings: Introduce CrScrollableBehavior

This CL:
* Defines a pattern for using <iron-list> for scrollable lists to get
  correct keyboard focus behavior.
* Introduces styling for scrolling containers.
* Introduces CrScrollableBehavior to set styling and update <iron-list>
* Converts startup-urls-page to use CrScrollableBehavior and <iron-list>

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

Committed: https://crrev.com/feeb114ee91d6bb97f162f2c05f66f6d3ef84c15
Cr-Commit-Position: refs/heads/master@{#413026}

[stevenjb, 2016-08-17 22:08:59]

Description was changed from

==========
MD Settings: Introduce CrScrollableBehavior

This CL:
* Defines a pattern for using <iron-list> for scrollable lists to get
  correct keyboard focus behavior.
* Introduces styling for scrolling containers.
* Introduces CrScrollableBehavior to set styling and update <iron-list>
* Converts startup-urls-page to use CrScrollableBehavior and <iron-list>

BUG=638377
==========

to

==========
MD Settings: Introduce CrScrollableBehavior

This CL:
* Defines a pattern for using <iron-list> for scrollable lists to get
  correct keyboard focus behavior.
* Introduces styling for scrolling containers.
* Introduces CrScrollableBehavior to set styling and update <iron-list>
* Converts startup-urls-page to use CrScrollableBehavior and <iron-list>

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

[stevenjb, 2016-08-17 22:24:41]

stevenjb@chromium.org changed reviewers:
+ dschuyler@chromium.org, hcarmona@chromium.org

[dschuyler, 2016-08-17 23:07:40]

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

https://codereview.chromium.org/2256753003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:52: }.bind(this));
When/where are these listeners be removed?

[hcarmona, 2016-08-17 23:43:23]

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

https://codereview.chromium.org/2256753003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:65:
ironList.notifyResize();
Are there non-IronListElements that need to be notified of resize?

https://codereview.chromium.org/2256753003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:71: * @private
Can we add an @param for |scrollable|?

[stevenjb, 2016-08-18 00:05:24]

PTAL

Note: I added the setTimeout because I noticed that sometimes, mysteriously, the
container was not showing up at all unless I resized the window or refreshed. I
could reproduce this by scrolling slowly down to the section (but not quickly
and not too slowly). After much experimentation, setTimeout seems to be robust.
Yay?

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

https://codereview.chromium.org/2256753003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:52: }.bind(this));
On 2016/08/17 23:07:40, dschuyler wrote:
> When/where are these listeners be removed?

Added detached(). (Which rarely gets called in practice, but it is still a good
idea, thanks). Also simplified this to simplify removing listeners.

https://codereview.chromium.org/2256753003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:65:
ironList.notifyResize();
On 2016/08/17 23:43:23, Hector Carmona wrote:
> Are there non-IronListElements that need to be notified of resize?

As far as I am aware, notfyResize is an iron-list specific feature:
https://elements.polymer-project.org/elements/iron-list#method-notifyResize

(Or, at least a IronResizableBehavior specific feature).

If we decide to use this model with anything other than iron-list, we could use
an attribute to identify the list element instead, but I think keeping this
specific to iron-list makes sense for now.

https://codereview.chromium.org/2256753003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:71: * @private
On 2016/08/17 23:43:23, Hector Carmona wrote:
> Can we add an @param for |scrollable|?

Effectively done; passing |event| to this now, with a @param comment.

[dschuyler, 2016-08-18 01:25:12]

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

https://codereview.chromium.org/2256753003/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:67: // Use setTimeout
to avoid initial render / sizing issues.
Would doWhenReady help?
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

The doWhenReady is preferred because it is more robust and 
helps highlight all the areas that are suffering from this
issue (which Michaelpg@ has been looking for a solution to).
Here's an example use:

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

[stevenjb, 2016-08-18 16:45:20]

On 2016/08/18 01:25:12, dschuyler wrote:
>
https://codereview.chromium.org/2256753003/diff/40001/ui/webui/resources/cr_e...
> File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right):
> 
>
https://codereview.chromium.org/2256753003/diff/40001/ui/webui/resources/cr_e...
> ui/webui/resources/cr_elements/cr_scrollable_behavior.js:67: // Use setTimeout
> to avoid initial render / sizing issues.
> Would doWhenReady help?
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...
> 
> The doWhenReady is preferred because it is more robust and 
> helps highlight all the areas that are suffering from this
> issue (which Michaelpg@ has been looking for a solution to).
> Here's an example use:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

I would suggest that if we want to use doWhenReady outside of
main_page_behavior.js we put it into the settings (or cr?) namespace in its own
module (or a generic utility one).

In the meanwhile, I changed this code to use setInterval instead of setTimeout.
If we find an alternative approach we'll want to audit any existing setInterval
/ setTimeout calls anyway.

[dschuyler, 2016-08-18 22:53:31]

lgtm

[stevenjb, 2016-08-18 23:39:47]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-08-18 23:40:48]

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-08-19 02:15:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-19 02:17:59]

Description was changed from

==========
MD Settings: Introduce CrScrollableBehavior

This CL:
* Defines a pattern for using <iron-list> for scrollable lists to get
  correct keyboard focus behavior.
* Introduces styling for scrolling containers.
* Introduces CrScrollableBehavior to set styling and update <iron-list>
* Converts startup-urls-page to use CrScrollableBehavior and <iron-list>

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

to

==========
MD Settings: Introduce CrScrollableBehavior

This CL:
* Defines a pattern for using <iron-list> for scrollable lists to get
  correct keyboard focus behavior.
* Introduces styling for scrolling containers.
* Introduces CrScrollableBehavior to set styling and update <iron-list>
* Converts startup-urls-page to use CrScrollableBehavior and <iron-list>

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

Committed: https://crrev.com/feeb114ee91d6bb97f162f2c05f66f6d3ef84c15
Cr-Commit-Position: refs/heads/master@{#413026}
==========

[commit-bot: I haz the power, 2016-08-19 02:18:00]

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