In-content preferences: in dialogs, some buttons are pushed into overflow (on Mac and Linux)

VERIFIED FIXED in Firefox 38

Status

()

defect
P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: cbadau, Assigned: Gijs)

Tracking

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

Firefox Tracking Flags

(firefox38 verified, firefox39 verified, firefox40 verified)

Details

Attachments

(2 attachments)

This bug was filled based on the discussion from bug 1044597 comment 19.

Reproducible on Nightly 39.0a1(BuildID: 20150304030231)
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:39.0) Gecko/20100101 Firefox/39.0
Reproducible on Aurora 38.0a2(BuildID: 20150304004138)
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:38.0) Gecko/20100101 Firefox/38.0

Steps to reproduce: 
1. Go to about:preferences. 
2. Open all dialogs (Set Home Page, Exceptions, Languages, etc.) 
from General, Content, Privacy and Security tabs. 

Actual results: 
- 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

Expected results: All the buttons are visible (on all dialogs). 

Please see attachment "issues Mac+Ubuntu.zip". 

Note: This issue is reproducible on Mac OSX 10.9.5 and Ubuntu 13.10 32bit.
OS: Windows 7 → All
Hardware: x86_64 → All
This seems to be because we're taking the style.width property, which is in em units, and then set that on the container, where 'em' mean something else, so we're causing a reflow (different width) which then alters the height of the dialog content in some cases. Matt says that we do need to prefer style.width over scrollWidth here because apparently there are cases where this is necessary.

Separately, I can't seem to get the scrollbars I added in bug 1128237 to work on any of these dialogs anymore, did that break or is it just me? :-\
Whiteboard: [sf-hackweek]
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 39.3 - 30 Mar
(In reply to :Gijs Kruitbosch (away 26-30/3) from comment #1)
> Matt says that we do need to
> prefer style.width over scrollWidth here because apparently there are cases
> where this is necessary.

Matt, do you remember / can you find out why this was necessary? If not, I'm tempted to just remove this and rely on scrollWidth instead.
Flags: needinfo?(MattN+bmo)
Bug 1043612, while landed, fixed all of the OS X issues for me.
Depends on: 1043612
Ugh. So I tried to fix this by "properly" transferring all of the em widths from the inner to the outer document's values by transforming the em value to px. I was hoping that would make the issue go away.

Unfortunately, it turns out that we've sneakily been making this problem not be as obvious by the thing that transferring the em size was doing: making the window wider than it should be. So doing things more correctly actually makes things *worse*.

There are several issues here:

1) we resize the fonts in the dialogs here with our added stylesheets, but we don't change the document font sizes, and so everything ends up being huge and the dialog then ends up being small because it's sized in "em" and 1em in the default font size is quite small whereas we upsize all the fonts and so then the dialog is tiny by comparison.
2) we read scrollWidth and scrollHeight, and then set both, but after we set the width and a reflow triggers, scrollHeight changes because text reflows (the language example is an obvious example of this with a bunch of wrapping text at the top - obviously if we change the width, the wrapping of said dialog changes and so the height of it changes, too. So we end up with a dialog that's too short for its width.


For the latter, I'm not sure how to force a reflow. Just fetching scrollHeight after setting the width should do this, I think, but it seems to not be working. I don't know why. I'll look at this more tomorrow.
Flags: needinfo?(MattN+bmo)
Jared, if you have time to think about comment #5 tomorrow, let me know.
Flags: needinfo?(jaws)
So this works. Apparently the box's minWidth triggers the scrollHeight update. Don't really know why, but I wrote tests. I also noticed we regressed the 'keep the dialog inside the viewport' logic, so I wrote a test for that too (and fixed it).
Attachment #8593387 - Flags: review?(jaws)
Flags: needinfo?(jaws)
Comment on attachment 8593387 [details] [diff] [review]
fix in-content prefs dialogs overflowing,

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

::: browser/components/preferences/in-content/subdialogs.js
@@ +188,5 @@
> +
> +    // The difference between the frame and box shouldn't change, either:
> +    let boxRect = this._box.getBoundingClientRect();
> +    let frameRect = this._frame.getBoundingClientRect();
> +    let frameSizeDifference = (frameRect.top - boxRect.top) + (boxRect.bottom - frameRect.bottom);

I was thinking of this two weeks ago but couldn't think of how to get this value. Nice!

@@ +213,5 @@
> +    // Do this with a frame height in pixels...
> +    let comparisonFrameHeight;
> +    if (frameHeight.endsWith("em")) {
> +      let fontSize = parseFloat(getComputedStyle(this._frame).fontSize);
> +      comparisonFrameHeight = parseFloat(frameHeight) * fontSize;

parseFloat(frameHeight, 10) just because these can be user-submitted values (from localizers)

@@ +215,5 @@
> +    if (frameHeight.endsWith("em")) {
> +      let fontSize = parseFloat(getComputedStyle(this._frame).fontSize);
> +      comparisonFrameHeight = parseFloat(frameHeight) * fontSize;
> +    } else if (frameHeight.endsWith("px")) {
> +      comparisonFrameHeight = parseFloat(frameHeight);

ditto.
Attachment #8593387 - Flags: review?(jaws) → review+
Considering the issues we had with landing the other bug, trypushing first...:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e443a2acbaa6
(In reply to :Gijs Kruitbosch from comment #9)
> Considering the issues we had with landing the other bug, trypushing
> first...:
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e443a2acbaa6

Delightful. WinXP bc1 failure, asan leaks (which I'm fairly sure are pre-existing intermittents, so I retriggered). I'll see if I can repro the bc1 failure and/or if this impacts the effectiveness of the patch there.
Flags: needinfo?(gijskruitbosch+bugs)
let's see if either of these helps:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=91ef09cdb364

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8d2150ffd56
Flags: needinfo?(gijskruitbosch+bugs)
It turns out that asan is always leaking, and that's not what turns it orange. Green runs on fx-team also have these leaks.

The trypushes fix the XP issue by adding more text.

remote:   https://hg.mozilla.org/integration/fx-team/rev/0c3f0143bed5
https://hg.mozilla.org/mozilla-central/rev/0c3f0143bed5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8593387 [details] [diff] [review]
fix in-content prefs dialogs overflowing,

Approval Request Comment
[Feature/regressing bug #]: in-content prefs
[User impact if declined]: buttons and/or other content is cut off in dialogs in in-content prefs, and/or dialogs are too big for the window
[Describe test coverage new/current, TreeHerder]: has more tests checked in with this patch!
[Risks and why]: medium-low, fiddling with this code could theoretically cause other issues of the same kind that the tests and/or my thought process didn't consider. I would have preferred getting this done + uplifted earlier in beta, but here we are. I do still believe the risk/reward here is such that uplifting it is the right decision at this point, because the current state is not OK and the patch significantly improves matters, and any risk is contained to the same process (resizing dialogs) that the patch affects.
[String/UUID change made/needed]: nope
Attachment #8593387 - Flags: approval-mozilla-beta?
Attachment #8593387 - Flags: approval-mozilla-aurora?
Iteration: 39.3 - 30 Mar → 40.2 - 27 Apr
Flags: qe-verify?
Flags: firefox-backlog+
Flags: qe-verify? → qe-verify+
QA Contact: camelia.badau
Comment on attachment 8593387 [details] [diff] [review]
fix in-content prefs dialogs overflowing,

Should be in 38 beta 6.
Attachment #8593387 - Flags: approval-mozilla-beta?
Attachment #8593387 - Flags: approval-mozilla-beta+
Attachment #8593387 - Flags: approval-mozilla-aurora?
Attachment #8593387 - Flags: approval-mozilla-aurora+
Verified fixed on Mac OSX 10.9.5 and Ubuntu 13.10 32bit using latest Nightly 40.0a1 (buildID: 20150421092928), latest Aurora 39.0a2 (buildID: 20150421004053) and Firefox 38 Beta 6 (buildID: 20150420134330): all the buttons are visible (on all dialogs).

Only one mention here: on latest Nightly 40.0a1, the following dialogs are no longer resizable: Content -> "Advanced"(Fonts); Content -> "Colors"; Privacy ->  "Settings for Clearing History"; Security -> "Change Master Password". It is intended behaviour? On latest Aurora 39.0a2 and on Firefox 38 Beta 6, all the dialogs are resizable.
Status: RESOLVED → VERIFIED
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Camelia Badau, QA [:cbadau] from comment #18)
> Verified fixed on Mac OSX 10.9.5 and Ubuntu 13.10 32bit using latest Nightly
> 40.0a1 (buildID: 20150421092928), latest Aurora 39.0a2 (buildID:
> 20150421004053) and Firefox 38 Beta 6 (buildID: 20150420134330): all the
> buttons are visible (on all dialogs).
> 
> Only one mention here: on latest Nightly 40.0a1, the following dialogs are
> no longer resizable: Content -> "Advanced"(Fonts); Content -> "Colors";
> Privacy ->  "Settings for Clearing History"; Security -> "Change Master
> Password". It is intended behaviour? On latest Aurora 39.0a2 and on Firefox
> 38 Beta 6, all the dialogs are resizable.

Yes, this was changed in bug 1153403, so that's expected.

Thanks for verifying. This makes me happy. Am I right in thinking that now bug 1146807 is also fixed?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(camelia.badau)
(In reply to :Gijs Kruitbosch from comment #19)
> (In reply to Camelia Badau, QA [:cbadau] from comment #18)
> > Verified fixed on Mac OSX 10.9.5 and Ubuntu 13.10 32bit using latest Nightly
> > 40.0a1 (buildID: 20150421092928), latest Aurora 39.0a2 (buildID:
> > 20150421004053) and Firefox 38 Beta 6 (buildID: 20150420134330): all the
> > buttons are visible (on all dialogs).
> > 
> > Only one mention here: on latest Nightly 40.0a1, the following dialogs are
> > no longer resizable: Content -> "Advanced"(Fonts); Content -> "Colors";
> > Privacy ->  "Settings for Clearing History"; Security -> "Change Master
> > Password". It is intended behaviour? On latest Aurora 39.0a2 and on Firefox
> > 38 Beta 6, all the dialogs are resizable.
> 
> Yes, this was changed in bug 1153403, so that's expected.
> 
> Thanks for verifying. This makes me happy. Am I right in thinking that now
> bug 1146807 is also fixed?

Thanks for your answer! 

Yes, you are right: the bug 1146807 is also fixed.
Flags: needinfo?(camelia.badau)
You need to log in before you can comment on or make changes to this bug.