Remove CrScrollableBehavior from Passwords Section.

Remove CrScrollableBehavior from Passwords Section.

Passwords section does not appear to need |CrScrollableBehavior| after
specifying the scroll target https://codereview.chromium.org/1914653002

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

Committed: https://crrev.com/6d67024bbbca61ddad11c5c242e30953af582637
Cr-Commit-Position: refs/heads/master@{#427426}

[hcarmona, 2016-10-24 21:46:30]

Description was changed from

==========
Remove CrScrollableBehavior from Passwords Section.

Scroll behavior does not appear to need |CrScrollableBehavior| after
specifying the scroll target https://codereview.chromium.org/1914653002

BUG=NONE
==========

to

==========
Remove CrScrollableBehavior from Passwords Section.

Scroll behavior does not appear to need |CrScrollableBehavior| after
specifying the scroll target https://codereview.chromium.org/1914653002

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

[hcarmona, 2016-10-24 21:46:34]

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

[commit-bot: I haz the power, 2016-10-24 21:47:16]

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

[hcarmona, 2016-10-24 21:48:51]

Description was changed from

==========
Remove CrScrollableBehavior from Passwords Section.

Scroll behavior does not appear to need |CrScrollableBehavior| after
specifying the scroll target https://codereview.chromium.org/1914653002

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

to

==========
Remove CrScrollableBehavior from Passwords Section.

Passwords section does not appear to need |CrScrollableBehavior| after
specifying the scroll target https://codereview.chromium.org/1914653002

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

[commit-bot: I haz the power, 2016-10-24 23:36:34]

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

[commit-bot: I haz the power, 2016-10-24 23:36:35]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[hcarmona, 2016-10-25 15:02:19]

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

[hcarmona, 2016-10-25 15:02:20]

I was thinking about this after yesterday's stand up. IIRC, the reason
we have this is because the list wasn't sized correctly on first load,
so the list wouldn't render. Setting the scroll target (in order to
get correct virtual scrolling) seems to also fix the initial size issue.

Is there something I'm forgetting, or would it be alright to remove
from here? Let me know what you think.

[stevenjb, 2016-10-25 16:26:07]

On 2016/10/25 15:02:20, Hector Carmona wrote:
> I was thinking about this after yesterday's stand up. IIRC, the reason
> we have this is because the list wasn't sized correctly on first load,
> so the list wouldn't render. Setting the scroll target (in order to
> get correct virtual scrolling) seems to also fix the initial size issue.
> 
> Is there something I'm forgetting, or would it be alright to remove
> from here? Let me know what you think.

I don't think we want to change these behaviors individually. If we decide we
don't want the top/bottom lines in scrollable behaviors or that we are certain
that we don't need updateScrollableContents(), we should change all scrollable
containers.

[hcarmona, 2016-10-25 17:44:43]

On 2016/10/25 16:26:07, stevenjb wrote:
> On 2016/10/25 15:02:20, Hector Carmona wrote:
> > I was thinking about this after yesterday's stand up. IIRC, the reason
> > we have this is because the list wasn't sized correctly on first load,
> > so the list wouldn't render. Setting the scroll target (in order to
> > get correct virtual scrolling) seems to also fix the initial size issue.
> > 
> > Is there something I'm forgetting, or would it be alright to remove
> > from here? Let me know what you think.
> 
> I don't think we want to change these behaviors individually. If we decide we
> don't want the top/bottom lines in scrollable behaviors or that we are certain
> that we don't need updateScrollableContents(), we should change all scrollable
> containers.

I think we do need to look at these individually because replacing
CrScrollableBehavior with GlobalScrollTargetBehavior only makes sense
for sub pages (where the scroll target is global).

In these cases the top/bottom bar also doesn't make sense because the
|scrollable| section should be the entire view.

We currently use iron-list in 8 places:

Places where we should keep CrScrollableBehavior:
- Startup URLs
- Bluetooth list
- Bluetooth devices
- Languages
- Dictionary (Should probably use the global scroll target b/c it's in
      a sub page)

Places where we should replace with GlobalScrollTargetBehavior:
- Passwords
- Search Engine list
- Search Engine extensions (I haven't tested this with 2 irons lists on
      the same page)

[stevenjb, 2016-10-25 17:48:44]

What is GlobalScrollTargetBehavior?



On Tue, Oct 25, 2016 at 10:44 AM, <hcarmona@chromium.org> wrote:

> On 2016/10/25 16:26:07, stevenjb wrote:
> > On 2016/10/25 15:02:20, Hector Carmona wrote:
> > > I was thinking about this after yesterday's stand up. IIRC, the reason
> > > we have this is because the list wasn't sized correctly on first load,
> > > so the list wouldn't render. Setting the scroll target (in order to
> > > get correct virtual scrolling) seems to also fix the initial size
> issue.
> > >
> > > Is there something I'm forgetting, or would it be alright to remove
> > > from here? Let me know what you think.
> >
> > I don't think we want to change these behaviors individually. If we
> decide we
> > don't want the top/bottom lines in scrollable behaviors or that we are
> certain
> > that we don't need updateScrollableContents(), we should change all
> scrollable
> > containers.
>
> I think we do need to look at these individually because replacing
> CrScrollableBehavior with GlobalScrollTargetBehavior only makes sense
> for sub pages (where the scroll target is global).
>
> In these cases the top/bottom bar also doesn't make sense because the
> |scrollable| section should be the entire view.
>
> We currently use iron-list in 8 places:
>
> Places where we should keep CrScrollableBehavior:
> - Startup URLs
> - Bluetooth list
> - Bluetooth devices
> - Languages
> - Dictionary (Should probably use the global scroll target b/c it's in
> a sub page)
>
> Places where we should replace with GlobalScrollTargetBehavior:
> - Passwords
> - Search Engine list
> - Search Engine extensions (I haven't tested this with 2 irons lists on
> the same page)
>
> https://codereview.chromium.org/2448653002/
>

-- 
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.

[hcarmona, 2016-10-25 18:01:38]

On 2016/10/25 17:48:44, stevenjb wrote:
> What is GlobalScrollTargetBehavior?

Defined here:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/global...

It provides a reference to the scroller in the header panel, which is
the viewable area. This lets the iron-list know how many virtual items
to render because its scroll target is not its immediate parent. It was
added recently to address iron-list scrolling issues when we have more
than 500 passwords.
See http://crrev.com/1914653002 for more details.

[stevenjb, 2016-10-25 18:11:44]

OK, I see. That makes sense. lgtm.

[hcarmona, 2016-10-25 18:19:13]

The CQ bit was checked by hcarmona@chromium.org

[commit-bot: I haz the power, 2016-10-25 18:20:06]

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-10-25 18:52:52]

Description was changed from

==========
Remove CrScrollableBehavior from Passwords Section.

Passwords section does not appear to need |CrScrollableBehavior| after
specifying the scroll target https://codereview.chromium.org/1914653002

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

to

==========
Remove CrScrollableBehavior from Passwords Section.

Passwords section does not appear to need |CrScrollableBehavior| after
specifying the scroll target https://codereview.chromium.org/1914653002

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

[commit-bot: I haz the power, 2016-10-25 18:52:53]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-25 19:09:18]

Description was changed from

==========
Remove CrScrollableBehavior from Passwords Section.

Passwords section does not appear to need |CrScrollableBehavior| after
specifying the scroll target https://codereview.chromium.org/1914653002

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

to

==========
Remove CrScrollableBehavior from Passwords Section.

Passwords section does not appear to need |CrScrollableBehavior| after
specifying the scroll target https://codereview.chromium.org/1914653002

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

Committed: https://crrev.com/6d67024bbbca61ddad11c5c242e30953af582637
Cr-Commit-Position: refs/heads/master@{#427426}
==========

[commit-bot: I haz the power, 2016-10-25 19:09:19]

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