Make lists in the passwords section aware of the scroll target.

chrome/browser/resources/settings/global_scroll_target_behavior.js

Index: chrome/browser/resources/settings/global_scroll_target_behavior.js
diff --git a/chrome/browser/resources/settings/global_scroll_target_behavior.js b/chrome/browser/resources/settings/global_scroll_target_behavior.js
new file mode 100644
index 0000000000000000000000000000000000000000..0870c0f26cb989aa0c821306b05ccb8d20790fdc
--- /dev/null
+++ b/chrome/browser/resources/settings/global_scroll_target_behavior.js
@@ -0,0 +1,53 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview A helper object to be used when an iron-list needs to be aware
+ * of a global scroll target.
+ */
+
+/**
+ * Global object to hold the scroll target.
+ * @type {{scrollTarget: (HTMLElement|undefined)}}
+ */
+global.settingsScrollTargetData = {};
+
+/** @polymerBehavior */
+var GlobalScrollTargetBehavior = {
+  properties: {
+    /**
+     * Read only property for the scroll target.
+     * @type {HTMLElement}
+     */
+    scrollTarget: {
+      type: Object,
+      computed: 'getScrollTarget_(globalData_.scrollTarget)',
+    },
+
+    /**
+     * Reference to the global data. This allows Polymer style data binding.
+     * @type {{scrollTarget: (HTMLElement|undefined)}}
+     * @private
+     */
+    globalData_: Object,

[michaelpg, 2016/10/18 21:58:00]

i'd suggest:

globalData_: {
  type: Object,
  value: global.settingsScrollTargetData,
}

[hcarmona, 2016/10/19 18:53:34]

Acknowledged.

+  },
+
+  /** @override */
+  ready: function() {
+    this.globalData_ = global.settingsScrollTargetData;
+  },
+
+  /** @param {HTMLElement} value */
+  setGlobalScrollTarget: function(value) {
+    this.globalData_.scrollTarget = value;

[michaelpg, 2016/10/18 21:58:00]

This doesn't actually propagate. It may work, but don't do it.

Yes, all elements with this behavior, have a |this.globalData_| that references
the same object. The value will always be correct once it's set.

But: How does one element "know" when this.globalData_ has been changed by
another element? Even if we did:

  this.set('globalData_.scrollTarget', value);

Polymer is only notified about *one* path change, for that *one* element, and
recomputes scrollTarget for that one element. I believe for all other elements,
scrollTarget would not be recomputed.

(By the same logic, updates to scrollTarget are not magically propagated
either.)

So even though the shared object has changed, whatever uses that object (e.g.
iron-list) isn't notified of the change, so their observers won't fire, the list
won't resize, etc etc.

*If* we call setGlobalScrollTarget() on the first instantiated element with this
behavior, all other elements will read that value at creation time, so all is
well. *If* we never call the function again, so scrollTarget never changes,
everything will appear to work.

The first assumption could break depending on initialization order; if we load a
subpage at startup it would only find the empty object. The second, IDK, maybe
would break tests.

Possible fix:
  - call setGlobalScrollTarget in settings-ui ready()
  - make this.scrollTarget readOnly, and not computed, just undefined
  - in this behavior's attached(), do a setTimeout callback which sets the
readOnly property:
      // Once initialization is finished, settings-ui will have set the scroll
target.
      this._setScrollTarget(assert(this.globalData_.scrollTarget))

In that fix, you could replace this.globalData_.scrollTarget with a simple
variable you set here:
  (function() {
    var scrollTarget;
    var MyBehavior = {
      // the scrollTarget variable exists in the closure now
    };
   })();

If we could come up with a non-async way, that would be cool, but I'm not sure
how.

[hcarmona, 2016/10/19 18:53:34]

------------------------------------------------------------------------

On 2016/10/18 21:58:00, michaelpg wrote:
Arg! Ok, didn't realize that this was working by coincidence instead of
by design. Thanks for the thorough explanation.
Is there a reason we would want to be able to change the scroll target
after it's been set?
Agreed, having our code depend on the order of things is bad.
I had issues when I was first trying this out and set it in |ready|.
Done.
Not a fan of timeouts, they can be flaky. How does a promise look?
Wrapping this in a function didn't work for me. I get MyBehavior is
undefined when trying to use it. :-/

[michaelpg, 2016/10/19 19:50:08]

No, but we should make that assumption explicit (which the new patch does)
Cool, that probably would make testing easier too.
Yeah, my bad, we have to export MyBehavior. cr.define would work. With
a closure, settings-ui doesn't even need to have the behavior -- just
expose the function it needs to call. Would make the behavior simpler.
rough sketch:

cr.define('settings', function() {
  var scrollTargetResolver = new PromiseResolver();
  var GlobalScrollTargetBehavior = {
    properties: /* same as patch... */

    attached: function() {
      scrollTargetResolver.promise.then(
        this._setScrollTarget.bind(this));  // shortcut if you like
    },
  };
  var setGlobalScrollTarget = function(scrollTarget) {
    scrollTargetResolver.resolve(scrollTarget);
  };
  return {
    GlobalScrollTargetBehavior: GlobalScrollTargetBehavior,
    setGlobalScrollTarget: setGlobalScrollTarget,
  };
});

[hcarmona, 2016/10/20 14:27:43]

Done.

+  },
+
+  /**
+   * @return {HTMLElement|undefined}
+   * @private
+   */
+  getScrollTarget_: function() {
+    return this.globalData_.scrollTarget;
+  },
+};