chrome/browser/resources/settings/settings_dialog.js - Issue 1974193002: MD Settings: fix overflow scrolling for <settings-dialog> - Code Review

Chromium Code Reviews

chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out

(18)

My Issues | Starred Open | Closed | All

Side by Side Diff: chrome/browser/resources/settings/settings_dialog.js

Issue 1974193002: MD Settings: fix overflow scrolling for <settings-dialog> (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@bt-polish3

Patch Set: better Created 4 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« no previous file with comments | « chrome/browser/resources/settings/settings_dialog.html ('k') | chrome/browser/resources/settings/site_settings/add_site_dialog.html » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 // Copyright 2016 The Chromium Authors. All rights reserved.	1 // Copyright 2016 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 /**	5 /**

6 * @fileoverview 'settings-dialog' is a component for showing a modal dialog.	6 * @fileoverview 'settings-dialog' is a component for showing a modal dialog.

7 */	7 */

8 Polymer({	8 Polymer({

9 is: 'settings-dialog',	9 is: 'settings-dialog',

10	10

(...skipping 12 matching lines...) Expand all Loading...
23	23

24 /** @override */	24 /** @override */

25 withBackdrop: {	25 withBackdrop: {

26 type: Boolean,	26 type: Boolean,

27 value: true,	27 value: true,

28 },	28 },

29 },	29 },

30	30

31 behaviors: [Polymer.PaperDialogBehavior],	31 behaviors: [Polymer.PaperDialogBehavior],

32	32

	33 /** @override */

	34 center: function() {
	valdrin 2016/05/18 18:43:37 This should not be needed, as iron-fit-behavior is 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 Sorry, i meant max-width, max-height. If you set t Show quoted text 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. Show quoted text > 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:11 valdrin: every <settings-dialog> has a different h Show quoted text 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
	35 Polymer.IronFitBehavior.center.call(this);

	36

	37 // Height of stuff that should never scroll (i.e. title, buttons).

	38 var staticHeight = this.$$('.title-container').offsetHeight +

	39 this.$$('.button-container').offsetHeight +

	40 this.$$('.footer-container').offsetHeight;

	41

	42 // The amount of space we've got to work with based on window size.

	43 var availableHeight = parseInt(this.style.maxHeight, 10);
	dpapad 2016/05/18 17:50:10 The code seems to assume that avaailableHeight is 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);
	44

	45 var MIN_BODY_HEIGHT = 120;

	46

	47 // Constrain body height to the remaining space; might add middle scrollbar.

	48 var bodyHeight = Math.max(availableHeight - staticHeight, MIN_BODY_HEIGHT);

	49 this.$$('.body-container').style.maxHeight = bodyHeight + 'px';

	50

	51 var requiredHeight = staticHeight + MIN_BODY_HEIGHT;

	52 var needsMinHeight = availableHeight < requiredHeight;

	53 this.style.minHeight = needsMinHeight ? requiredHeight + 'px' : '';

	54

	55 // TODO(dbeam): give a min-height to something like <body> so if the window

	56 // get smaller than this dialog, buttons can still be scrolled to. Adding a
	dpapad 2016/05/18 17:50:11 s/get/gets s/get/gets
	57 // min-height to either this dialog or the backdropElement doesn't help as

	58 // they're position: fixed; and not in the flow of the page.

	59 },

	60

33 /** @return {!PaperIconButtonElement} */	61 /** @return {!PaperIconButtonElement} */

34 getCloseButton: function() {	62 getCloseButton: function() {

35 return this.$.close;	63 return this.$.close;

36 },	64 },

37 });	65 });

OLD	NEW

« no previous file with comments | « chrome/browser/resources/settings/settings_dialog.html ('k') | chrome/browser/resources/settings/site_settings/add_site_dialog.html » ('j') | no next file with comments »

Powered by Google App Engine

This is Rietveld 408576698