Closed Bug 1036595 Opened 10 years ago Closed 8 years ago

Convert the "Clear recent history" dialog on privacy page to be in-content

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: Paenglab, Assigned: jyeh)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

Bug 1036595 - Convert the 'Clear recent history' dialog to be in-content;
58 bytes, text/x-review-board-request
MattN
: review+
Details
clear-recent-history-dialog-height.png
243.69 KB, image/png
Details 
This needs a

gSubDialog.open("chrome://browser/content/sanitize.xul");

on line http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#518

but it needs also some logic to switch to "everything" when on "never remember history" (aClearEverything).

Also does the dialog not grow when changing from "Last Hour" etc. to "Everything".
It should not be forgotten here that the "Clear history" dialog can appear on every pages triggered by the shortcut (Ctrl+shift+delete) or the item in the history subview.
(In reply to Guillaume C. [:ge3k0s] from comment #1)

And it should stay like that. "Clear Recent History" dialog is one of the very few dialogs that should stay modal and non-incontent, because clearing recent history is usually a quick, swift action of Ctrl+Shift+Delete and Enter, and right after finishing the action, the user can get back to what they've been doing before. Making this dialog incontent means having to close yet another tab, and having to deal with the confusion of why a new tab opened when the shortcut has been pressed.

On the other hand, I do think that the "Clear Recent History" button should be added to the in-content settings, tab Privacy, right next to the "Firefox will: Never/Remember/Custom settings for History" dropdown menu.
It seems reasonable to leave it as a dialog when it's not launched from the preferences tab.
It would be nice to have the new styling everywhere though.
Hi, I would like to try this bug :)
Assignee: nobody → jyeh
Joseph, I think this bug is about making it a subdialog using gSubDialog. You can search for other examples of it being used in preferences.
Hi Matthew, I use gSubDialog to open the dialog and pass the aClosingCallback param to do things that need to be reset.

Thanks!
Comment on attachment 8771255 [details]
Bug 1036595 - Convert the 'Clear recent history' dialog to be in-content;

https://reviewboard.mozilla.org/r/64502/#review61504

\[x\] it looks like there are still remaining callers for nsIBrowserGlue::sanitize so we don't need to worry about removing that API.
\[x\] No arguments being passed to sanitize.xul looks correct when comparing to https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/browser/base/content/sanitize.js#806
Looks good to me.

The main issue I see is that the subdialog isn't resizing like the window does when you toggle between time ranges or toggle open the "Details" which means that stuff gets clipped at the bottom.

STR:
1) In the dialog, collapse "Details". Close the subdialog.
2) Open in from privacy prefs again.
3) Expand "Details"

Expected result:
Buttons aren't clipped on the bottom.

Actual result:
Buttons are clipped on the bottom.

The problem is even worse if you toggle from a time range to "Everything".

I suspect the problem is that window.resizeBy doesn't work in the iframe of the subdialog: https://dxr.mozilla.org/mozilla-central/search?q=path%3Acontent%2FsanitizeDialog.js+window.&redirect=false
I think we either need to override window.resizeBy in subdialogs.js like we do for window.open at https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/browser/components/preferences/in-content/subdialogs.js#175
or have the dialog open with a larger size by default which may lead to extraneous whitespace in some cases. There are probably some other possible solutions/workarounds too. For example, is it even valuable to be able to collapse the "Details" in the first place. That just seems like it could lead to a bad surprise. Of course that won't fix the issue with the warning for "Everything".

I would start with looking at resizeBy as it seems fairly straightforward to implement without having touched this code for over a year. You may want to refer to the _onLoad code in subdialogs.js for help with the resizeBy shim implementation.
Attachment #8771255 - Flags: review?(MattN+bmo) → review-
FWIW there is a long-standing issue with the "clear browsing dialog" window that doesn't resize correctly. See bug 814565.
I have some question about the dialog's behavior. I made some screenshots with different clear history dialog sizes. 

As described in bug 814565, I supposed dialog 3, 4, 7 and 8 are not the expected result, right?

If the dialog should expand and shrink normally, should the dialog be resizable when it is in-content? 

Thanks!
Flags: needinfo?(MattN+bmo)
Attached patch wip-bug-1036595.patch (obsolete) — — Splinter Review 
Hi Matthew,

I made another patch with an override on resizeBy by calculating the size inside the frame and then set the height.

Some parts may look like workaround, such as using setTimeout in order to get the correct value of the frame height. I am not sure if there is a formal way to do this. Can you give me some feedback for this patch?

Thanks :)
Attachment #8771255 - Attachment is obsolete: true
Attachment #8774242 - Flags: feedback?(MattN+bmo)
Comment on attachment 8774242 [details] [diff] [review]
wip-bug-1036595.patch

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

::: browser/components/preferences/in-content/subdialogs.js
@@ +176,5 @@
> +    this._frame.contentWindow.resizeBy = function(resizeByWidth, resizeByHeight) {
> +      let docEl = gSubDialog._frame.contentDocument.documentElement;
> +
> +      if (docEl.id === "SanitizeDialog") {
> +        let groupBoxTitle = document.getAnonymousElementByAttribute(gSubDialog._box, "class", "groupbox-title");

Do we really need code specific to one dialog in subdialogs.js? It should be generic for the most part. Why can't this code run for all subdialogs?
(In reply to Joseph Yeh [:joseph] from comment #11)
> If the dialog should expand and shrink normally, should the dialog be
> resizable when it is in-content? 

When the window is shown from the History menu it isn't resizable so I think it's fine to not have it resizable in-content either. This means you can set the features (2nd) argument to `open` to not include "resizable"[1] which I think would mean: "dialog=no,centerscreen"

[1] https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/browser/components/preferences/in-content/subdialogs.js#73
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] from comment #13)
> Do we really need code specific to one dialog in subdialogs.js? It should be
> generic for the most part. Why can't this code run for all subdialogs?

Not every subdialogs contain paneDeckContainer and prefWindow-dlgbuttons which I use to calculate the height of the frame. 

To make it generic, should I use `document.getAnonymousNodes` and iterate through all nodes to calculate the height?
Attached patch wip-bug-1036595.patch (obsolete) — — Splinter Review 
Hi Matthew, I update the patch with more generic solutions and also make it non-resizable. Please take a look at it, thanks :)
Attachment #8774242 - Attachment is obsolete: true
Attachment #8774242 - Flags: feedback?(MattN+bmo)
Attachment #8775065 - Flags: feedback?(MattN+bmo)
Can you give an overview of what your patch is doing? It seems a lot more complicated than I would expect for a method which simply increases the width or height of the frame by a fixed number of pixels.
Flags: needinfo?(jyeh)
Attached patch wip-bug-1036595.patch (obsolete) — — Splinter Review 
Update the patch as I found some parts can be simpler.
Attachment #8775065 - Attachment is obsolete: true
Attachment #8775065 - Flags: feedback?(MattN+bmo)
Comment on attachment 8776845 [details] [diff] [review]
wip-bug-1036595.patch

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

::: browser/components/preferences/in-content/subdialogs.js
@@ +172,5 @@
>  
>      this._frame.contentWindow.addEventListener("dialogclosing", this);
>  
> +    let oldResizeBy = this._frame.contentWindow.resizeBy;
> +    this._frame.contentWindow.resizeBy = function(resizeByWidth, resizeByHeight) {

Override the original `resizeBy` function.

@@ +180,5 @@
> +
> +      let groupBoxBody = document.getAnonymousElementByAttribute(gSubDialog._box, "class", "groupbox-body");
> +
> +      let boxVerticalPadding = 2 * parseFloat(getComputedStyle(groupBoxBody).paddingTop);
> +      let boxVerticalBorder = 2 * parseFloat(getComputedStyle(gSubDialog._box).borderTopWidth);

Get the information in order to calculate the height of the box.

These code were copied from the `_onLoad` function.

I am wondering if we need to extract these code into a method so they can be reused in this function and the `_onLoad` function.

@@ +182,5 @@
> +
> +      let boxVerticalPadding = 2 * parseFloat(getComputedStyle(groupBoxBody).paddingTop);
> +      let boxVerticalBorder = 2 * parseFloat(getComputedStyle(gSubDialog._box).borderTopWidth);
> +
> +      let frameHeight = gSubDialog._frame.clientHeight + resizeByHeight;

New frame height will be current frame height added by `resizeByHeight`.

@@ +187,5 @@
> +
> +      gSubDialog._frame.style.height = frameHeight + "px";
> +      gSubDialog._box.style.minHeight =
> +        "calc(" + (boxVerticalBorder + groupBoxTitleHeight + boxVerticalPadding) +
> +        "px + " + frameHeight + "px)";

Update frame's height and box's minHeight.

@@ +285,4 @@
>        }
>      }
>  
> +    let docAnonymousNodes = document.getAnonymousNodes(docEl);

We will need to recalculate the frame height here because the frame height may contain some blank space. Those blank space will be expanded when we resize the height by adding the `resizeByHeight` directly. 

You can see the dialog 3 and 4 in the previous attachment 'clear-recent-history-dialog-height.png' for the blank space issue.

@@ +289,5 @@
> +
> +    if (docAnonymousNodes && !docEl.style.height) {
> +      let docAnonymousNodesHeight = 0;
> +
> +      for (let docAnonymousNode of docAnonymousNodes) {

We iterate through all child nodes here in order to calculate the frame's height by adding the child node's height.

@@ +296,5 @@
> +        // Enable frame shrinking to smaller size.
> +        if (docAnonymousNode.flex === "1") {
> +          docAnonymousNode.flex = "";
> +          hasFlex = true;
> +        }

The `flex` attribute will cause the node expand its height to fit its parent's height which will make some extra blank space. In order to get the correct height of this node, we will need to remove this attribute for temporary.

@@ +298,5 @@
> +          docAnonymousNode.flex = "";
> +          hasFlex = true;
> +        }
> +
> +        docAnonymousNodesHeight += docAnonymousNode.clientHeight;

Calculating the height here.

@@ +304,5 @@
> +        // Restore flex attribute.
> +        if (hasFlex) {
> +          docAnonymousNode.flex = "1";
> +          hasFlex = false;
> +        }

Restore the `flex` attribute after we done calculating. 

I am not sure if removing and restoring this attribute will cause any side effects, perhaps you can give some advise here.

@@ +309,5 @@
> +      }
> +
> +      frameHeight = frameMinHeight = docAnonymousNodesHeight +
> +        parseFloat(getComputedStyle(docEl).paddingTop) +
> +        parseFloat(getComputedStyle(docEl).paddingBottom) + "px";

So the frame height will be child nodes height plus the padding.

@@ +333,5 @@
> +
> +    // Prevent frame width and height from being removed.
> +    if (document.getAnonymousNodes(docEl)) {
> +      return;
> +    }

We will need to prevent the frame's width and height from being removed here. Otherwise, when we update the height in the `resizeBy` function, it will trigger the `_onResize` function here and then remove the frame's width and height (line 343-344). I'm not sure what this intended to do, but it will cause the frame size being changed which is pretty weird.

This is a very straightforward solution, I am wondering if there is a better solution.
Notice that this patch only implement the resizing height part. Currently we don't have any width changing with `resizeBy` in sanitizeDialog. 

However, we should provide both width and height `resizeBy` implementation to make this more generic. I'll add the resizing width part when the implementation on resizing height are reviewed. I think they should be implemented in a similar way.

Thanks!
Flags: needinfo?(jyeh)
Attachment #8776845 - Flags: feedback?(MattN+bmo)
The reason for recalculating the height is due to the blank space issue, you can see more details in the previous comment. I am not sure if it is the right way to solve this problem, but I hope my explanation will satisfy your concerns :)
Comment on attachment 8776845 [details] [diff] [review]
wip-bug-1036595.patch

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

It seems like you're solving multiple cases at once and that makes it hard for me to understand the complexity. Lets start by not worrying about the case where we start large and then need to shrink as that doesn't cause UX issues, only UI ugliness. Let's have one patch that changes privacy.js, another to handle resizeBy increasing the height by the relative amount (see my upcoming trivial patch that needs to still handle max height), and then we can figure out other issues separately as needed.

::: browser/components/preferences/in-content/subdialogs.js
@@ +180,5 @@
> +
> +      let groupBoxBody = document.getAnonymousElementByAttribute(gSubDialog._box, "class", "groupbox-body");
> +
> +      let boxVerticalPadding = 2 * parseFloat(getComputedStyle(groupBoxBody).paddingTop);
> +      let boxVerticalBorder = 2 * parseFloat(getComputedStyle(gSubDialog._box).borderTopWidth);

It seems like it would be useful to have a frameHeight setter that takes a pixel value and prevents setting a height larger than the maxHeight desired.

I'm not really sure why we even care about some of these numbers as my naive solution was to just adjust the height by the amount that the resizeBy caller says. What's wrong with that approach? I'll attach a simple patch which seems to work for the most part.

@@ +329,4 @@
>  
>    _onResize: function(mutations) {
>      let frame = gSubDialog._frame;
> +    let docEl = frame.contentDocument.documentElement;

Perhaps the resize observer should only be setup for resizable dialogs with @resizable?
Attachment #8776845 - Flags: feedback?(MattN+bmo) → feedback-
This patch only contains height adjustment in resizeBy. 

If you apply the patch, you can see the height resizing is working, but also cause two problems.

The first problem is the width of the frame also got changed. It results from the `_onResize` function that will remove the property of frame's style.

And the second one is the blank space issue which I already mentioned before. If this should solve in a different patch then it should be fine now.


In `subdialogs.js` in the previous patch (i.e. wip-bug-1036595.patch), 

L332-337 will solve the first problem

and L288-314 will solve the second problem.


I keep the previous patch so you can tell the difference between them.


Thanks!
Attachment #8781730 - Attachment is obsolete: true
Attachment #8782376 - Flags: feedback?(MattN+bmo)
(In reply to Matthew N. [:MattN] from comment #22)
> @@ +329,4 @@
> >  
> >    _onResize: function(mutations) {
> >      let frame = gSubDialog._frame;
> > +    let docEl = frame.contentDocument.documentElement;
> 
> Perhaps the resize observer should only be setup for resizable dialogs with
> @resizable?

This looks reasonable and works.
Hi Joseph, it would be great if you could add a screenshot test to https://dxr.mozilla.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Preferences.jsm like we have for panePrivacy-DNTDialog and then do a try push so we can check that it looks decent on the OSs we test in automation.

See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots
(In reply to Joseph Yeh [:joseph] from comment #25)
> (In reply to Matthew N. [:MattN] from comment #22)
> > @@ +329,4 @@
> > >  
> > >    _onResize: function(mutations) {
> > >      let frame = gSubDialog._frame;
> > > +    let docEl = frame.contentDocument.documentElement;
> > 
> > Perhaps the resize observer should only be setup for resizable dialogs with
> > @resizable?
> 
> This looks reasonable and works.

I'm confused. Did you mean to attach a new patch or was comment 24's patch already with that fix even though comment 24 mentions _onResize getting called and causing a problem?
Attachment #8782376 - Flags: feedback?(MattN+bmo) → feedback+
Sorry for the confusing. I mean to add it in the new patch.
Attachment #8776845 - Attachment is obsolete: true
Attachment #8782376 - Attachment is obsolete: true
Attachment #8785850 - Flags: review?(MattN+bmo)
Comment on attachment 8785850 [details] [diff] [review]
Bug-1036595-Convert-the-Clear-recent-history-dialog-to-be-in-content.patch

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

Hi Joseph, Please use MozReview for future patches (or at least include the recommend 8 lines of context in .patch files).

Instead of an integration test I still think that adding configurations to mozscreenshots would be useful to avoid this regressing. See comment 26.

r=me but this needs either an automated test or mozscreenshot configurations before landing since this is a complicated bit of code with all of the calculations.

::: browser/components/preferences/in-content/subdialogs.js
@@ +172,5 @@
>  
>      this._frame.contentWindow.addEventListener("dialogclosing", this);
>  
> +    let oldResizeBy = this._frame.contentWindow.resizeBy;
> +    this._frame.contentWindow.resizeBy = function(resizeByWidth, resizeByHeight) {

Add a comment to note that we only handle `resizeByHeight` currently.

@@ +177,5 @@
> +      let frameHeight = gSubDialog._frame.clientHeight;
> +      let boxMinHeight = parseFloat(getComputedStyle(gSubDialog._box).minHeight, 10);
> +
> +      gSubDialog._frame.style.height = (frameHeight + resizeByHeight) + "px";
> +      gSubDialog._box.style.minHeight = (boxMinHeight + resizeByHeight) + "px";

It seems like we don't need to adjust minHeight in all cases as long as the new height is greater than minHeight but I can see how it would be more complicated to keep track of which number needs to be adjusted so that we can then collapse the frame later. With that in mind I think it's okay to adjust both together like you are here.

@@ +178,5 @@
> +      let boxMinHeight = parseFloat(getComputedStyle(gSubDialog._box).minHeight, 10);
> +
> +      gSubDialog._frame.style.height = (frameHeight + resizeByHeight) + "px";
> +      gSubDialog._box.style.minHeight = (boxMinHeight + resizeByHeight) + "px";
> +

It seems like clobbering .minHeight's `calc` expression is okay here but if that ends up being a problem we could use a CSS variable for `frameMinHeight` in the calc expression and then modify that variable here instead. That's an alternate solution for `height` as well that doesn't require calculations btw.

Basically we would have a CSS variable that keeps track of the cumulative `resizeByHeight` (initialize to 0) and have that as part of the initial style in a `calc()`.

I think it's okay as-is though for now.

@@ +283,4 @@
>      this._overlay.style.visibility = "visible";
>      this._overlay.style.opacity = ""; // XXX: focus hack continued from _onContentLoaded
>  
> +    if (this._box.getAttribute("resizable") === "true") {

Nit: double equals is preferred in m-c by default unless === is needed. In this case only == is necessary.
Attachment #8785850 - Flags: review?(MattN+bmo) → review+
Attachment #8785850 - Attachment is obsolete: true
Comment on attachment 8771255 [details]
Bug 1036595 - Convert the 'Clear recent history' dialog to be in-content;

https://reviewboard.mozilla.org/r/64502/#review75270

(In reply to Joseph Yeh [:joseph] from comment #31)
> I try the screenshot test, but I am not sure if this is correct.

I think you may have misunderstood comment 26. Preferences.jsm doesn't currently take a screenshot of the `Clear Recent History` subdialog so we would need to add a configuration to that file to have it captured (like we did with panePrivacy-DNTDialog). Then you can do a try push to see how it looks on each platform (like you did).

The code changes so far are fine but I don't want to land this before seeing the screenshots on the other platforms so please add the screenshot configuration.
Attachment #8771255 - Flags: review?(MattN+bmo)
I add a configuration named panePrivacy-clearRecentHistoryDialog to the screenshot test.

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=938ce16be25f9c551c19ef8938e8717ed3d41ff5&newProject=try&newRev=69ecd7f2ca22233cd35d9c39ec11977ee838ef0a

Also found out that the `detailsExpander` button will get cropped on Ubuntu, so I move the #detailsExpanderWrapper 2px backward in sanitizeDialog.css.
I did a try push of my own which captured screenshots of the Details section expanded and I notice that the blank space issue is there still. I thought you were mentioning in comment 28 that the newer patches fixed that issue?

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=938ce16be25f9c551c19ef8938e8717ed3d41ff5&newProject=try&newRev=44348b9e93207e525f842b143577b9589790ebff&filter=clearRecentHistory
Flags: needinfo?(jyeh)
(In reply to Matthew N. [:MattN] from comment #35)
> I did a try push of my own which captured screenshots of the Details section
> expanded and I notice that the blank space issue is there still. I thought
> you were mentioning in comment 28 that the newer patches fixed that issue?

No, what I mentioned in comment 28 only solved the first issue in comment 24. The blank space issue (which is the second issue in comment 24) still exists, but I am not sure if we are going to fix it in this bug (perhaps in bug 814565?).
Flags: needinfo?(jyeh)
Hi Hatthew, I just come up with a simple solution.

Since the blank space only occurred when the Details section is collapsed, maybe we can always expand it when the dialog opened. Just like when we select 'Everthing', the dialog will always opened with the Details section expanded.

Do you think this is a proper solution?
Flags: needinfo?(MattN+bmo)
Comment on attachment 8771255 [details]
Bug 1036595 - Convert the 'Clear recent history' dialog to be in-content;

I considered that option but I think it's probably not ideal for UX. First I would like to know what is making the initial height too high when details are hidden by default? Is it a calculation in gSubdialogs or is it some CSS (min-)height rule?
Flags: needinfo?(MattN+bmo)
Attachment #8771255 - Flags: review?(MattN+bmo)
https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/incontentprefs/preferences.inc.css#372

It seems like the height from #dialogFrame will effect the initial height of #SanitizeDialog. Should we remove it or make it smaller to solve this issue?
Flags: needinfo?(MattN+bmo)
Update patch with reducing the initial height of #dialogFrame and also rebase the screenshot test to the latest build.
I made several screenshot tests to demonstrate that the initial height of #dialogFrame is effecting the initial height of #SanitizeDialog.

I am not sure how to compare between two revisions, so I will just list them separately.


1. Remove the height of #dialogFrame (https://hg.mozilla.org/try/rev/85c1b7664dcc2e986b6f570b7d10f13ae7b34db6#l4.12)

Screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=f7d5008ee2ab9200052e45ad6ecc3f3a348f7f86&newProject=try&newRev=85c1b7664dcc2e986b6f570b7d10f13ae7b34db6&filter=clearRecentHistory


2. Reduce the height of #dialogFrame to 10em (https://hg.mozilla.org/try/rev/d67fb133cd5664dc71b3da91d0bc0e0ecb706189#l4.13)

Screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd&newProject=try&newRev=d67fb133cd5664dc71b3da91d0bc0e0ecb706189&filter=clearRecentHistory


3. Reduce the height of #dialogFrame to 9em (https://hg.mozilla.org/try/rev/1d57556ca718ad1d2299dc3177031d0023e2025c#l4.13)

Screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=7c576fe3279d87543f0a03b844eba7bc215e17f1&newProject=try&newRev=1d57556ca718ad1d2299dc3177031d0023e2025c&filter=clearRecentHistory


4. Reduce the height of #dialogFrame to 8em (https://hg.mozilla.org/try/rev/699f1df24acf2f3eb1dea863c515c3d844c6c65a#l4.13)

Screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=f7d5008ee2ab9200052e45ad6ecc3f3a348f7f86&newProject=try&newRev=699f1df24acf2f3eb1dea863c515c3d844c6c65a&filter=clearRecentHistory


The screenshots show that #SanitizeDialog are having same height in revision 1 and 4. So I think reducing the height of #dialogFrame to 8em should be more appropriate.
Comment on attachment 8771255 [details]
Bug 1036595 - Convert the 'Clear recent history' dialog to be in-content;

https://reviewboard.mozilla.org/r/64502/#review86404

::: browser/themes/shared/incontentprefs/preferences.inc.css:371
(Diff revisions 4 - 5)
>    /* Default dialog dimensions */
> -  height: 10em;
> +  height: 8em;

I tested your patch that simply removed this and it seemed like it wasn't really useful anyways so I'm fine with landing this with the height property removed. I think it may have been necessary before subdialogs got smarter about height calculations.
Attachment #8771255 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8771255 [details]
Bug 1036595 - Convert the 'Clear recent history' dialog to be in-content;

https://reviewboard.mozilla.org/r/64502/#review86404

> I tested your patch that simply removed this and it seemed like it wasn't really useful anyways so I'm fine with landing this with the height property removed. I think it may have been necessary before subdialogs got smarter about height calculations.

And apologies for the delay. I will push this now.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/fx-team/rev/ba6fc133264d
Convert the 'Clear recent history' dialog to be in-content; r=MattN
(In reply to Matthew N. [:MattN] from comment #45)
> And apologies for the delay. I will push this now.

Thanks! Glad to see this landing :)
Flags: needinfo?(MattN+bmo)
Does this patch also fix bug 814565 ?
(In reply to Guillaume C. [:ge3k0s] from comment #48)
> Does this patch also fix bug 814565 ?

Yes, looks like the same issue in this bug.
(In reply to Joseph Yeh [:joseph] from comment #49)
> (In reply to Guillaume C. [:ge3k0s] from comment #48)
> > Does this patch also fix bug 814565 ?
> 
> Yes, looks like the same issue in this bug.

I've just tested latest fx-team build. The clear history dialog is in-content and resizes correctly (except for a small issue mentioned below), but the classic dialog still doesn't resize correctly. 

Concerning the in-content dialog sometime an unnecessary scrollbar appears when the dialog is expanded (at least on Windows).
Quick note : the scrollbar issue seems to be only present on hi-dpi screens.
https://hg.mozilla.org/mozilla-central/rev/ba6fc133264d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
(In reply to Guillaume C. [:ge3k0s] from comment #50)
> Concerning the in-content dialog sometime an unnecessary scrollbar appears
> when the dialog is expanded (at least on Windows).

Can you please file a new bug with STR? I suspect that at least one of subdialog.js calculations aren't talking the DPI into account.
Flags: needinfo?(ge3k0s)
(In reply to Matthew N. [:MattN] from comment #53)
> (In reply to Guillaume C. [:ge3k0s] from comment #50)
> > Concerning the in-content dialog sometime an unnecessary scrollbar appears
> > when the dialog is expanded (at least on Windows).
> 
> Can you please file a new bug with STR? I suspect that at least one of
> subdialog.js calculations aren't talking the DPI into account.

Filed bug 1313029. It would be nice to fix bug 814565 for the classic dialog since it seems to be fixed in-content.
Depends on: 1313029
Flags: needinfo?(ge3k0s)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: