1044597 - in-content preferences: resized dialogs should not push buttons into overflow

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Sören Hentzschel]

Attachment: screenshot

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

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

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.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 4

[Tim Nguyen :ntim]

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

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

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.

Comment 7

[:Gijs (out for now; he/him)]

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.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

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

Comment 9

[:Gijs (out for now; he/him)]

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

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Thanks.

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

Comment 11

[Phil Ringnalda (:philor)]

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.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Camelia Badau [:cbadau], Desktop Test Engineering]

Attachment: issues.zip

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?

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

(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?

Comment 20

[Camelia Badau [:cbadau], Desktop Test Engineering]

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.

Comment 21

[Camelia Badau [:cbadau], Desktop Test Engineering]

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?

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

(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?

Comment 23

[Camelia Badau [:cbadau], Desktop Test Engineering]

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