Sören Hentzschel Reporter
	Description • 10 years ago

It should not be possible to resize the dialogs of the in-content preferences so that the buttons are no longer visible.

	Tim Nguyen :ntim
	Comment 1 • 10 years ago

This adds overflow: auto on .groupbox-body, so the dialog body is scrollable if there are overflowed items. It also adds overflow: auto on the dialog overlay, to allow scrolling in case the dialog is bigger than the screen.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 2 • 10 years ago

Comment on attachment 8523186 [details] [diff] [review]
Patch

Review of attachment 8523186 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for not getting to this sooner. This approach, while making the dialogs usable, doesn't get at the root issue. We should figure out what we need to do to make it so that the dialog never gets smaller than its contents, with the exception being if the dialogs contents are larger than the viewport.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 3 • 10 years ago

min-width:-moz-fit-content seems like it would be what we want.

	Tim Nguyen :ntim
	Comment 4 • 10 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> min-width:-moz-fit-content seems like it would be what we want.

Except that our problem here is concerning the height. And -moz-fit-content isn't supported for the height properties yet.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 5 • 9 years ago

(In reply to Tim Nguyen [:ntim] from comment #4)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> > min-width:-moz-fit-content seems like it would be what we want.
> 
> Except that our problem here is concerning the height. And -moz-fit-content
> isn't supported for the height properties yet.

Until bug 567039 is fixed (and I wouldn't count on that happening any time soon), we'll probably need to use JS to calculate the minimum height of the dialog and then set min-height on #dialogBox to be that value. We could probably calculate the minimum height by summing the heights of:
.groupbox-title
40px (20px top padding and 20px bottom padding of .groupbox-body, but computing the padding is better than hardcoding it)
and #dialogFrame

Computing the minimum height of #dialogFrame may turn out to be tricky.

Another workaround until bug 567039 is fixed may be to store some pre-computed heights of dialogs and apply those when the dialog is opened. This ideally would include 'em' units so that it scales with font-size changes.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 6 • 9 years ago

This patch turned out to be simpler than I thought it would be. Note that the padding is not explicitly added in the summation of the box minHeight because it is included implicitly through the height of the frame calculation above.

	:Gijs (he/him)
	Comment 7 • 9 years ago

Comment on attachment 8560476 [details] [diff] [review]
Patch

Review of attachment 8560476 [details] [diff] [review]:
-----------------------------------------------------------------

I got some $questions, so clearing r? for now.

::: browser/components/preferences/in-content/subdialogs.js
@@ +154,5 @@
> +    this._frame.style.height = docEl.style.height || (docEl.scrollHeight + framePadding) + "px";
> +
> +    let boxBorder = 2 * parseFloat(getComputedStyle(this._box).borderTopWidth);
> +    this._box.style.minHeight = (boxBorder + this._box.clientHeight) + "px";
> +    this._box.style.minWidth = (boxBorder + framePadding + this._box.clientWidth) + "px";

This is going to trigger a lot of sync reflows because the getters and setters of style info are all intermixed.

Any chance of reading the data first, and setting them in a second step, to reduce the number of reflows here?

Also, why do we need to style the box separately here? It seems like it's the direct parent of this._frame, and so the box's own border shouldn't need to be taken into account separately to fix the height here...

This code is also making the assumption that the border of this._box is completely symmetric (ie the top border width == bottom, left and right border). That seems like a not-great assumption to make for the width of the box.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 8 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #7)
> This is going to trigger a lot of sync reflows because the getters and
> setters of style info are all intermixed.

I don't see a way around this. I tried calculating the this._box.clientHeight before setting the height of the frame, but the clientHeight is too short then. Using the frameHeight here as a replacement for the box clientHeight doens't work either (too short).

> Any chance of reading the data first, and setting them in a second step, to
> reduce the number of reflows here?

I've tried to do as much precomputing in this patch as possible, but it will still involve a sync-reflow. I don't think we need to be as concerned with sync reflows here as in with the tab animations (there is no animation here and it is not as high-level UI as the tabs).

> Also, why do we need to style the box separately here? It seems like it's
> the direct parent of this._frame, and so the box's own border shouldn't need
> to be taken into account separately to fix the height here...

In XUL markup it is, but after being laid out there are other elements that get inserted. You can see this difference when comparing the XUL source file and what the devtools Inspector sees. Without the code for the borders, the boxes can resize two pixels smaller in each direction. I tracked down the 2px difference to the width of the borders (1px for each side).

> This code is also making the assumption that the border of this._box is
> completely symmetric (ie the top border width == bottom, left and right
> border). That seems like a not-great assumption to make for the width of the
> box.

Yes, that was just done to simplify the code since they are all (currently) symmetrical. I've adjusted this patch to have separate horizontal and vertical measurements. I don't think we should go out of our way to handle asymmetrical horizontal or vertical measurements.

	:Gijs (he/him)
	Comment 9 • 9 years ago

Comment on attachment 8560584 [details] [diff] [review]
Patch v2

Review of attachment 8560584 [details] [diff] [review]:
-----------------------------------------------------------------

r+ considering how non-perf-critical this code is, I guess, even if it makes me cringe a little; as long as it does the job... :-)

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 10 • 9 years ago

Thanks.

https://hg.mozilla.org/integration/fx-team/rev/c6aca5f18fdf

	Phil Ringnalda (:philor)
	Comment 11 • 9 years ago

Backed out in https://hg.mozilla.org/integration/fx-team/rev/9c5e1c119b2d for https://treeherder.mozilla.org/logviewer.html#?job_id=1933473&repo=fx-team - you and browser_subdialogs.js need to get together on whether it's px or em.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 12 • 9 years ago

Relanded with the test fixed,
https://hg.mozilla.org/integration/fx-team/rev/f1ce87511750

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 13 • 9 years ago

This was backed out in https://hg.mozilla.org/integration/fx-team/rev/f6fde9d79acf for causing browser_subdialogs.js to fail on OSX.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 14 • 9 years ago

Updated the test to use px instead of em,
https://hg.mozilla.org/integration/fx-team/rev/19e0922d6d0c

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 9 years ago

Backed out for OSX browser_subdialogs.js failures.
https://hg.mozilla.org/integration/fx-team/rev/e1af04992cfb

https://treeherder.mozilla.org/logviewer.html#?job_id=1971083&repo=fx-team

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 16 • 9 years ago

Looked at it on OSX and fixed the issue with a little usage of calc(). Try results are green:
https://tbpl.mozilla.org/?tree=Try&rev=3505e3fe8fce

Landed on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/13b908ad9c77

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/13b908ad9c77

	Camelia Badau [:cbadau], Desktop QA
	Comment 18 • 9 years ago

I've tested on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Aurora 38.0a2 (buildID: 20150304004138) and latest Nightly 39.0a1 (buildID: 20150304030231) and here are the results: 
- on Windows 7 64bit: 
     - on Aurora 38.0a2: 
             - under Security tab -> "Change Master Password" dialog: the buttons are pushed into overflow
     - on latest Nightly 39.0a1: no problem occurs, all the buttons are visible (on all dialogs)

- on Mac OSX 10.9.5: 
     - on Aurora 38.0a2: 
             - under Content tab -> "Exceptions" dialog: the buttons are pushed into overflow
             - under Content tab -> "Languages" dialog: the buttons are pushed into overflow
             - under Privacy tab -> "Settings for Clearing History": the buttons are not visible
             - under Security tab -> "Change Master Password" dialog: the buttons are pushed into overflow
     - on latest Nightly 39.0a1:
             - under Content tab -> "Languages" dialog: the buttons are pushed into overflow
             - under Privacy tab -> "Settings for Clearing History": the buttons are pushed into overflow
             - under Security tab -> "Change Master Password" dialog: the buttons are pushed into overflow

- on Ubuntu 13.10 32bit: 
     - on Aurora 38.0a2: 
             - under General tab -> "Set Home Page" dialog: the buttons are pushed into overflow
             - under under Privacy tab -> "Settings for Clearing History": the buttons are pushed into overflow
             - under Security tab -> "Change Master Password" dialog: the buttons are not visible
     - on latest Nightly 39.0a1: 
             - under General tab -> "Set Home Page" dialog: the buttons are pushed into overflow
             - under under Privacy tab -> "Settings for Clearing History": the buttons are pushed into overflow
             - under Security tab -> "Change Master Password" dialog: the buttons are pushed into overflow

Please see attachment "issues.zip".

Any thoughts about this?

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 19 • 9 years ago

(In reply to Camelia Badau, QA [:cbadau] from comment #18)
> Any thoughts about this?

Can you please do a regression-hunt for the difference between Aurora 38.0a2 and Nightly 39.0a1 on Windows 7 to see at what build it began working?

Can you please file a bug for the OSX + Linux issues?

	Camelia Badau [:cbadau], Desktop QA
	Comment 20 • 9 years ago

I've tested on Windows 7 64bit using the first Aurora 38.0a2 (buildID: 20150224004223) and first Nightly 39.0a1 (buildID: 20150224030228) builds and there is the same difference as mentioned in comment #18: 
     - on Aurora 38.0a2: under Security tab -> "Change Master Password" dialog: the buttons are pushed into overflow
     - on Nightly 39.0a1: all the buttons are visible (on all dialogs). 

For the OSX + Linux issues, I filled bug 1141031.

	Camelia Badau [:cbadau], Desktop QA
	Comment 21 • 9 years ago

Should I also file a separate bug for the issue mentioned in comment 20 (the difference between Aurora 38.0a2 and Nightly 39.0a1 on Windows 7)? Or I should reopen this one?

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 22 • 9 years ago

(In reply to Camelia Badau, QA [:cbadau] from comment #21)
> Should I also file a separate bug for the issue mentioned in comment 20 (the
> difference between Aurora 38.0a2 and Nightly 39.0a1 on Windows 7)? Or I
> should reopen this one?

Yeah, a separate bug will be good. And can you mark it as blocking bug 1014201?

	Camelia Badau [:cbadau], Desktop QA
	Comment 23 • 9 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> (In reply to Camelia Badau, QA [:cbadau] from comment #21)
> > Should I also file a separate bug for the issue mentioned in comment 20 (the
> > difference between Aurora 38.0a2 and Nightly 39.0a1 on Windows 7)? Or I
> > should reopen this one?
> 
> Yeah, a separate bug will be good. And can you mark it as blocking bug
> 1014201?

I filled bug 1146807. 
Marking this bug as VERIFIED FIXED.