Closed Bug 1035629 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Preferences, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 34

People

(Reporter: MattN, Assigned: Paenglab, Mentored)

References

()

Details

(Whiteboard: [bugday-20140820])

Attachments

(1 file, 1 obsolete file)

Convert privacy.js to use gSubDialog.
Attached patch privacyDialogsIncontent.patch (obsolete) — Splinter Review
To look correct it needs bug 1035540 applied first.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8452601 - Flags: review?(MattN+bmo)
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.
Attachment #8452601 - Flags: review?(MattN+bmo) → feedback+
(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.
Attachment #8452601 - Attachment is obsolete: true
Attachment #8453271 - Flags: review?(MattN+bmo)
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.
Attachment #8453271 - Flags: review?(dao)
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.
Attachment #8453271 - Flags: review?(dao)
Attachment #8453271 - Flags: review?(MattN+bmo)
Attachment #8453271 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3b35a8e4da0e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
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?
QA Whiteboard: [bugday-20140820]
Flags: needinfo?(richard.marti)
This is true and will be handled in bug 1036595.
Flags: needinfo?(richard.marti)
(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.
Summary: Convert Privacy pane dialogs to be in-content → Convert Privacy pane individual cookie removal dialog to be in-content
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
Status: RESOLVED → VERIFIED
Whiteboard: [bugday-20140820]
You need to log in before you can comment on or make changes to this bug.