MD Settings: Save and restore scroll position in iron-list

MD Settings: Save and restore scroll position in iron-list

TODO: cr-scrollable-behavior needs tests.

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

Review-Url: https://codereview.chromium.org/2627373002
Cr-Commit-Position: refs/heads/master@{#447842}
Committed: https://chromium.googlesource.com/chromium/src/+/5c10efaf05bdde8188c305283ae207071674c0fd

[stevenjb, 2017-01-13 01:33:08]

Description was changed from

==========
MD Settings: Save and restore scroll item in iron-list

BUG=669248
==========

to

==========
MD Settings: Save and restore scroll item in iron-list

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

[stevenjb, 2017-01-13 01:34:35]

Description was changed from

==========
MD Settings: Save and restore scroll item in iron-list

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

to

==========
MD Settings: Save and restore scroll item in iron-list (WIP)

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

[stevenjb, 2017-01-13 01:38:41]

stevenjb@chromium.org changed reviewers:
+ dbeam@chromium.org, fukino@chromium.org

[stevenjb, 2017-01-13 01:38:42]

So, here is a fairly simpleminded approach that seems to work reasonably well.
It requires separating the device list used in iron-list from the input device
list, but in this case (and probably other cases) that's actually for the
better. I have only implemented this for bluetooth_device_dialog so far, and I
haven't tested it yet in a real world scenario (I will do that tomorrow, but I
wanted to throw this idea out there). LMKWYT

[fukino, 2017-01-13 10:12:50]

On 2017/01/13 01:38:42, stevenjb wrote:
> So, here is a fairly simpleminded approach that seems to work reasonably well.
> It requires separating the device list used in iron-list from the input device
> list, but in this case (and probably other cases) that's actually for the
> better. I have only implemented this for bluetooth_device_dialog so far, and I
> haven't tested it yet in a real world scenario (I will do that tomorrow, but I
> wanted to throw this idea out there). LMKWYT

Thank you! I like your approach.
However, I still see that the scroll position is moved a bit when this.splice()
is called in
https://codesearch.chromium.org/chromium/src/chrome/browser/resources/setting....
There seems to be a path where the scroll position is reset outside the period
between saveScroll() and restoreScroll().
If I find a cause, I'll let you know.

[stevenjb, 2017-01-16 00:25:12]

On 2017/01/13 10:12:50, fukino wrote:
> On 2017/01/13 01:38:42, stevenjb wrote:
> > So, here is a fairly simpleminded approach that seems to work reasonably
well.
> > It requires separating the device list used in iron-list from the input
device
> > list, but in this case (and probably other cases) that's actually for the
> > better. I have only implemented this for bluetooth_device_dialog so far, and
I
> > haven't tested it yet in a real world scenario (I will do that tomorrow, but
I
> > wanted to throw this idea out there). LMKWYT
> 
> Thank you! I like your approach.
> However, I still see that the scroll position is moved a bit when
this.splice()
> is called in
>
https://codesearch.chromium.org/chromium/src/chrome/browser/resources/setting....
> There seems to be a path where the scroll position is reset outside the period
> between saveScroll() and restoreScroll().
> If I find a cause, I'll let you know.

Using firstVisibleIndex has a couple of problems. It shifts slightly (as you
observed), and it fails completely if the first item is removed. I played with
converting the item to an index, but that still has the first problem.

Instead, I switched to using scrollTop, which seems very stable. I found some
edge cases when multiple updates occur between microtasks, so the solution is a
little more complex than I would prefer, but overall I don't think this is too
bad, and my manual tests have been pretty stable.

I will plan to add some tests for cr-scrollable-behvior in general on Tuesday
(Monday is a holiday in the US), but in the meanwhile LMKWYT.

[stevenjb, 2017-01-16 00:25:59]

Description was changed from

==========
MD Settings: Save and restore scroll item in iron-list (WIP)

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

to

==========
MD Settings: Save and restore scroll position in iron-list

TODO: cr-scrollable-behavior needs tests.

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

[stevenjb, 2017-01-23 19:12:59]

Ping

[stevenjb, 2017-01-23 20:12:51]

stevenjb@chromium.org changed reviewers:
- fukino@chromium.org

[Dan Beam, 2017-01-24 06:15:18]

https://codereview.chromium.org/2627373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js
(right):

https://codereview.chromium.org/2627373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_dialog.js:75:
});
can't we just make this do a diff instead?

iron-list is pretty good at handling change to specific items with the mutation
API (i.e. splice, set, append).  can we just add / remove and potentially keep
in sorted order for consistency?

we used to do something like this in downloads.

[stevenjb, 2017-01-24 17:12:11]

We can. It will be more complicated, but if you're confident it will be a
better result I can spend the time to write and test it. We have no
guarantees about sort order here. This solution seemed simpler.


On Jan 23, 2017 10:15 PM, <dbeam@chromium.org> wrote:

>
> https://codereview.chromium.org/2627373002/diff/40001/
> chrome/browser/resources/settings/bluetooth_page/
> bluetooth_device_dialog.js
> File
> chrome/browser/resources/settings/bluetooth_page/
> bluetooth_device_dialog.js
> (right):
>
> https://codereview.chromium.org/2627373002/diff/40001/
> chrome/browser/resources/settings/bluetooth_page/
> bluetooth_device_dialog.js#newcode75
> chrome/browser/resources/settings/bluetooth_page/
> bluetooth_device_dialog.js:75:
> });
> can't we just make this do a diff instead?
>
> iron-list is pretty good at handling change to specific items with the
> mutation API (i.e. splice, set, append). can we just add / remove and
> potentially keep in sorted order for consistency?
>
> we used to do something like this in downloads.
>
> https://codereview.chromium.org/2627373002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2017-01-25 20:06:39]

Patchset #4 (id:60001) has been deleted

[Dan Beam, 2017-01-31 04:06:44]

note: egarciad@ didn't get back to me today, but there's a bunch of other crazy
hacks we could do here that might be less error-prone, like:

var ORIGINAL_SCROLLTOP =
     Object.getOwnPropertyDescriptor(Element.prototype, 'scrollTop');

saveScroll: function(list) {
  Object.defineProperty(list.scrollTarget, 'scrollTop', {
    get: ORIGINAL_SCROLLTOP.get,
    set: function() { /* do nothing */ },
  });
},

restoreScroll: function(list) {
  Object.defineProperty(list.scrollTarget, 'scrollTop', ORIGINAL_SCROLLTOP);
},

or overriding list._resetScrollPosition:
https://github.com/PolymerElements/iron-list/blob/12ce24013db6ce6b04cd349acda...

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

https://codereview.chromium.org/2627373002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:104: saveScroll(list) {
drop ES6 stuff

https://codereview.chromium.org/2627373002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:108: let scroll =
target.scrollTop || -1;
why wouldn't we just do

list.prevScrollTop_ = list.scrollTarget.scrollTop

https://codereview.chromium.org/2627373002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:119: list.scroll(0,
scroll);
if (list.prevScrollTop_)
  list.scroll(0, list.prevScrollTop_);

[stevenjb, 2017-01-31 16:57:16]

On 2017/01/31 04:06:44, Dan Beam wrote:
> note: egarciad@ didn't get back to me today, but there's a bunch of other
crazy
> hacks we could do here that might be less error-prone, like:
> 
> var ORIGINAL_SCROLLTOP =
>      Object.getOwnPropertyDescriptor(Element.prototype, 'scrollTop');
> 
> saveScroll: function(list) {
>   Object.defineProperty(list.scrollTarget, 'scrollTop', {
>     get: ORIGINAL_SCROLLTOP.get,
>     set: function() { /* do nothing */ },
>   });
> },
> 
> restoreScroll: function(list) {
>   Object.defineProperty(list.scrollTarget, 'scrollTop', ORIGINAL_SCROLLTOP);
> },
> 
> or overriding list._resetScrollPosition:
>
https://github.com/PolymerElements/iron-list/blob/12ce24013db6ce6b04cd349acda...
> 
The first seems reasonable. I've never used defineProperty but it sounds
promising. I couldn't say if the solution is more or less hacky.

Overriding a method in iron-list would require wrapping it, right? I guess maybe
we could just replace it for the element? That also strays into an area I am
less familiar with, and iron-list is pretty complicated. Again, I couldn't say
if I think the solution would be better or not.

[stevenjb, 2017-01-31 18:43:09]

PTAL

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

https://codereview.chromium.org/2627373002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:104: saveScroll(list) {
On 2017/01/31 04:06:44, Dan Beam wrote:
> drop ES6 stuff

Done.

https://codereview.chromium.org/2627373002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:108: let scroll =
target.scrollTop || -1;
On 2017/01/31 04:06:44, Dan Beam wrote:
> why wouldn't we just do
> 
> list.prevScrollTop_ = list.scrollTarget.scrollTop

Code evolution. Done.

https://codereview.chromium.org/2627373002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:119: list.scroll(0,
scroll);
On 2017/01/31 04:06:44, Dan Beam wrote:
> if (list.prevScrollTop_)
>   list.scroll(0, list.prevScrollTop_);

Done.

[Dan Beam, 2017-02-01 06:13:18]

https://codereview.chromium.org/2627373002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html
(right):

https://codereview.chromium.org/2627373002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html:69:
<iron-list id="devices"  items="[[pairedDeviceList_]]"
nit: remove extra space

https://codereview.chromium.org/2627373002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right):

https://codereview.chromium.org/2627373002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:242: var
devices = this.$.devices;
what's the point of this var?  |this| wont change here, afaict...

https://codereview.chromium.org/2627373002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right):

https://codereview.chromium.org/2627373002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:105: let target =
list.scrollTarget;
no let until we vulcanize

https://codereview.chromium.org/2627373002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:106:
this.saveScrollTop_[target.id] = this.saveScrollTop_[target.id] || [];
just save on the list itself, i.e.

list.prevScrollTops_ = list.prevScrollTops_ || [];
list.prevScrollTops_.push(list.scrollTarget.scrollTop);

list.scroll(0, list.prevScrollTops_.pop());

also, do we really need an array?  why?

[stevenjb, 2017-02-01 18:36:19]

Patchset #6 (id:120001) has been deleted

[stevenjb, 2017-02-02 01:17:47]

https://codereview.chromium.org/2627373002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html
(right):

https://codereview.chromium.org/2627373002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html:69:
<iron-list id="devices"  items="[[pairedDeviceList_]]"
On 2017/02/01 06:13:17, Dan Beam wrote:
> nit: remove extra space

Done.

https://codereview.chromium.org/2627373002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right):

https://codereview.chromium.org/2627373002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:242: var
devices = this.$.devices;
On 2017/02/01 06:13:17, Dan Beam wrote:
> what's the point of this var?  |this| wont change here, afaict...

We use it twice below... Not sure I understand the question. I guess you're
saying that this.$.devices isn't long enough to merit the use of a local? Sounds
like a nit, but yeah, sure.

https://codereview.chromium.org/2627373002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right):

https://codereview.chromium.org/2627373002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:105: let target =
list.scrollTarget;
On 2017/02/01 06:13:17, Dan Beam wrote:
> no let until we vulcanize

Oops, missed that. Will vulcanize be part of the build step so that these will
eventually be caught?

https://codereview.chromium.org/2627373002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:106:
this.saveScrollTop_[target.id] = this.saveScrollTop_[target.id] || [];
On 2017/02/01 06:13:17, Dan Beam wrote:
> just save on the list itself, i.e.
> 
> list.prevScrollTops_ = list.prevScrollTops_ || [];
> list.prevScrollTops_.push(list.scrollTarget.scrollTop);
> 
> list.scroll(0, list.prevScrollTops_.pop());
> 
> also, do we really need an array?  why?

The C++ programmer in me cringes at decorating another class with it's own
cruft, but yeah, that works too, and I guess is more Javascripty? It does
certainly simplify the code.

We need a fifo here because we may get multiple updates in a single frame, so we
want to apply the last non zero update. It is also important that we don't apply
a stale update, so we can't just always use the latest non-zero value since we
wouldn't know when to clear it. I'll add a comment.

[Dan Beam, 2017-02-02 04:18:01]

lgtm

https://codereview.chromium.org/2627373002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_scrollable_behavior.js (right):

https://codereview.chromium.org/2627373002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_scrollable_behavior.js:105: let target =
list.scrollTarget;
On 2017/02/02 01:17:47, stevenjb wrote:
> On 2017/02/01 06:13:17, Dan Beam wrote:
> > no let until we vulcanize
> 
> Oops, missed that. Will vulcanize be part of the build step so that these will
> eventually be caught?

yes

https://codereview.chromium.org/2627373002/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list.html (right):

https://codereview.chromium.org/2627373002/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list.html:21: <iron-list
id="list" selection-enabled items="[[listItems_]]"
nit: networks?  something less generic than list?  (if you don't like networks
because this.networks or whatever)

https://codereview.chromium.org/2627373002/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list.js (right):

https://codereview.chromium.org/2627373002/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list.js:73:
this.saveScroll(list);
same nit, re: list vs this.$.list

this.$.list is easier to understand

list requires looking up what |list| means.

#justsayin #optional

https://codereview.chromium.org/2627373002/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list.js:75: var customItems =
this.customItems.slice();
are you making a defensive copy here with slice()?  what's the point?

[Dan Beam, 2017-02-02 04:18:36]

btw, egarciad@ says this will probably work OK if these items are the same size.
 meaning: the same height.

do we elide network names?  or do they wrap?

[stevenjb, 2017-02-02 18:10:24]

On 2017/02/02 04:18:36, Dan Beam (slow) wrote:
> btw, egarciad@ says this will probably work OK if these items are the same
size.
>  meaning: the same height.
> 
> do we elide network names?  or do they wrap?

We do whatever the default is, which is probably wrap? I will look into eliding
both network and bluetooth names in general, I think that would be better (So
far I haven't seen a name long enough for it to be an issue).

Also, this is not intended to be perfect. There will be some movement if an item
at the top of the list is removed, but that is unavoidable. In practice it does
what it needs to do, which is prevent the list from scrolling back to the top
every time an update occurs.

Thanks for the review. Tests forthcoming (I hope).

[stevenjb, 2017-02-02 18:20:17]

https://codereview.chromium.org/2627373002/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list.html (right):

https://codereview.chromium.org/2627373002/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list.html:21: <iron-list
id="list" selection-enabled items="[[listItems_]]"
On 2017/02/02 04:18:00, Dan Beam (slow) wrote:
> nit: networks?  something less generic than list?  (if you don't like networks
> because this.networks or whatever)

Yeah, I don't like shadowing a property, so went with networkList.

https://codereview.chromium.org/2627373002/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/network/cr_network_list.js (right):

https://codereview.chromium.org/2627373002/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list.js:73:
this.saveScroll(list);
On 2017/02/02 04:18:00, Dan Beam (slow) wrote:
> same nit, re: list vs this.$.list
> 
> this.$.list is easier to understand
> 
> list requires looking up what |list| means.
> 
> #justsayin #optional

Done.

https://codereview.chromium.org/2627373002/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/network/cr_network_list.js:75: var customItems =
this.customItems.slice();
On 2017/02/02 04:18:00, Dan Beam (slow) wrote:
> are you making a defensive copy here with slice()?  what's the point?

I was avoiding modifying the original list, although in practice that doesn't
really matter since the custom items never get re-ordered. That said, I have a
CL that gets rid of isFirstCustomItem (all items will have separators as per UX)
which also eliminates the extra slice. I will leave this as-is for now to
simplify merging.

[stevenjb, 2017-02-02 20:07:35]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-02-02 20:07:35]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2627373002/#ps180001
(title: "Nits")

[commit-bot: I haz the power, 2017-02-02 20:07:54]

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, 2017-02-02 21:22:04]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486066055579980,
"parent_rev": "89e4e70ff72647ccc29b884b0965112a7153a4cf", "commit_rev":
"5c10efaf05bdde8188c305283ae207071674c0fd"}

[commit-bot: I haz the power, 2017-02-02 21:22:40]

Description was changed from

==========
MD Settings: Save and restore scroll position in iron-list

TODO: cr-scrollable-behavior needs tests.

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

to

==========
MD Settings: Save and restore scroll position in iron-list

TODO: cr-scrollable-behavior needs tests.

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

Review-Url: https://codereview.chromium.org/2627373002
Cr-Commit-Position: refs/heads/master@{#447842}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c10efaf05bdde8188c305283ae2...
==========

[commit-bot: I haz the power, 2017-02-02 21:22:41]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/5c10efaf05bdde8188c305283ae2...