MD Settings: long dialog body should have overscroll line.

MD Settings: long dialog body should have overscroll line.

BUG=684152, 595804
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2702523005
Cr-Original-Commit-Position: refs/heads/master@{#460937}
Committed: https://chromium.googlesource.com/chromium/src/+/27793e2b00cd4f06e24cde9599be5693006ed02b
Review-Url: https://codereview.chromium.org/2702523005
Cr-Commit-Position: refs/heads/master@{#461264}
Committed: https://chromium.googlesource.com/chromium/src/+/a4c0720e53ea714a31f27ee2a8c4bf6ec2841a34

[scottchen, 2017-02-17 22:37:02]

Description was changed from

==========
MD Settings: long add-address dialog body should have overscroll line.

BUG=684152
==========

to

==========
MD Settings: long add-address dialog body should have overscroll line.

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

[scottchen, 2017-02-17 22:40:03]

scottchen@chromium.org changed reviewers:
+ dpapad@chromium.org

[scottchen, 2017-02-17 22:40:04]

demo video:
https://drive.google.com/open?id=0ByIc0PEad7DmZm1uSGg0dWdFNVE

https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html
(right):

https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html:59:
border-top-color: transparent;
This is to prevent double-border, since the dialog title already has a top
border all the time.

[dpapad, 2017-02-18 00:37:17]

https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
(right):

https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:17:
CrScrollableBehavior
CrScrollableBehavior as advertized is meant to be used with iron-list (and has a
LOT of otherwise unnecessary code, for example this call to setInterval,
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scroll...).

AddressEditDialog is not using iron-list. Is there a better (and lighter) way to
achieve the desired overscroll behavior?

[scottchen, 2017-02-18 01:18:57]

https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
(right):

https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:17:
CrScrollableBehavior
On 2017/02/18 00:37:17, dpapad wrote:
> CrScrollableBehavior as advertized is meant to be used with iron-list (and has
a
> LOT of otherwise unnecessary code, for example this call to setInterval,
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scroll...).
> 
> AddressEditDialog is not using iron-list. Is there a better (and lighter) way
to
> achieve the desired overscroll behavior?

I could copy just the scroll-class-attaching part of the javascript over, but
that's going to duplicate some good chunk of code.. thoughts?

[dpapad, 2017-02-21 18:49:26]

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

[dpapad, 2017-02-21 18:49:32]

On 2017/02/18 at 01:18:57, scottchen wrote:
>
https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
> File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
(right):
> 
>
https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
>
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:17:
CrScrollableBehavior
> On 2017/02/18 00:37:17, dpapad wrote:
> > CrScrollableBehavior as advertized is meant to be used with iron-list (and
has a
> > LOT of otherwise unnecessary code, for example this call to setInterval,
> >
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scroll...).
> > 
> > AddressEditDialog is not using iron-list. Is there a better (and lighter)
way to
> > achieve the desired overscroll behavior?
> 
> I could copy just the scroll-class-attaching part of the javascript over, but
that's going to duplicate some good chunk of code.. thoughts?

+stevenjb Who originally authored cr_scrollable_behavior.js.

Couple of questions:
 1) Tried the patch locally. The bottom border shows up while scrolling, but
when I reach the bottom of the scroll goes away which looked odd. Is that
intended?
 2) Adding a scroll handler just to display a border on a certain condition
seems more trouble than worth IMO (thinking complexity + performance). I would
rather
    remove that behavior from iron-lists too, than adding it in more places.
Have we considered a simpler visual solution? How about adding a border between
the
    buttons and the dialog body in all cases (regardless of how tall the dialog
is, or whether a scrollbar exists), see example http://imgur.com/a/kdIuT.

If we really really want the showing/hiding logic of the bottom border as done
by cr_scrollable_behawvior.js, I leave it to @stevenjb to comment whether it is
OK
to reuse that logic for UIs that don't use iron-lists.

[stevenjb, 2017-02-21 19:38:13]

https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
(right):

https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:17:
CrScrollableBehavior
On 2017/02/18 01:18:56, scottchen wrote:
> On 2017/02/18 00:37:17, dpapad wrote:
> > CrScrollableBehavior as advertized is meant to be used with iron-list (and
has
> a
> > LOT of otherwise unnecessary code, for example this call to setInterval,
> >
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scroll...).
> > 
> > AddressEditDialog is not using iron-list. Is there a better (and lighter)
way
> to
> > achieve the desired overscroll behavior?
> 
> I could copy just the scroll-class-attaching part of the javascript over, but
> that's going to duplicate some good chunk of code.. thoughts?

I think that using CrScrollableBehavior here is fine. 

The referenced code is only called if updateScrollableContents() is called
explicitly. The code in ready() is necessary for the use case here, so there is
minimal extra overhead in including this behavior as-is.

We could update the behavior comment to say that it is designed to provide
paper-dialog-scrollable style borders for any scrollable container, with
additional helper methods to work around known issues with iron-list specific
behaviors.

[Dan Beam, 2017-02-21 20:06:13]

On 2017/02/21 18:49:32, dpapad wrote:
> On 2017/02/18 at 01:18:57, scottchen wrote:
> >
>
https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
> > File
>
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
> (right):
> > 
> >
>
https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
> >
>
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:17:
> CrScrollableBehavior
> > On 2017/02/18 00:37:17, dpapad wrote:
> > > CrScrollableBehavior as advertized is meant to be used with iron-list (and
> has a
> > > LOT of otherwise unnecessary code, for example this call to setInterval,
> > >
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scroll...).
> > > 
> > > AddressEditDialog is not using iron-list. Is there a better (and lighter)
> way to
> > > achieve the desired overscroll behavior?
> > 
> > I could copy just the scroll-class-attaching part of the javascript over,
but
> that's going to duplicate some good chunk of code.. thoughts?
> 
> +stevenjb Who originally authored cr_scrollable_behavior.js.
> 
> Couple of questions:
>  1) Tried the patch locally. The bottom border shows up while scrolling, but
> when I reach the bottom of the scroll goes away which looked odd. Is that
> intended?
>  2) Adding a scroll handler just to display a border on a certain condition
> seems more trouble than worth IMO (thinking complexity + performance). I would
> rather
>     remove that behavior from iron-lists too, than adding it in more places.
> Have we considered a simpler visual solution? How about adding a border
between
> the
>     buttons and the dialog body in all cases (regardless of how tall the
dialog
> is, or whether a scrollbar exists), see example http://imgur.com/a/kdIuT.
> 
> If we really really want the showing/hiding logic of the bottom border as done
> by cr_scrollable_behawvior.js, I leave it to @stevenjb to comment whether it
is
> OK
> to reuse that logic for UIs that don't use iron-lists.

that's not how material design works.  there's a border if there's more content.
 that's an indicator of "scrolling is possible".  it's a bug that we DON'T show
a border currently

[scottchen, 2017-02-21 20:13:12]

On 2017/02/21 20:06:13, Dan Beam wrote:
> On 2017/02/21 18:49:32, dpapad wrote:
> > On 2017/02/18 at 01:18:57, scottchen wrote:
> > >
> >
>
https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
> > > File
> >
>
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
> > >
> >
>
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:17:
> > CrScrollableBehavior
> > > On 2017/02/18 00:37:17, dpapad wrote:
> > > > CrScrollableBehavior as advertized is meant to be used with iron-list
(and
> > has a
> > > > LOT of otherwise unnecessary code, for example this call to setInterval,
> > > >
> >
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scroll...).
> > > > 
> > > > AddressEditDialog is not using iron-list. Is there a better (and
lighter)
> > way to
> > > > achieve the desired overscroll behavior?
> > > 
> > > I could copy just the scroll-class-attaching part of the javascript over,
> but
> > that's going to duplicate some good chunk of code.. thoughts?
> > 
> > +stevenjb Who originally authored cr_scrollable_behavior.js.
> > 
> > Couple of questions:
> >  1) Tried the patch locally. The bottom border shows up while scrolling, but
> > when I reach the bottom of the scroll goes away which looked odd. Is that
> > intended?
> >  2) Adding a scroll handler just to display a border on a certain condition
> > seems more trouble than worth IMO (thinking complexity + performance). I
would
> > rather
> >     remove that behavior from iron-lists too, than adding it in more places.
> > Have we considered a simpler visual solution? How about adding a border
> between
> > the
> >     buttons and the dialog body in all cases (regardless of how tall the
> dialog
> > is, or whether a scrollbar exists), see example http://imgur.com/a/kdIuT.
> > 
> > If we really really want the showing/hiding logic of the bottom border as
done
> > by cr_scrollable_behawvior.js, I leave it to @stevenjb to comment whether it
> is
> > OK
> > to reuse that logic for UIs that don't use iron-lists.
> 
> that's not how material design works.  there's a border if there's more
content.
>  that's an indicator of "scrolling is possible".  it's a bug that we DON'T
show
> a border currently

dbeam@ I'm not sure what "that's not how MD works" is referring to, could you
clarify please? Thanks!

[scottchen, 2017-02-21 20:15:15]

On 2017/02/21 18:49:32, dpapad wrote:
> On 2017/02/18 at 01:18:57, scottchen wrote:
> >
>
https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
> > File
>
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
> (right):
> > 
> >
>
https://codereview.chromium.org/2702523005/diff/1/chrome/browser/resources/se...
> >
>
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:17:
> CrScrollableBehavior
> > On 2017/02/18 00:37:17, dpapad wrote:
> > > CrScrollableBehavior as advertized is meant to be used with iron-list (and
> has a
> > > LOT of otherwise unnecessary code, for example this call to setInterval,
> > >
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scroll...).
> > > 
> > > AddressEditDialog is not using iron-list. Is there a better (and lighter)
> way to
> > > achieve the desired overscroll behavior?
> > 
> > I could copy just the scroll-class-attaching part of the javascript over,
but
> that's going to duplicate some good chunk of code.. thoughts?
> 
> +stevenjb Who originally authored cr_scrollable_behavior.js.
> 
> Couple of questions:
>  1) Tried the patch locally. The bottom border shows up while scrolling, but
> when I reach the bottom of the scroll goes away which looked odd. Is that
> intended?

Yeah, that's the intended behavior - border indicates that there's more stuff
below that fold.

>  2) Adding a scroll handler just to display a border on a certain condition
> seems more trouble than worth IMO (thinking complexity + performance). I would
> rather
>     remove that behavior from iron-lists too, than adding it in more places.
> Have we considered a simpler visual solution? How about adding a border
between
> the
>     buttons and the dialog body in all cases (regardless of how tall the
dialog
> is, or whether a scrollbar exists), see example http://imgur.com/a/kdIuT.
> 
> If we really really want the showing/hiding logic of the bottom border as done
> by cr_scrollable_behawvior.js, I leave it to @stevenjb to comment whether it
is
> OK
> to reuse that logic for UIs that don't use iron-lists.

[dpapad, 2017-02-21 20:16:41]

> that's not how material design works.  there's a border if there's more
content.  that's an indicator of "scrolling is possible".  it's a bug that we
DON'T show a border currently

So, should the proper fix be within cr-dialog itself (if possible), and not just
a one-off fix for this dialog then?

Using a bottom border as an indicator that there is "more to scroll" is not
intuitive IMO. I really don't think this MD guideline is as relevant in desktop,
at least in platforms where the scrollbars and scrollbar buttons are visible
(everything except Mac?)

I would not care as much if there was a CSS pseudo selector to achieve this,
like div:scrolled-to-bottom, but adding a JS scroll listener executing on every
scroll event to determine if the border should be shown/hidden sounds expensive
and not in the best interest of the user (similar to adding a delay before
dismissing a UI so that the user has time to see a ripple).

[dpapad, 2017-02-22 00:53:19]

FYI, https://bugs.chromium.org/p/chromium/issues/detail?id=595804 also refers to
what this CL is attempting to address.

[scottchen, 2017-02-22 18:35:10]

On 2017/02/22 00:53:19, dpapad wrote:
> FYI, https://bugs.chromium.org/p/chromium/issues/detail?id=595804 also refers
to
> what this CL is attempting to address.

I'm not sure if we should apply this to all cr-dialogs - I think this should be
applied case-by-case, since 1) I think not all dialogs want to be scrollable by
default - I could see cases where the dialog would rather be a bit longer and
fits everything in one view, 2) it has to be applied to the fixed-height
container, which is provided by the consuming page. 

Thoughts?

[dpapad, 2017-02-22 19:02:15]

On 2017/02/22 at 18:35:10, scottchen wrote:
> On 2017/02/22 00:53:19, dpapad wrote:
> > FYI, https://bugs.chromium.org/p/chromium/issues/detail?id=595804 also
refers to
> > what this CL is attempting to address.
> 
> I'm not sure if we should apply this to all cr-dialogs - I think this should
be applied case-by-case, since 1) I think not all dialogs want to be scrollable
by default - I could see cases where the dialog would rather be a bit longer and
fits everything in one view, 2) it has to be applied to the fixed-height
container, which is provided by the consuming page. 
> 
> Thoughts?

The fixed height can be passed to the dialog's body-container [1] with a CSS
mixin, which might be more correct than adding the border on .body from the
client side instead.

Having said that, I would first like to clarify whether we are 100% committed to
the MD guide about using a border to indicate "more content is visible". For
example we already don't do that in the "Clear browsing data" dialog
(http://imgur.com/a/FitL5), in which we always display a top and border when a
scrollbar is visible. In other words we don't remove the border when the user
scrolls all the way to the top/bottom. This seems simpler to me, does not
require a scroll listener, and we already do it, which is what I am proposing
instead. 

[1]
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog...

[scottchen, 2017-02-22 19:33:38]

On 2017/02/22 19:02:15, dpapad wrote:
> On 2017/02/22 at 18:35:10, scottchen wrote:
> > On 2017/02/22 00:53:19, dpapad wrote:
> > > FYI, https://bugs.chromium.org/p/chromium/issues/detail?id=595804 also
> refers to
> > > what this CL is attempting to address.
> > 
> > I'm not sure if we should apply this to all cr-dialogs - I think this should
> be applied case-by-case, since 1) I think not all dialogs want to be
scrollable
> by default - I could see cases where the dialog would rather be a bit longer
and
> fits everything in one view, 2) it has to be applied to the fixed-height
> container, which is provided by the consuming page. 
> > 
> > Thoughts?
> 
> The fixed height can be passed to the dialog's body-container [1] with a CSS
> mixin, which might be more correct than adding the border on .body from the
> client side instead.
> 
> Having said that, I would first like to clarify whether we are 100% committed
to
> the MD guide about using a border to indicate "more content is visible". For
> example we already don't do that in the "Clear browsing data" dialog
> (http://imgur.com/a/FitL5), in which we always display a top and border when a
> scrollbar is visible. In other words we don't remove the border when the user
> scrolls all the way to the top/bottom. This seems simpler to me, does not
> require a scroll listener, and we already do it, which is what I am proposing
> instead. 
> 
> [1]
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog...

I'll put this CL on hold until I have a discussion with bettes@ on which visual
we want to be consistent with, since currently the border's all over the place
(some have conditional bottom borders, some have persistent border above the
buttons, and some have persistent border under the button and above the footer.)

[scottchen, 2017-03-24 00:41:53]

On 2017/02/22 19:33:38, scottchen wrote:
> On 2017/02/22 19:02:15, dpapad wrote:
> > On 2017/02/22 at 18:35:10, scottchen wrote:
> > > On 2017/02/22 00:53:19, dpapad wrote:
> > > > FYI, https://bugs.chromium.org/p/chromium/issues/detail?id=595804 also
> > refers to
> > > > what this CL is attempting to address.
> > > 
> > > I'm not sure if we should apply this to all cr-dialogs - I think this
should
> > be applied case-by-case, since 1) I think not all dialogs want to be
> scrollable
> > by default - I could see cases where the dialog would rather be a bit longer
> and
> > fits everything in one view, 2) it has to be applied to the fixed-height
> > container, which is provided by the consuming page. 
> > > 
> > > Thoughts?
> > 
> > The fixed height can be passed to the dialog's body-container [1] with a CSS
> > mixin, which might be more correct than adding the border on .body from the
> > client side instead.
> > 
> > Having said that, I would first like to clarify whether we are 100%
committed
> to
> > the MD guide about using a border to indicate "more content is visible". For
> > example we already don't do that in the "Clear browsing data" dialog
> > (http://imgur.com/a/FitL5), in which we always display a top and border when
a
> > scrollbar is visible. In other words we don't remove the border when the
user
> > scrolls all the way to the top/bottom. This seems simpler to me, does not
> > require a scroll listener, and we already do it, which is what I am
proposing
> > instead. 
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog...
> 
> I'll put this CL on hold until I have a discussion with bettes@ on which
visual
> we want to be consistent with, since currently the border's all over the place
> (some have conditional bottom borders, some have persistent border above the
> buttons, and some have persistent border under the button and above the
footer.)

The bug is now marked as beta-blocker (see:
https://bugs.chromium.org/p/chromium/issues/detail?id=595804) and had more time
for feedback. Picking this CL back up to do the same for other dialogs.

[dpapad, 2017-03-25 01:34:00]

> The bug is now marked as beta-blocker (see:
https://bugs.chromium.org/p/chromium/issues/detail?id=595804) and had more time
for feedback. Picking this CL back up to do the same for other dialogs.

I managed to get a very promising solution working, with IntersectionObserver,
see https://jsfiddle.net/zjL8709b/2/. Let's examine next week whether  it is
possible to use the same approach for all 3 of our cases
1) Removing paper-header-panel (issue 699302).
2) Adding top+bottom lines in dialogs (this CL).
3) Adding top+bottom lines in iron-lists (currently implemented by
CrScrollableBehavior).

[scottchen, 2017-03-29 00:58:42]

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

[commit-bot: I haz the power, 2017-03-29 00:59:54]

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

[scottchen, 2017-03-29 01:00:42]

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

[commit-bot: I haz the power, 2017-03-29 01:01:55]

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

[scottchen, 2017-03-29 01:05:13]

I redid the effects using InteractionObserver, this CL is ready for review. 

Closure compiler will fail, pending on this CL:
https://critique.corp.google.com/#review/151507518

[commit-bot: I haz the power, 2017-03-29 02:03:25]

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

[commit-bot: I haz the power, 2017-03-29 02:03:27]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dpapad, 2017-03-29 17:42:16]

https://codereview.chromium.org/2702523005/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right):

https://codereview.chromium.org/2702523005/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:50: <dialog is="cr-dialog"
id="dialog">
Is the id used anywhere?

https://codereview.chromium.org/2702523005/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:60: // Height is normally
set via css, but mixin doesn't work with innerHTML.
s/css/CSS

https://codereview.chromium.org/2702523005/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:61:
bodyContainer.style.height = 1;
Should this be

bodyContainer.style.height = '1px';
innerBody.style.height = '100px';

https://codereview.chromium.org/2702523005/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right):

https://codereview.chromium.org/2702523005/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:62: /* TODO(dbeam): copy
<paper-dialog-scrollable>'s dividers? */
This TODO is being addressed with this CL. Can we remove it now?

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

https://codereview.chromium.org/2702523005/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:47: attached: function() {
@override

https://codereview.chromium.org/2702523005/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:61:
this.intersectionObserver_.observe(this.$.bodyBottomMarker);
We only have a few dialogs that use this functionality. Can we make it opt-in
instead of adding the handlers for all dialogs, even the ones that don't need
it?

something like

<dialog is="cr-dialog" show-scroll-borders>....</dialog>

https://codereview.chromium.org/2702523005/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:64: detached: function() {
@override

[scottchen, 2017-03-29 20:52:43]

Description was changed from

==========
MD Settings: long add-address dialog body should have overscroll line.

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

to

==========
MD Settings: long dialog body should have overscroll line.

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

[scottchen, 2017-03-29 21:02:08]

https://codereview.chromium.org/2702523005/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right):

https://codereview.chromium.org/2702523005/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:50: <dialog is="cr-dialog"
id="dialog">
On 2017/03/29 17:42:16, dpapad wrote:
> Is the id used anywhere?

was, not anymore. Deleting.

https://codereview.chromium.org/2702523005/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:60: // Height is normally
set via css, but mixin doesn't work with innerHTML.
On 2017/03/29 17:42:16, dpapad wrote:
> s/css/CSS

Done.

https://codereview.chromium.org/2702523005/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:61:
bodyContainer.style.height = 1;
On 2017/03/29 17:42:16, dpapad wrote:
> Should this be
> 
> bodyContainer.style.height = '1px';
> innerBody.style.height = '100px';

I think either way works, but might be more correct with 'px' since the
attribute is supposed to return a string.

https://codereview.chromium.org/2702523005/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right):

https://codereview.chromium.org/2702523005/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:62: /* TODO(dbeam): copy
<paper-dialog-scrollable>'s dividers? */
On 2017/03/29 17:42:16, dpapad wrote:
> This TODO is being addressed with this CL. Can we remove it now?

Done.

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

https://codereview.chromium.org/2702523005/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:47: attached: function() {
On 2017/03/29 17:42:16, dpapad wrote:
> @override

Done.

https://codereview.chromium.org/2702523005/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:61:
this.intersectionObserver_.observe(this.$.bodyBottomMarker);
On 2017/03/29 17:42:16, dpapad wrote:
> We only have a few dialogs that use this functionality. Can we make it opt-in
> instead of adding the handlers for all dialogs, even the ones that don't need
> it?
> 
> something like
> 
> <dialog is="cr-dialog" show-scroll-borders>....</dialog>

Done.

https://codereview.chromium.org/2702523005/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:64: detached: function() {
On 2017/03/29 17:42:16, dpapad wrote:
> @override

Done.

[dpapad, 2017-03-29 21:12:22]

https://codereview.chromium.org/2702523005/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right):

https://codereview.chromium.org/2702523005/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:67: }
How about moving those border showing related rules to a different rule as
follows? This makes it more explicit that they are applicable only when
show-scroll-borders is applied.

:host[show-scroll-borders] .body-container {...}
:host[show-scroll-borders] .body-container.bottom-scrollable {...}

For this to work showScrollBorders needs "reflectToAttribute: true".

https://codereview.chromium.org/2702523005/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:120: <span
id="bodyBottomMarker">
Thi CL only adds a border to the bottom, but in the standup you mentioned that a
border should be shown both on top and bottom, depending on whether there is
more content to scroll. Can you clarify?

[scottchen, 2017-03-30 06:42:48]

https://codereview.chromium.org/2702523005/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right):

https://codereview.chromium.org/2702523005/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:67: }
On 2017/03/29 21:12:22, dpapad wrote:
> How about moving those border showing related rules to a different rule as
> follows? This makes it more explicit that they are applicable only when
> show-scroll-borders is applied.
> 
> :host[show-scroll-borders] .body-container {...}
> :host[show-scroll-borders] .body-container.bottom-scrollable {...}
> 
> For this to work showScrollBorders needs "reflectToAttribute: true".

Done.

https://codereview.chromium.org/2702523005/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:120: <span
id="bodyBottomMarker">
On 2017/03/29 21:12:22, dpapad wrote:
> Thi CL only adds a border to the bottom, but in the standup you mentioned that
a
> border should be shown both on top and bottom, depending on whether there is
> more content to scroll. Can you clarify?

I think I misspoke during standup - in the case of dialog, there's always a
border below the header and on top of the body, so top overscroll border is not
necessary.

[dpapad, 2017-03-30 17:18:39]

https://codereview.chromium.org/2702523005/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
(right):

https://codereview.chromium.org/2702523005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:15:
behaviors: [I18nBehavior],
Can you revert this? There are no other changes in this file, so let's remove
this and make the number of files affected by this CL smaller.

https://codereview.chromium.org/2702523005/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right):

https://codereview.chromium.org/2702523005/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:67: }
> Done.

Did you forget to upload latest patch? I don't see it.

[scottchen, 2017-03-30 18:05:45]

Patchset #8 (id:140001) has been deleted

[scottchen, 2017-03-30 18:11:18]

https://codereview.chromium.org/2702523005/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js
(right):

https://codereview.chromium.org/2702523005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:15:
behaviors: [I18nBehavior],
On 2017/03/30 17:18:39, dpapad wrote:
> Can you revert this? There are no other changes in this file, so let's remove
> this and make the number of files affected by this CL smaller.

Done.

https://codereview.chromium.org/2702523005/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right):

https://codereview.chromium.org/2702523005/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:67: }
On 2017/03/30 17:18:39, dpapad wrote:
> > Done.
> 
> Did you forget to upload latest patch? I don't see it.

..I keep forgetting that it would prompt me for a message after "git cl upload",
my bad.

[dpapad, 2017-03-30 18:25:00]

https://codereview.chromium.org/2702523005/diff/160001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html
(right):

https://codereview.chromium.org/2702523005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html:72:
}
Blank line between CSS rules missing?

https://codereview.chromium.org/2702523005/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right):

https://codereview.chromium.org/2702523005/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:122: <span
id="bodyBottomMarker">
<span> needs a closing tag, no?

https://codereview.chromium.org/2702523005/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right):

https://codereview.chromium.org/2702523005/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:60:
entries[0].intersectionRatio == 0 ?
Can you simplify this, similar to
https://codereview.chromium.org/2184863002/diff/220001/chrome/browser/resourc...

[scottchen, 2017-03-30 19:24:35]

https://codereview.chromium.org/2702523005/diff/160001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html
(right):

https://codereview.chromium.org/2702523005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html:72:
}
On 2017/03/30 18:25:00, dpapad wrote:
> Blank line between CSS rules missing?

Done.

https://codereview.chromium.org/2702523005/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right):

https://codereview.chromium.org/2702523005/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:122: <span
id="bodyBottomMarker">
On 2017/03/30 18:25:00, dpapad wrote:
> <span> needs a closing tag, no?

Done. Curious if it's possible to add a presubmit check for this.

https://codereview.chromium.org/2702523005/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right):

https://codereview.chromium.org/2702523005/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:60:
entries[0].intersectionRatio == 0 ?
On 2017/03/30 18:25:00, dpapad wrote:
> Can you simplify this, similar to
>
https://codereview.chromium.org/2184863002/diff/220001/chrome/browser/resourc...

Done.

[scottchen, 2017-03-30 19:24:41]

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

[commit-bot: I haz the power, 2017-03-30 19:25:20]

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

[dpapad, 2017-03-30 19:41:22]

LGTM

https://codereview.chromium.org/2702523005/diff/160001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right):

https://codereview.chromium.org/2702523005/diff/160001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:122: <span
id="bodyBottomMarker">
On 2017/03/30 at 19:24:34, scottchen wrote:
> On 2017/03/30 18:25:00, dpapad wrote:
> > <span> needs a closing tag, no?
> 
> Done. Curious if it's possible to add a presubmit check for this.

It would require parsing HTML. Probably possible (there are both python and Node
parsers). I don't know if it would be worth it though (depends on the cost of
maintaining this presubmit VS the benefit it gives us).

[commit-bot: I haz the power, 2017-03-30 21:12:11]

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

[commit-bot: I haz the power, 2017-03-30 21:12:12]

Dry run: This issue passed the CQ dry run.

[scottchen, 2017-03-30 23:42:44]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-03-30 23:43:49]

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-03-30 23:53:54]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490917364486860,
"parent_rev": "d0203954892c94a15b4f5843bdc519b2be81f0bc", "commit_rev":
"27793e2b00cd4f06e24cde9599be5693006ed02b"}

[commit-bot: I haz the power, 2017-03-30 23:55:02]

Description was changed from

==========
MD Settings: long dialog body should have overscroll line.

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

to

==========
MD Settings: long dialog body should have overscroll line.

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

Review-Url: https://codereview.chromium.org/2702523005
Cr-Commit-Position: refs/heads/master@{#460937}
Committed:
https://chromium.googlesource.com/chromium/src/+/27793e2b00cd4f06e24cde9599be...
==========

[commit-bot: I haz the power, 2017-03-30 23:55:03]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/27793e2b00cd4f06e24cde9599be...

[stgao, 2017-03-31 01:00:50]

A revert of this CL (patchset #9 id:180001) has been created in
https://codereview.chromium.org/2781363004/ by stgao@chromium.org.

The reason for reverting is: Failed 	
CrElementsDialogTest.All on Waterfall

https://findit-for-me.appspot.com/waterfall/failure?url=https%3A%2F%2Fbuild.c....

[scottchen, 2017-03-31 01:50:25]

Description was changed from

==========
MD Settings: long dialog body should have overscroll line.

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

Review-Url: https://codereview.chromium.org/2702523005
Cr-Commit-Position: refs/heads/master@{#460937}
Committed:
https://chromium.googlesource.com/chromium/src/+/27793e2b00cd4f06e24cde9599be...
==========

to

==========
MD Settings: long dialog body should have overscroll line.

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

Review-Url: https://codereview.chromium.org/2702523005
Cr-Commit-Position: refs/heads/master@{#460937}
Committed:
https://chromium.googlesource.com/chromium/src/+/27793e2b00cd4f06e24cde9599be...
==========

[scottchen, 2017-03-31 01:54:53]

Updated the test to be less flaky, please re-lg.

[dpapad, 2017-03-31 01:55:26]

LGTM with optional nit.

https://codereview.chromium.org/2702523005/diff/200001/chrome/test/data/webui...
File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right):

https://codereview.chromium.org/2702523005/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:65: return
PolymerTest.flushTasks().then(function() {
Optional nit: Perhaps this test is more readable if we use the MutationObserver
for both assertions. Similar to as follows

var observerCalls = 0;

var observer = new MutationObserver(function() {
  observerCalls++;
  if (observerCalls == 1) {...assert ....}
  else if (observerCalls == 2) {.... assert....done...}

});
observer.observe(bodyContainer, {attributes: true});

[scottchen, 2017-03-31 18:06:50]

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

[commit-bot: I haz the power, 2017-03-31 18:07:28]

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

[scottchen, 2017-03-31 18:09:50]

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

[commit-bot: I haz the power, 2017-03-31 18:10:30]

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

[scottchen, 2017-03-31 18:10:56]

https://codereview.chromium.org/2702523005/diff/200001/chrome/test/data/webui...
File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right):

https://codereview.chromium.org/2702523005/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:65: return
PolymerTest.flushTasks().then(function() {
On 2017/03/31 01:55:26, dpapad wrote:
> Optional nit: Perhaps this test is more readable if we use the
MutationObserver
> for both assertions. Similar to as follows
> 
> var observerCalls = 0;
> 
> var observer = new MutationObserver(function() {
>   observerCalls++;
>   if (observerCalls == 1) {...assert ....}
>   else if (observerCalls == 2) {.... assert....done...}
> 
> });
> observer.observe(bodyContainer, {attributes: true});

Done.

[dpapad, 2017-03-31 18:16:01]

https://codereview.chromium.org/2702523005/diff/220001/chrome/test/data/webui...
File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right):

https://codereview.chromium.org/2702523005/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:48: test('dialog body
indicates over-scroll when appropriate', function(done) {
'overscroll' is used to indicate that blank space is added for the sole purpose
of allowing something to be scrolled to the top of the viewport. This is not the
case here, so let's rephrase.

https://codereview.chromium.org/2702523005/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:56: var innerBody =
document.body.querySelector('.body');
Is this used anywhere?

https://codereview.chromium.org/2702523005/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:66:
console.log(observerCount);
Let's remove this.

[scottchen, 2017-03-31 20:20:21]

https://codereview.chromium.org/2702523005/diff/220001/chrome/test/data/webui...
File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right):

https://codereview.chromium.org/2702523005/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:48: test('dialog body
indicates over-scroll when appropriate', function(done) {
On 2017/03/31 18:16:01, dpapad wrote:
> 'overscroll' is used to indicate that blank space is added for the sole
purpose
> of allowing something to be scrolled to the top of the viewport. This is not
the
> case here, so let's rephrase. 

Done.

https://codereview.chromium.org/2702523005/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:56: var innerBody =
document.body.querySelector('.body');
On 2017/03/31 18:16:01, dpapad wrote:
> Is this used anywhere?

I was using it to set height, but realized that having the text inside of
div.body already gives it enough height (i.e. greater than 1px).

Removing.

https://codereview.chromium.org/2702523005/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_dialog_test.js:66:
console.log(observerCount);
On 2017/03/31 18:16:01, dpapad wrote:
> Let's remove this.

Done.

[scottchen, 2017-03-31 20:20:27]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-03-31 20:20:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2702523005/#ps280001
(title: "remove unused variable")

[commit-bot: I haz the power, 2017-03-31 20:20:56]

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

[scottchen, 2017-03-31 20:41:22]

Patchset #15 (id:300001) has been deleted

[scottchen, 2017-03-31 20:42:36]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-03-31 20:42:37]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2702523005/#ps280001
(title: "remove unused variable")

[commit-bot: I haz the power, 2017-03-31 20:43:15]

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-03-31 23:06:35]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1490992956841540,
"parent_rev": "82e698b4a53d8592222d7e641275f14057665f4e", "commit_rev":
"a4c0720e53ea714a31f27ee2a8c4bf6ec2841a34"}

[commit-bot: I haz the power, 2017-03-31 23:07:29]

Description was changed from

==========
MD Settings: long dialog body should have overscroll line.

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

Review-Url: https://codereview.chromium.org/2702523005
Cr-Commit-Position: refs/heads/master@{#460937}
Committed:
https://chromium.googlesource.com/chromium/src/+/27793e2b00cd4f06e24cde9599be...
==========

to

==========
MD Settings: long dialog body should have overscroll line.

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

Review-Url: https://codereview.chromium.org/2702523005
Cr-Original-Commit-Position: refs/heads/master@{#460937}
Committed:
https://chromium.googlesource.com/chromium/src/+/27793e2b00cd4f06e24cde9599be...
Review-Url: https://codereview.chromium.org/2702523005
Cr-Commit-Position: refs/heads/master@{#461264}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4c0720e53ea714a31f27ee2a8c4...
==========

[commit-bot: I haz the power, 2017-03-31 23:07:30]

Committed patchset #14 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/a4c0720e53ea714a31f27ee2a8c4...

[scottchen, 2017-04-03 17:28:48]

Description was changed from

==========
MD Settings: long dialog body should have overscroll line.

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

Review-Url: https://codereview.chromium.org/2702523005
Cr-Original-Commit-Position: refs/heads/master@{#460937}
Committed:
https://chromium.googlesource.com/chromium/src/+/27793e2b00cd4f06e24cde9599be...
Review-Url: https://codereview.chromium.org/2702523005
Cr-Commit-Position: refs/heads/master@{#461264}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4c0720e53ea714a31f27ee2a8c4...
==========

to

==========
MD Settings: long dialog body should have overscroll line.

BUG=684152, 595804
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2702523005
Cr-Original-Commit-Position: refs/heads/master@{#460937}
Committed:
https://chromium.googlesource.com/chromium/src/+/27793e2b00cd4f06e24cde9599be...
Review-Url: https://codereview.chromium.org/2702523005
Cr-Commit-Position: refs/heads/master@{#461264}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4c0720e53ea714a31f27ee2a8c4...
==========