MD Settings: fix overflow scrolling for <settings-dialog>

MD Settings: fix overflow scrolling for <settings-dialog>

Currently the whole dialog scrolls if there's too much middle content.
We really just want a scrollbar between the title and the buttons.

BUG=595804
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/0726838693d45b022178e8ddaea8120364ea876d
Cr-Commit-Position: refs/heads/master@{#395813}

[Dan Beam, 2016-05-13 01:40:23]

Description was changed from

==========
MD Settings: fix overflow scrolling for <settings-dialog>

Currently the whole dialog scrolls if there's too much middle content.
We really just want a scrollbar between the title and the buttons.

BUG=595804
R=dpapad@chromium.org
==========

to

==========
MD Settings: fix overflow scrolling for <settings-dialog>

Currently the whole dialog scrolls if there's too much middle content.
We really just want a scrollbar between the title and the buttons.

BUG=595804
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-05-13 01:42:13]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-05-13 01:42:40]

https://codereview.chromium.org/1974193002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.js (right):

https://codereview.chromium.org/1974193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.js:40:
this.$$('.footer-container').offsetHeight) + 'px';
Do you hate this? (y/n) _

[dpapad, 2016-05-13 18:14:03]

Perhaps you could remove the button-container -> buttons renaming from this CL,
as it is making it look larger than it actually is.

https://codereview.chromium.org/1974193002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.js (right):

https://codereview.chromium.org/1974193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.js:40:
this.$$('.footer-container').offsetHeight) + 'px';
On 2016/05/13 at 01:42:40, Dan Beam wrote:
> Do you hate this? (y/n) _

Well, it is a bit complicated at first sight. My main concern is not so much
this particular calculation but the overall complexity of the desired dialog
scrolling behavior. Issues I see so far for example,

1) clipping issue at http://imgur.com/HshMbbE (after patching this CL).
2) The "Clear browsing data" dialog looks very odd (too small for the amount of
content it has), see http://imgur.com/XOVA4oY. The scrollbar and the buttons are
adjacent at the corner which seems weird IMO. 

Scrolling the entire dialog (current behavior) seems more robust to resizing,
but I understand that the current spec calls for scrolling the body only, so we
should try to achieve that behavior, but without creating corner cases that
current behavior does not have.

[dpapad, 2016-05-13 18:20:33]

https://codereview.chromium.org/1974193002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.js (right):

https://codereview.chromium.org/1974193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.js:40:
this.$$('.footer-container').offsetHeight) + 'px';
FYI regarding the clipping issue, I had filed
https://github.com/PolymerElements/paper-dialog-behavior/issues/83 in the past.
Polymer team tried to fix, but this caused other bugs and was reverted, and
eventually it was left up to users to specify overflow.

[Dan Beam, 2016-05-17 19:26:12]

https://codereview.chromium.org/1974193002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.js (right):

https://codereview.chromium.org/1974193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.js:40:
this.$$('.footer-container').offsetHeight) + 'px';
On 2016/05/13 18:14:03, dpapad wrote:
> On 2016/05/13 at 01:42:40, Dan Beam wrote:
> > Do you hate this? (y/n) _
> 
> Well, it is a bit complicated at first sight. My main concern is not so much
> this particular calculation but the overall complexity of the desired dialog
> scrolling behavior. Issues I see so far for example,
> 
> 1) clipping issue at http://imgur.com/HshMbbE (after patching this CL).

fixed

> 2) The "Clear browsing data" dialog looks very odd (too small for the amount
of
> content it has), see http://imgur.com/XOVA4oY. The scrollbar and the buttons
are
> adjacent at the corner which seems weird IMO. 

CBD should now look better, but without a border between the scrollable area and
the button strip, we're bound to end up in odd states.  this is not different
from options.

> 
> Scrolling the entire dialog (current behavior) seems more robust to resizing,
> but I understand that the current spec calls for scrolling the body only, so
we
> should try to achieve that behavior, but without creating corner cases that
> current behavior does not have.

the new patchset should be a good blend of simple-ish and good UX

[dpapad, 2016-05-18 17:50:11]

As discussed via chat, can we explore the alternative of using flexbox to
achieve the same functionality, before going forward with this approach? Perhaps
this could allow us to not have to override the center() method, see
https://jsfiddle.net/qa0e8p5x/2/.

https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.js (right):

https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.js:43: var availableHeight =
parseInt(this.style.maxHeight, 10);
The code seems to assume that avaailableHeight is not NaN after this line? Can
you add an assertion?

Would it be more robust as follows? Or does the following have bad performance?
var availableHeight = parseInt(getComputedStyle(this)['max-height'], 10);

https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.js:56: // get smaller than
this dialog, buttons can still be scrolled to. Adding a
s/get/gets

[valdrin, 2016-05-18 18:43:36]

valdrin@google.com changed reviewers:
+ valdrin@google.com

[valdrin, 2016-05-18 18:43:37]

https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.js (right):

https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.js:34: center: function() {
This should not be needed, as iron-fit-behavior is already doing all this for
you, you just need to set the right sizingTarget. In your case, its default
value (`this`) should be enough.
Also, iron-fit-behavior won't set the min-width/min-height if these are already
set. Finally, each time you resize the window, opened overlays will
refit/position.

The problem is caused by the fact that some child nodes have margin, hence if
you have a min-height that is less than the sum of these margins, you'll see
content overflowing.
My suggestion would be to set a min-height that is at least as big as the sum of
these margins. Set this in the  css of this component, you shouldn't need to
override this function at all :)

[valdrin, 2016-05-18 18:47:18]

https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.js (right):

https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.js:34: center: function() {
On 2016/05/18 18:43:37, valdrin wrote:
> This should not be needed, as iron-fit-behavior is already doing all this for
> you, you just need to set the right sizingTarget. In your case, its default
> value (`this`) should be enough.
> Also, iron-fit-behavior won't set the min-width/min-height if these are
already
Sorry, i meant max-width, max-height. If you set these, the behavior will keep
them, so consider setting those on your css as well.
> set. Finally, each time you resize the window, opened overlays will
> refit/position.
> 
> The problem is caused by the fact that some child nodes have margin, hence if
> you have a min-height that is less than the sum of these margins, you'll see
> content overflowing.
> My suggestion would be to set a min-height that is at least as big as the sum
of
> these margins. Set this in the  css of this component, you shouldn't need to
> override this function at all :)

[Dan Beam, 2016-05-24 02:22:12]

https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_dialog.js (right):

https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_dialog.js:34: center: function() {
On 2016/05/18 18:47:18, valdrin wrote:
> On 2016/05/18 18:43:37, valdrin wrote:
> > This should not be needed, as iron-fit-behavior is already doing all this
for
> > you, you just need to set the right sizingTarget. In your case, its default
> > value (`this`) should be enough.
> > Also, iron-fit-behavior won't set the min-width/min-height if these are
> already
> Sorry, i meant max-width, max-height. If you set these, the behavior will keep
> them, so consider setting those on your css as well.
> > set. Finally, each time you resize the window, opened overlays will
> > refit/position.
> > 
> > The problem is caused by the fact that some child nodes have margin, hence
if
> > you have a min-height that is less than the sum of these margins, you'll see
> > content overflowing.
> > My suggestion would be to set a min-height that is at least as big as the
sum
> of
> > these margins. Set this in the  css of this component, you shouldn't need to
> > override this function at all :)
> 

valdrin: every <settings-dialog> has a different height, as it's got many
<content> slots, so there's no way to useful set a minimum height statically in
CSS

[valdrin, 2016-05-24 05:55:09]

On 2016/05/24 02:22:12, Dan Beam wrote:
>
https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_dialog.js (right):
> 
>
https://codereview.chromium.org/1974193002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_dialog.js:34: center: function() {
> On 2016/05/18 18:47:18, valdrin wrote:
> > On 2016/05/18 18:43:37, valdrin wrote:
> > > This should not be needed, as iron-fit-behavior is already doing all this
> for
> > > you, you just need to set the right sizingTarget. In your case, its
default
> > > value (`this`) should be enough.
> > > Also, iron-fit-behavior won't set the min-width/min-height if these are
> > already
> > Sorry, i meant max-width, max-height. If you set these, the behavior will
keep
> > them, so consider setting those on your css as well.
> > > set. Finally, each time you resize the window, opened overlays will
> > > refit/position.
> > > 
> > > The problem is caused by the fact that some child nodes have margin, hence
> if
> > > you have a min-height that is less than the sum of these margins, you'll
see
> > > content overflowing.
> > > My suggestion would be to set a min-height that is at least as big as the
> sum
> > of
> > > these margins. Set this in the  css of this component, you shouldn't need
to
> > > override this function at all :)
> > 
> 
> valdrin: every <settings-dialog> has a different height, as it's got many
> <content> slots, so there's no way to useful set a minimum height statically
in
> CSS

Please have a look at this jsbin, it seems to solve the issue
http://jsbin.com/biwuvu/2/edit
It basically consists in setting the sizingTarget to be .body-content, and
styling .body-content to be scrollable and flex.

Have you considered using paper-dialog-scrollable?
https://github.com/PolymerElements/paper-dialog-scrollable

[dpapad, 2016-05-24 19:01:31]

+hcarmona

Hector has an interesting use case where a drop down exists within the dialog.
The dropdown might be getting clipped as a result of adding overflow auto to the
dialog (or parts of the dialog). @Hector: Could you check if that is still
happening after patching this CL?

[valdrin, 2016-05-24 19:09:02]

On 2016/05/24 19:01:31, dpapad wrote:
> +hcarmona
> 
> Hector has an interesting use case where a drop down exists within the dialog.
> The dropdown might be getting clipped as a result of adding overflow auto to
the
> dialog (or parts of the dialog). @Hector: Could you check if that is still
> happening after patching this CL?

The dropdown might get clipped if the dialog is contained in an element that
creates a new stacking context.
Also, on mobile you might want to set the -webkit-overflow-scrolling: touch;
property to have smoother scrolling experience (paper-dialog-scrollable does
that); beware that it creates a new stacking context, hence it would clip
overlays overflowing the dialog (e.g long dropdowns).

[Dan Beam, 2016-05-24 19:13:17]

On 2016/05/24 19:09:02, valdrin wrote:
> On 2016/05/24 19:01:31, dpapad wrote:
> > +hcarmona
> > 
> > Hector has an interesting use case where a drop down exists within the
dialog.
> > The dropdown might be getting clipped as a result of adding overflow auto to
> the
> > dialog (or parts of the dialog). @Hector: Could you check if that is still
> > happening after patching this CL?
> 
> The dropdown might get clipped if the dialog is contained in an element that
> creates a new stacking context.
> Also, on mobile you might want to set the -webkit-overflow-scrolling: touch;
> property to have smoother scrolling experience (paper-dialog-scrollable does
> that); beware that it creates a new stacking context, hence it would clip
> overlays overflowing the dialog (e.g long dropdowns).

no mobile, only desktop chroem

[hcarmona, 2016-05-24 20:36:06]

On 2016/05/24 19:13:17, Dan Beam wrote:
> On 2016/05/24 19:09:02, valdrin wrote:
> > On 2016/05/24 19:01:31, dpapad wrote:
> > > +hcarmona
> > > 
> > > Hector has an interesting use case where a drop down exists within the
> dialog.
> > > The dropdown might be getting clipped as a result of adding overflow auto
to
> > the
> > > dialog (or parts of the dialog). @Hector: Could you check if that is still
> > > happening after patching this CL?
> > 
> > The dropdown might get clipped if the dialog is contained in an element that
> > creates a new stacking context.
> > Also, on mobile you might want to set the -webkit-overflow-scrolling: touch;
> > property to have smoother scrolling experience (paper-dialog-scrollable does
> > that); beware that it creates a new stacking context, hence it would clip
> > overlays overflowing the dialog (e.g long dropdowns).
> 
> no mobile, only desktop chroem

It's working fine for me now. Both with and without this patch. I took
an update this morning and it started working for me. I don't like not
knowing what fixed it but I don't see clipping anymore.

[Dan Beam, 2016-05-25 00:55:06]

ok, dpapad@, this has all the fun things that you, valdrin, and I discovered or
fixed today :)

[dpapad, 2016-05-25 01:32:28]

https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_dialog.html (right):

https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_dialog.html:62: flex: 1;
Nit: Can we group common parts together?

:host ::content .title,
:host ::content .body {
  -webkit-padding-end: 24px;
  -webkit-padding-start: 24px;
  flex: 1;
}

https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_dialog.js (right):

https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_dialog.js:46: _renderOpened:
function() {
Ah, not a huge fun of this, but I understand.

Could you explain a bit more in the comment what "don't deal well" means, such
that it can be more helpful to a future reader who might be attempting to remove
this hack without regressing?

Specifically, what is the exact symptom (which dialog is affected? what happens
exactly?)

[valdrin, 2016-05-25 01:56:59]

On 2016/05/25 01:32:28, dpapad wrote:
>
https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
> File chrome/browser/resources/settings/settings_dialog.html (right):
> 
>
https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
> chrome/browser/resources/settings/settings_dialog.html:62: flex: 1;
> Nit: Can we group common parts together?
> 
> :host ::content .title,
> :host ::content .body {
>   -webkit-padding-end: 24px;
>   -webkit-padding-start: 24px;
>   flex: 1;
> }
> 
>
https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
> File chrome/browser/resources/settings/settings_dialog.js (right):
> 
>
https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
> chrome/browser/resources/settings/settings_dialog.js:46: _renderOpened:
> function() {
> Ah, not a huge fun of this, but I understand.
> 
> Could you explain a bit more in the comment what "don't deal well" means, such
> that it can be more helpful to a future reader who might be attempting to
remove
> this hack without regressing?
> 
> Specifically, what is the exact symptom (which dialog is affected? what
happens
> exactly?)

Apparently the ".footer" elements "change" their size after the dialog is fully
opened. The dialog will observe for any node change (added/removed nodes), but
can't monitor stuff like styling change. In those cases, the elements that
change their size should fire an `iron-resize` event; the dialog listens for
these events and refits accordingly. 
We debugged this a bit with danbeam@ but couldn't find the root cause of this
behavior. Here's my tentative to reproduce the scenario in isolation, but I
can't reproduce the issue yet http://jsbin.com/hicepa/5/edit?html,output

[Dan Beam, 2016-05-25 02:07:18]

Patchset #9 (id:180001) has been deleted

[Dan Beam, 2016-05-25 02:07:49]

https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_dialog.html (right):

https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_dialog.html:62: flex: 1;
On 2016/05/25 01:32:28, dpapad wrote:
> Nit: Can we group common parts together?
> 
> :host ::content .title,
> :host ::content .body {
>   -webkit-padding-end: 24px;
>   -webkit-padding-start: 24px;
>   flex: 1;
> }

Done.

https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_dialog.js (right):

https://codereview.chromium.org/1974193002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_dialog.js:46: _renderOpened:
function() {
On 2016/05/25 01:32:28, dpapad wrote:
> Ah, not a huge fun of this, but I understand.
> 
> Could you explain a bit more in the comment what "don't deal well" means, such
> that it can be more helpful to a future reader who might be attempting to
remove
> this hack without regressing?
> 
> Specifically, what is the exact symptom (which dialog is affected? what
happens
> exactly?)

added more context

[dpapad, 2016-05-25 02:17:46]

LGTM. Thanks for investigating and coming up with a fix (both @dbeam and
@valdrin)!

@valdrin: I don't think that having the footer changing size is a common problem
for all dialogs, it only seems to happen for the "Clear Browsing Data" dialog,
and specifically the code in question is here,
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

I believe that the problem could also have been fixed by calling center() right
after modifying the footer, but that would be a targeted fix for that particular
dialog, instead of the more generic fix that is included in this CL. I am still
a bit nervous that the fix relies on a certain time window, and if for some
reason the code that modifies the footer executes later, the dialog would still
be incorrectly centered.

FWIW, I am facing a similar problem in a new dialog I am implementing where the
distributed children are changing size based on a user action (clicking on a
button belonging to a radio group). In those cases, calling center() manually is
the only way to go (at least the only way I've found).

[valdrin, 2016-05-25 02:23:07]

On 2016/05/25 02:17:46, dpapad wrote:
> LGTM. Thanks for investigating and coming up with a fix (both @dbeam and
> @valdrin)!
> 
> @valdrin: I don't think that having the footer changing size is a common
problem
> for all dialogs, it only seems to happen for the "Clear Browsing Data" dialog,
> and specifically the code in question is here,
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....
> 
> I believe that the problem could also have been fixed by calling center()
right
> after modifying the footer, but that would be a targeted fix for that
particular
> dialog, instead of the more generic fix that is included in this CL. I am
still
> a bit nervous that the fix relies on a certain time window, and if for some
> reason the code that modifies the footer executes later, the dialog would
still
> be incorrectly centered.
> 
> FWIW, I am facing a similar problem in a new dialog I am implementing where
the
> distributed children are changing size based on a user action (clicking on a
> button belonging to a radio group). In those cases, calling center() manually
is
> the only way to go (at least the only way I've found).

Ah ha! That is the part that should notify about the size change..! Have you
tried calling `this.$.dialog.notifyResize()` after modifying those properties?
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

[Dan Beam, 2016-05-25 05:40:46]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-05-25 05:40:47]

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/1974193002/#ps220001
(title: "unhack")

[commit-bot: I haz the power, 2016-05-25 05:41:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974193002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974193002/220001

[commit-bot: I haz the power, 2016-05-25 06:16:59]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-05-25 06:17:57]

Description was changed from

==========
MD Settings: fix overflow scrolling for <settings-dialog>

Currently the whole dialog scrolls if there's too much middle content.
We really just want a scrollbar between the title and the buttons.

BUG=595804
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: fix overflow scrolling for <settings-dialog>

Currently the whole dialog scrolls if there's too much middle content.
We really just want a scrollbar between the title and the buttons.

BUG=595804
R=dpapad@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/0726838693d45b022178e8ddaea8120364ea876d
Cr-Commit-Position: refs/heads/master@{#395813}
==========

[commit-bot: I haz the power, 2016-05-25 06:17:58]

Patchset 10 (id:??) landed as
https://crrev.com/0726838693d45b022178e8ddaea8120364ea876d
Cr-Commit-Position: refs/heads/master@{#395813}