Fix issue caused by updating list that references global scroll target.

Fix issue caused by updating list that references global scroll target.

An iron-list in a subpage should only look at the global scroll target
when the subpage has been navigated to. Scroll position of the
basic/advanced page could be lost when an iron-list was updated if it
wasn't visible.

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

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

[hcarmona, 2016-12-02 18:20:23]

Description was changed from

==========
Fix issue caused by updating list that references global scroll target.

When an iron-list is updated but not visible, the entire view scrolls to
the top because it's the global scroll target. Updated so that an
iron-list that uses the global scroll target will only use it when it's
in view.

BUG=666582
==========

to

==========
Fix issue caused by updating list that references global scroll target.

When an iron-list is updated but not visible, the entire view scrolls to
the top because it's the global scroll target. Updated so that an
iron-list that uses the global scroll target will only use it when it's
in view.

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

[hcarmona, 2016-12-02 18:20:25]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 18:20:45]

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

[hcarmona, 2016-12-02 18:22:34]

hcarmona@chromium.org changed reviewers:
+ michaelpg@chromium.org

[hcarmona, 2016-12-02 18:22:35]

PTAL

[commit-bot: I haz the power, 2016-12-02 19:35:51]

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

[commit-bot: I haz the power, 2016-12-02 19:35:52]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-12-02 20:37:26]

> When an iron-list is updated but not visible, the entire view scrolls to
> the top because it's the global scroll target. Updated so that an
> iron-list that uses the global scroll target will only use it when it's
> in view.

I'm not 100% clear how this fixes that problem: after this patch, when
currentRoute is changed, the entire view still becomes the global scroll target
*before* the iron-list is visible or expanded. But if it works...

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/global_scroll_target_behavior.html
(right):

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/global_scroll_target_behavior.html:1: <link
rel="import" href="/route.html">
nit: chrome://resources before local imports

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/global_scroll_target_behavior.js (right):

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/global_scroll_target_behavior.js:7: * the
global scroll target. |scrollTarget| will be populated async by
update comments: when should scrollTarget be used vs activeTarget? and
activeRoute has to be overloaded

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/global_scroll_target_behavior.js:30:
activeTarget: {
nit: can you use something other than "active" for activeTarget/activeRoute?
because "activeTarget is populated only when the activeRoute is active" is
confusing.

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/global_scroll_target_behavior.js:79:
[settings.RouteObserverBehavior, settings.GlobalScrollTargetBehaviorImpl];
4-space indent

[hcarmona, 2016-12-02 22:48:51]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 22:49:27]

Dry run: 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-12-03 01:26:05]

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

[commit-bot: I haz the power, 2016-12-03 01:26:05]

Dry run: This issue passed the CQ dry run.

[hcarmona, 2016-12-05 15:57:17]

Description was changed from

==========
Fix issue caused by updating list that references global scroll target.

When an iron-list is updated but not visible, the entire view scrolls to
the top because it's the global scroll target. Updated so that an
iron-list that uses the global scroll target will only use it when it's
in view.

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

to

==========
Fix issue caused by updating list that references global scroll target.

An iron-list in a subpage should only look at the global scroll target
when the subpage has been navigated to. Scroll position of the
basic/advanced page could be lost when an iron-list was updated if it
wasn't visible.

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

[hcarmona, 2016-12-05 15:57:36]

On 2016/12/02 20:37:26, michaelpg wrote:
> > When an iron-list is updated but not visible, the entire view scrolls to
> > the top because it's the global scroll target. Updated so that an
> > iron-list that uses the global scroll target will only use it when it's
> > in view.
> 
> I'm not 100% clear how this fixes that problem: after this patch, when
> currentRoute is changed, the entire view still becomes the global scroll
target
> *before* the iron-list is visible or expanded. But if it works...

I've updated the description. WDYT?

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/global_scroll_target_behavior.html
(right):

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/global_scroll_target_behavior.html:1: <link
rel="import" href="/route.html">
On 2016/12/02 20:37:25, michaelpg wrote:
> nit: chrome://resources before local imports

Done.

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/global_scroll_target_behavior.js (right):

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/global_scroll_target_behavior.js:7: * the
global scroll target. |scrollTarget| will be populated async by
On 2016/12/02 20:37:26, michaelpg wrote:
> update comments: when should scrollTarget be used vs activeTarget? and
> activeRoute has to be overloaded

Done.

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/global_scroll_target_behavior.js:30:
activeTarget: {
On 2016/12/02 20:37:26, michaelpg wrote:
> nit: can you use something other than "active" for activeTarget/activeRoute?
> because "activeTarget is populated only when the activeRoute is active" is
> confusing.

Updated names to make it more apparent that these properties are intended for
subpages.

https://codereview.chromium.org/2538183008/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/global_scroll_target_behavior.js:79:
[settings.RouteObserverBehavior, settings.GlobalScrollTargetBehaviorImpl];
On 2016/12/02 20:37:26, michaelpg wrote:
> 4-space indent

Done.

[michaelpg, 2016-12-06 02:52:56]

lgtm

https://codereview.chromium.org/2538183008/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/global_scroll_target_behavior.js (right):

https://codereview.chromium.org/2538183008/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/global_scroll_target_behavior.js:66: * @param
{boolean} active
nit: @return

[michaelpg, 2016-12-06 02:53:14]

On 2016/12/05 15:57:36, hcarmona wrote:
> On 2016/12/02 20:37:26, michaelpg wrote:
> > > When an iron-list is updated but not visible, the entire view scrolls to
> > > the top because it's the global scroll target. Updated so that an
> > > iron-list that uses the global scroll target will only use it when it's
> > > in view.
> > 
> > I'm not 100% clear how this fixes that problem: after this patch, when
> > currentRoute is changed, the entire view still becomes the global scroll
> target
> > *before* the iron-list is visible or expanded. But if it works...
> 
> I've updated the description. WDYT?

works for me, thanks!

[hcarmona, 2016-12-06 17:35:15]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 17:35:55]

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

[hcarmona, 2016-12-06 17:40:53]

The CQ bit was unchecked by hcarmona@chromium.org

[hcarmona, 2016-12-06 17:40:53]

The CQ bit was checked by hcarmona@chromium.org

[hcarmona, 2016-12-06 17:40:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2538183008/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2016-12-06 17:41:31]

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-12-06 18:44:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481046053274190,
"parent_rev": "4d28daf37be21c0a43b70c32edeb4dc1194a2c91", "commit_rev":
"ac492cd5e4a2f941ce69dcf2b3f6c0af5e15c030"}

[commit-bot: I haz the power, 2016-12-06 18:45:12]

Description was changed from

==========
Fix issue caused by updating list that references global scroll target.

An iron-list in a subpage should only look at the global scroll target
when the subpage has been navigated to. Scroll position of the
basic/advanced page could be lost when an iron-list was updated if it
wasn't visible.

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

to

==========
Fix issue caused by updating list that references global scroll target.

An iron-list in a subpage should only look at the global scroll target
when the subpage has been navigated to. Scroll position of the
basic/advanced page could be lost when an iron-list was updated if it
wasn't visible.

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

[commit-bot: I haz the power, 2016-12-06 18:45:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-06 18:48:19]

Description was changed from

==========
Fix issue caused by updating list that references global scroll target.

An iron-list in a subpage should only look at the global scroll target
when the subpage has been navigated to. Scroll position of the
basic/advanced page could be lost when an iron-list was updated if it
wasn't visible.

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

to

==========
Fix issue caused by updating list that references global scroll target.

An iron-list in a subpage should only look at the global scroll target
when the subpage has been navigated to. Scroll position of the
basic/advanced page could be lost when an iron-list was updated if it
wasn't visible.

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

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

[commit-bot: I haz the power, 2016-12-06 18:48:20]

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