1035541 - Convert Content pane dialogs to be in-content

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Matthew N. [:MattN]]

Convert colors.xul openers to use gSubDialog.

Comment 1

[Richard Marti (:Paenglab)]

This is already in bug 752197 included. Or what needs to add in this bug?

Comment 2

[Richard Marti (:Paenglab)]

Attachment: contentDialogsIncontent.patch

There are the Fonts and Tranlation dialogs which are needed to convert.

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #63)
> Comment on attachment 8448050 [details] [diff] [review]
> inContentDialogs.patch
> 
> Review of attachment 8448050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +905,5 @@
> >   * End sub-dialog
> >   */
> > +
> > +#color-separator {
> > +  width: 1.5em;
> 
> I don't think specific sub-dialog IDs belong in this file. If you want to
> style the tabbox widgets when they're in a sub-dialog, do so in a more
> generic way.

Is this generic enough:

dialog groupbox,
prefwindow groupbox {
  -moz-margin-start: 8px;
  -moz-margin-end: 8px;
}

Comment 3

[Richard Marti (:Paenglab)]

Forgot to write: this patch needs the patch from bug 1035540 applied first.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8452545 [details] [diff] [review]
contentDialogsIncontent.patch

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

::: browser/components/preferences/fonts.xul
@@ +18,5 @@
>              dlgbuttons="accept,cancel,help"
>              ondialoghelp="openPrefsHelp()"
>  #ifdef XP_UNIX
>  #ifdef XP_MACOSX
> +            style="width: &window.macWidth; !important; height: 36em;">

This height is not enough for me on 10.9 with HiDPI (the buttons are cut off). The auto-calculated height works fine though so why do we need this?

Using an empty style="" attribute and having both dimensions auto-calculated works for me. (I filed bug 1036018 about missing @style on the dialog documentElement)

Comment 5

[Richard Marti (:Paenglab)]

Attachment: contentDialogsIncontent.patch

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #4)
> Comment on attachment 8452545 [details] [diff] [review]
> contentDialogsIncontent.patch
> 
> Review of attachment 8452545 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/fonts.xul
> @@ +18,5 @@
> >              dlgbuttons="accept,cancel,help"
> >              ondialoghelp="openPrefsHelp()"
> >  #ifdef XP_UNIX
> >  #ifdef XP_MACOSX
> > +            style="width: &window.macWidth; !important; height: 36em;">
> 
> This height is not enough for me on 10.9 with HiDPI (the buttons are cut
> off). The auto-calculated height works fine though so why do we need this?

Removed the fonts.xul changes. It works now also without them on Windows. I don't know why it was needed earlier.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8452600 [details] [diff] [review]
contentDialogsIncontent.patch

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

r- for fonts.xul. Please test that change on Windows too then ask Mano@mozilla.… for review on that version since he worked on adding the width entities. Perhaps they are still needed for some locales?

::: browser/components/preferences/in-content/content.js
@@ +152,5 @@
>     * configured.
>     */  
>    configureFonts: function ()
>    {
> +    gSubDialog.open("chrome://browser/content/preferences/fonts.xul");

This isn't wide enough on OS X with the old width which is why I was saying to change the dialog to an empty style attribute. I believe it's because the controls are all larger now. Bug 285574 which added the width doesn't seem to be a problem anymore for me.

::: browser/themes/shared/incontentprefs/preferences.css
@@ +904,5 @@
>   * End sub-dialog
>   */
> +
> +dialog groupbox,
> +prefwindow groupbox {

May as well include "window" and use -moz-any for consistency

Comment 7

[Richard Marti (:Paenglab)]

Attachment: contentDialogsIncontent.patch

Mano, please read the comment below from Matthew about the Fonts dialog.

I tested it on Linux and Windows without the width definition and it is still fully visible on in-content dialog and also on normal dialog.  Do you see a case where this doesn't work without the width definition in dialog?

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #6)
> Comment on attachment 8452600 [details] [diff] [review]
> contentDialogsIncontent.patch
> 
> Review of attachment 8452600 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for fonts.xul. Please test that change on Windows too then ask
> Mano@mozilla.… for review on that version since he worked on adding the
> width entities. Perhaps they are still needed for some locales?
> 
> ::: browser/components/preferences/in-content/content.js
> @@ +152,5 @@
> >     * configured.
> >     */  
> >    configureFonts: function ()
> >    {
> > +    gSubDialog.open("chrome://browser/content/preferences/fonts.xul");
> 
> This isn't wide enough on OS X with the old width which is why I was saying
> to change the dialog to an empty style attribute. I believe it's because the
> controls are all larger now. Bug 285574 which added the width doesn't seem
> to be a problem anymore for me.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8453233 [details] [diff] [review]
contentDialogsIncontent.patch

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

r=me with the fix but don't land without Mano's review.

::: browser/components/preferences/fonts.xul
@@ +15,5 @@
>  <prefwindow id="FontsDialog" type="child"
>              xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>              title="&fontsDialog.title;"
>              dlgbuttons="accept,cancel,help"
> +            ondialoghelp="openPrefsHelp()">

As I said before, you need to leave an empty style="" otherwise an exception will be raised. Bug 1036018 will allow use to remove the empty attribute.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: contentDialogsIncontent.patch

Added the empty style.

Comment 10

[Asaf Romano (gone)]

What was the previously set margin for menulist? Did you mean to change it?

Comment 11

[Richard Marti (:Paenglab)]

The margin change slipped in from bug 1038288. I'll remove it on next patch after review, if this is okay.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 8453585 [details] [diff] [review]
contentDialogsIncontent.patch

Without any of the margin changes.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: patch for check-in

Removed the margin change on buttons but still change the groupbox in dialogs margins to align them correctly.

Comment 14

[Richard Marti (:Paenglab)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=ad0bfc575810

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df8afcddd88a

Comment 17

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 34.0a1 (buildID:	20140728030201) and the dialogs from Content pane are in-content.