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

VERIFIED FIXED in Firefox 38

Status

()

Firefox
Preferences
P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Sören Hentzschel, Assigned: jaws)

Tracking

Trunk
Firefox 38
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox38 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8463130 [details]
screenshot

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

Updated

4 years ago
Blocks: 1014201
Flags: firefox-backlog+
Points: --- → 3
Summary: in-content preferences: dialogs should not be arbitrary resizable → in-content preferences: resized dialogs should not push buttons into overflow

Comment 1

3 years ago
Created attachment 8523186 [details] [diff] [review]
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.
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attachment #8523186 - Flags: review?(jaws)
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.
Attachment #8523186 - Flags: review?(jaws) → review-

Updated

3 years ago
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW
OS: Mac OS X → All
min-width:-moz-fit-content seems like it would be what we want.

Comment 4

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

Updated

3 years ago
Depends on: 1116494
Depends on: 567039
(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.

Updated

3 years ago
No longer depends on: 1116494
Priority: -- → P1
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Created attachment 8560476 [details] [diff] [review]
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.
Attachment #8523186 - Attachment is obsolete: true
Attachment #8560476 - Flags: review?(gijskruitbosch+bugs)

Comment 7

3 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.
Attachment #8560476 - Flags: review?(gijskruitbosch+bugs)
Created attachment 8560584 [details] [diff] [review]
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.
Attachment #8560476 - Attachment is obsolete: true
Attachment #8560584 - Flags: review?(gijskruitbosch+bugs)

Updated

3 years ago
Flags: qe-verify?
Flags: qe-verify? → qe-verify+

Comment 9

3 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... :-)
Attachment #8560584 - Flags: review?(gijskruitbosch+bugs) → review+
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.

Updated

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

Updated

3 years ago
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb

Updated

3 years ago
No longer depends on: 567039
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
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/13b908ad9c77
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
QA Contact: camelia.badau
Created attachment 8572648 [details]
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?
Flags: needinfo?(jaws)
(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?
Flags: needinfo?(jaws) → needinfo?(camelia.badau)
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.
Flags: needinfo?(camelia.badau)
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?
Flags: needinfo?(jaws)
(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?
Flags: needinfo?(jaws)
(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.
Status: RESOLVED → VERIFIED
status-firefox38: fixed → verified
You need to log in before you can comment on or make changes to this bug.