1035629 - Convert Privacy pane individual cookie removal dialog to be in-content

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Matthew N. [:MattN]]

Convert privacy.js to use gSubDialog.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: privacyDialogsIncontent.patch

To look correct it needs bug 1035540 applied first.

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8452601 [details] [diff] [review]
privacyDialogsIncontent.patch

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

::: browser/components/preferences/in-content/privacy.js
@@ +477,5 @@
>     * Displays all the user's cookies in a dialog.
>     */
>    showCookies: function (aCategory)
>    {
> +    gSubDialog.open("chrome://browser/content/preferences/cookies.xul");

This dialog doesn't have enough padding at the bottom on OS X. Maybe add padding-bottom: 6px to #dialogBox or figure out why this dialog is different and make a better fix. f+ for now but I can be convinced to r+ if there is a bigger plan to fix this.

@@ +503,5 @@
>    /**
>     * Displays a dialog from which individual parts of private data may be
>     * cleared.
>     */
>    clearPrivateDataNow: function (aClearEverything)

Since you're not doing it here, can you get a bug on file about making this dialog in-content. You can trigger it when "Remember History" is specified and you click the "clear your recent history" link.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: privacyDialogsIncontent.patch

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #2)
> Comment on attachment 8452601 [details] [diff] [review]
> privacyDialogsIncontent.patch
> 
> Review of attachment 8452601 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/in-content/privacy.js
> @@ +477,5 @@
> >     * Displays all the user's cookies in a dialog.
> >     */
> >    showCookies: function (aCategory)
> >    {
> > +    gSubDialog.open("chrome://browser/content/preferences/cookies.xul");
> 
> This dialog doesn't have enough padding at the bottom on OS X. Maybe add
> padding-bottom: 6px to #dialogBox or figure out why this dialog is different
> and make a better fix. f+ for now but I can be convinced to r+ if there is a
> bigger plan to fix this.

This is really a strange beast. On Linux and Windows the height is correctly calculated but OS X needs this additional padding. For me this is a bit a hack but I don't know why only OS X calculates this differently and how to fix this without the padding-bottom I added to the OS X file..
 
> @@ +503,5 @@
> >    /**
> >     * Displays a dialog from which individual parts of private data may be
> >     * cleared.
> >     */
> >    clearPrivateDataNow: function (aClearEverything)
> 
> Since you're not doing it here, can you get a bug on file about making this
> dialog in-content. You can trigger it when "Remember History" is specified
> and you click the "clear your recent history" link.

Filed bug 1036595.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8453271 [details] [diff] [review]
privacyDialogsIncontent.patch

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

Dão, any chance you could try find some cleaner CSS for Richard that isn't specific to one dialog? I haven't had time to look at this due to search/Loop stuff.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8453271 [details] [diff] [review]
privacyDialogsIncontent.patch

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

r=me without the CSS change by adding the missing padding-bottom in the height calculation instead. I'll make this change and land it now.

::: browser/themes/osx/preferences/in-content/preferences.css
@@ +96,5 @@
>  }
> +
> +/* Add to Cookies dialog a bottom padding */
> +#CookiesDialog > hbox > .actionButtons {
> +  padding-bottom: 10px;

It seems like this is only necessary since the padding-bottom of #CookiesDialog isn't included in the height calculation of the <browser> for some reason despite MDN saying the padding should be included in scrollHeight. It seems like we should fix the height calculation JS to include padding instead of hard-coding this. I confirmed with a reduced testcase that the padding-top is included in scrollHeight but padding-bottom is not. The same applies to <prefwindow> so I think it applies to any document element. It's not clear to me whether this is a bug in XUL or not since there is no real spec.

Comment 6

[Matthew N. [:MattN]]

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

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3b35a8e4da0e

Comment 8

[Ricardo Maçãs [:Ricmacas]]

Reproduced bug on older build Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0
I cannot confirm fix on latest Nightly Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0 because even though the individual cookies setting opens an in-content dialog, the "clear your recent history" link is not currently opening an in-content dialog.
Richard, is this expected?

Comment 9

[Richard Marti (:Paenglab)]

This is true and will be handled in bug 1036595.

Comment 10

[Ricardo Maçãs [:Ricmacas]]

(In reply to Richard Marti (:Paenglab) from comment #9)
> This is true and will be handled in bug 1036595.

Ok, then as per my last comment, pending verification in another platform I recommend this bug to be set as VERIFIED.
Since this bug is marked as fixed, I've changed title to better reflect what is fixed in this bug.

Comment 11

[alex_mayorga]

Clicking "remove individual cookies" on about:preferences#privacy open in-content on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140820030202 CSet: ffdd1a398105