MD Settings: Fix iron-list losing focus when items change.

MD Settings: Fix iron-list losing focus when items change.

This CL makes a patch to the iron-list javascript code directly, in order to fix issues with iron-list focus bugs when the backing items changes.

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

Review-Url: https://codereview.chromium.org/2794173002
Cr-Commit-Position: refs/heads/master@{#477356}
Committed: https://chromium.googlesource.com/chromium/src/+/3a7d55be3d2245f52c60b892c83ea17f62766b7c

[scottchen, 2017-04-03 23:49:57]

Description was changed from

==========
MD Settings: experimenting with iron-list bug

BUG=
==========

to

==========
MD Settings: experimenting with iron-list bug

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

[scottchen, 2017-04-10 23:17:19]

Description was changed from

==========
MD Settings: experimenting with iron-list bug

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

to

==========
MD Settings: experimenting with iron-list bug

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

[scottchen, 2017-04-27 17:30:35]

Description was changed from

==========
MD Settings: experimenting with iron-list bug

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

to

==========
MD Settings: Fix iron-list losing focus when items change.

This CL makes a patch to the iron-list javascript code directly, in order to fix
issues with iron-list focus bugs when the backing items changes.

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

[scottchen, 2017-05-04 00:28:18]

scottchen@chromium.org changed reviewers:
+ dbeam@chromium.org, dpapad@chromium.org, stevenjb@chromium.org

[scottchen, 2017-05-04 00:28:19]

Finally, after preemptively making a bunch of other CLs to see whether this fix
will suffice in all of setting's use-cases, here's the patch.

-------
NOTE: 

After this CL lands, we'll still have to do the following to fix all our
iron-list focus issues:

1) land the paper-icon-button -> -light CL (CL# 2848973003)
2) finish the CL to add the 'preserve-focus' flags. (CL# 2835133005, WIP)

I also opened a few CLs to prepare specific iron-lists to work correctly with
this patch:
- search-engine page's omnibox extensions list: CL# 2852413003
- spell-check custom words list: CL# 2858153002
- passwords list: CL# 2818283002 (by hcarmona@)

[stevenjb, 2017-05-09 17:14:00]

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
File
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js
(right):

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js:948:
}
Can we do something like:

var idx = Math.min(this.items.length - 1, lastFocusedIndex);
if (focusedElement == this._physicalItems[this._getPhysicalIndex(idx)]) {
  if (!this._isIndexRendered(idx))
    this.scrollToIndex(idx);
} else {
  this._focusPhysicalItem(idx);
}

That would avoid having to blur() then focus() the item if it's still present,
which might have side effects?

Also, it looks like _focusPhysicalItem already calls scrollToIndex?

[scottchen, 2017-05-22 22:57:31]

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
File
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js
(right):

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js:948:
}
On 2017/05/09 17:14:00, stevenjb wrote:
> Can we do something like:
> 
> var idx = Math.min(this.items.length - 1, lastFocusedIndex);
> if (focusedElement == this._physicalItems[this._getPhysicalIndex(idx)]) {
>   if (!this._isIndexRendered(idx))
>     this.scrollToIndex(idx);
> } else {
>   this._focusPhysicalItem(idx);
> }
> 
> That would avoid having to blur() then focus() the item if it's still present,
> which might have side effects?
> 
> Also, it looks like _focusPhysicalItem already calls scrollToIndex?

This would not work as intended:

  focusedElement == this._physicalItems[this._getPhysicalIndex(idx)]

Because sometimes "focusedElement" is an element within the list-item, not the
list item itself. (e.g. when exiting a cr-action-menu, we call anchor.focus(),
anchor being the paper-icon-button). In such cases, there's not a good way to
detect which list-item's descendants has focus.

However, dpapad@ did point out that a new pseudo :focus-within selector  is
implemented in tip of tree, so I will attempt to simplify this code using that.

[stevenjb, 2017-05-22 23:14:55]

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
File
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js
(right):

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js:948:
}
On 2017/05/22 22:57:31, scottchen wrote:
> On 2017/05/09 17:14:00, stevenjb wrote:
> > Can we do something like:
> > 
> > var idx = Math.min(this.items.length - 1, lastFocusedIndex);
> > if (focusedElement == this._physicalItems[this._getPhysicalIndex(idx)]) {
> >   if (!this._isIndexRendered(idx))
> >     this.scrollToIndex(idx);
> > } else {
> >   this._focusPhysicalItem(idx);
> > }
> > 
> > That would avoid having to blur() then focus() the item if it's still
present,
> > which might have side effects?
> > 
> > Also, it looks like _focusPhysicalItem already calls scrollToIndex?
> 
> This would not work as intended:
> 
>   focusedElement == this._physicalItems[this._getPhysicalIndex(idx)]
> 
> Because sometimes "focusedElement" is an element within the list-item, not the
> list item itself. (e.g. when exiting a cr-action-menu, we call anchor.focus(),
> anchor being the paper-icon-button). In such cases, there's not a good way to
> detect which list-item's descendants has focus.
> 
> However, dpapad@ did point out that a new pseudo :focus-within selector  is
> implemented in tip of tree, so I will attempt to simplify this code using
that.

It's been a while, but I think I was being hand-wavy with "if (focusedElement ==
this._physicalItems[this._getPhysicalIndex(idx)])"; 

I just meant to suggest that we should be able to detect whether the focused
item corresponds to "lastFocusedIndex" and just call scrollToIndex, instead of
the extra blur/focus.

That said if there's a better approach, by all means explore that!

[Dan Beam, 2017-05-23 16:29:23]

i'm not a particularly useful reviewer here.  i don't really know iron-list's
code well enough.

have you talked to keanulee@ or blasten@?

[Dan Beam, 2017-05-23 16:38:50]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[Dan Beam, 2017-05-23 16:38:51]

see also: https://codereview.chromium.org/2885723002/

[scottchen, 2017-05-23 21:05:48]

> have you talked to keanulee@ or blasten@?

Yeah, talked to keanulee@, he's aware of the forking we're doing and
acknowledged it's probably the easiest solution to our problem.

> see also: https://codereview.chromium.org/2885723002/

Thanks for pointing me to this. I can resolve the merge conflicts once that one
lands.

[scottchen, 2017-05-23 21:43:49]

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
File
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js
(right):

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js:948:
}
On 2017/05/22 23:14:54, stevenjb wrote:
> On 2017/05/22 22:57:31, scottchen wrote:
> > On 2017/05/09 17:14:00, stevenjb wrote:
> > > Can we do something like:
> > > 
> > > var idx = Math.min(this.items.length - 1, lastFocusedIndex);
> > > if (focusedElement == this._physicalItems[this._getPhysicalIndex(idx)]) {
> > >   if (!this._isIndexRendered(idx))
> > >     this.scrollToIndex(idx);
> > > } else {
> > >   this._focusPhysicalItem(idx);
> > > }
> > > 
> > > That would avoid having to blur() then focus() the item if it's still
> present,
> > > which might have side effects?
> > > 
> > > Also, it looks like _focusPhysicalItem already calls scrollToIndex?
> > 
> > This would not work as intended:
> > 
> >   focusedElement == this._physicalItems[this._getPhysicalIndex(idx)]
> > 
> > Because sometimes "focusedElement" is an element within the list-item, not
the
> > list item itself. (e.g. when exiting a cr-action-menu, we call
anchor.focus(),
> > anchor being the paper-icon-button). In such cases, there's not a good way
to
> > detect which list-item's descendants has focus.
> > 
> > However, dpapad@ did point out that a new pseudo :focus-within selector  is
> > implemented in tip of tree, so I will attempt to simplify this code using
> that.
> 
> It's been a while, but I think I was being hand-wavy with "if (focusedElement
==
> this._physicalItems[this._getPhysicalIndex(idx)])"; 
> 
> I just meant to suggest that we should be able to detect whether the focused
> item corresponds to "lastFocusedIndex" and just call scrollToIndex, instead of
> the extra blur/focus.

focusedItem at this point is already reset to 0 (this is what caused the bug in
the first place), so it doesn't correspond to anything useful at this point.

focusedElement is just some dom element that was focused by javascript, it
doesn't have much to do with what iron-list thinks the focusedItem is, so
scrolling to the item that contains it isn't really helpful if iron-list still
thinks focusedItem = items[0]. 

This is also why its required to call this._focusPhysicalItem() to actually
correctly set iron-list's notion of what the focusedItem is. 

focusedElement is only being retrieved for the sole purpose of calling blur() on
it, which is required just because paper- controls break if you try to focus it
while its already focused.


> 
> That said if there's a better approach, by all means explore that!

I looked at :focus-within more, and unfortunately that wouldn't benefit us in
this case, because I would still need the focusedElement to call blur() on it.

[stevenjb, 2017-05-23 21:53:47]

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
File
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js
(right):

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js:948:
}
On 2017/05/23 21:43:49, scottchen wrote:
> On 2017/05/22 23:14:54, stevenjb wrote:
> > On 2017/05/22 22:57:31, scottchen wrote:
> > > On 2017/05/09 17:14:00, stevenjb wrote:
> > > > Can we do something like:
> > > > 
> > > > var idx = Math.min(this.items.length - 1, lastFocusedIndex);
> > > > if (focusedElement == this._physicalItems[this._getPhysicalIndex(idx)])
{
> > > >   if (!this._isIndexRendered(idx))
> > > >     this.scrollToIndex(idx);
> > > > } else {
> > > >   this._focusPhysicalItem(idx);
> > > > }
> > > > 
> > > > That would avoid having to blur() then focus() the item if it's still
> > present,
> > > > which might have side effects?
> > > > 
> > > > Also, it looks like _focusPhysicalItem already calls scrollToIndex?
> > > 
> > > This would not work as intended:
> > > 
> > >   focusedElement == this._physicalItems[this._getPhysicalIndex(idx)]
> > > 
> > > Because sometimes "focusedElement" is an element within the list-item, not
> the
> > > list item itself. (e.g. when exiting a cr-action-menu, we call
> anchor.focus(),
> > > anchor being the paper-icon-button). In such cases, there's not a good way
> to
> > > detect which list-item's descendants has focus.
> > > 
> > > However, dpapad@ did point out that a new pseudo :focus-within selector 
is
> > > implemented in tip of tree, so I will attempt to simplify this code using
> > that.
> > 
> > It's been a while, but I think I was being hand-wavy with "if
(focusedElement
> ==
> > this._physicalItems[this._getPhysicalIndex(idx)])"; 
> > 
> > I just meant to suggest that we should be able to detect whether the focused
> > item corresponds to "lastFocusedIndex" and just call scrollToIndex, instead
of
> > the extra blur/focus.
> 
> focusedItem at this point is already reset to 0 (this is what caused the bug
in
> the first place), so it doesn't correspond to anything useful at this point.
> 
> focusedElement is just some dom element that was focused by javascript, it
> doesn't have much to do with what iron-list thinks the focusedItem is, so
> scrolling to the item that contains it isn't really helpful if iron-list still
> thinks focusedItem = items[0]. 
> 
> This is also why its required to call this._focusPhysicalItem() to actually
> correctly set iron-list's notion of what the focusedItem is. 
> 
> focusedElement is only being retrieved for the sole purpose of calling blur()
on
> it, which is required just because paper- controls break if you try to focus
it
> while its already focused.
> 
> 
> > 
> > That said if there's a better approach, by all means explore that!
> 
> I looked at :focus-within more, and unfortunately that wouldn't benefit us in
> this case, because I would still need the focusedElement to call blur() on it.

OK, lgtm then

[dpapad, 2017-05-23 22:19:43]

LGTM

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
File
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js
(right):

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js:905:
var rendering = ['items', 'items.splices'].includes(change.path);
Nit (optional): Maybe use a regex instead (to avoid creating an array)?

var rendering = /^items(\.splices){0,1}$/.test(change.path)

[scottchen, 2017-05-24 20:56:45]

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

[commit-bot: I haz the power, 2017-05-24 20:57:41]

Dry run: CQ is trying da patch.

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

[scottchen, 2017-05-24 21:34:35]

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
File
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js
(right):

https://codereview.chromium.org/2794173002/diff/160001/third_party/polymer/v1...
third_party/polymer/v1_0/components-chromium/iron-list/iron-list-extracted.js:905:
var rendering = ['items', 'items.splices'].includes(change.path);
On 2017/05/23 22:19:43, dpapad wrote:
> Nit (optional): Maybe use a regex instead (to avoid creating an array)?
> 
> var rendering = /^items(\.splices){0,1}$/.test(change.path)

Done.

[commit-bot: I haz the power, 2017-05-24 22:41:46]

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

[commit-bot: I haz the power, 2017-05-24 22:41:47]

Dry run: This issue passed the CQ dry run.

[scottchen, 2017-06-01 21:05:51]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-06-01 21:05:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2794173002/#ps220001
(title: "merge")

[commit-bot: I haz the power, 2017-06-01 21:06:37]

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-06-01 22:53:34]

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

[commit-bot: I haz the power, 2017-06-01 22:53:35]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[scottchen, 2017-06-01 22:54:06]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-06-01 22:54:43]

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-06-02 00:26:01]

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

[commit-bot: I haz the power, 2017-06-02 00:26:02]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[scottchen, 2017-06-05 21:43:44]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-06-05 21:44:45]

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-06-05 23:04:58]

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

[commit-bot: I haz the power, 2017-06-05 23:04:59]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[scottchen, 2017-06-06 01:09:28]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-06-06 01:10:03]

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-06-06 02:46:13]

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

[commit-bot: I haz the power, 2017-06-06 02:46:14]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[scottchen, 2017-06-06 17:34:58]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-06-06 17:35:28]

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-06-06 18:53:37]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496770498430610,
"parent_rev": "1afcf3a6e926552729262f213769a6cbde2a0839", "commit_rev":
"3a7d55be3d2245f52c60b892c83ea17f62766b7c"}

[commit-bot: I haz the power, 2017-06-06 18:53:47]

Description was changed from

==========
MD Settings: Fix iron-list losing focus when items change.

This CL makes a patch to the iron-list javascript code directly, in order to fix
issues with iron-list focus bugs when the backing items changes.

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

to

==========
MD Settings: Fix iron-list losing focus when items change.

This CL makes a patch to the iron-list javascript code directly, in order to fix
issues with iron-list focus bugs when the backing items changes.

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

Review-Url: https://codereview.chromium.org/2794173002
Cr-Commit-Position: refs/heads/master@{#477356}
Committed:
https://chromium.googlesource.com/chromium/src/+/3a7d55be3d2245f52c60b892c83e...
==========

[commit-bot: I haz the power, 2017-06-06 18:53:50]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/3a7d55be3d2245f52c60b892c83e...