All dialogs that currently appear as windows should be in-content (Google Chrome handles this very good). As dialogs that I can think of right now there are : -Exceptions, Fonts and languages in "content" preferences -"Cookies" and "clear recent history" in "privacy" -"Passwords" and "Exceptions" in "Security" -"Pair a device" and "Set up sync" in "Sync" -Various dialogs in "Advanced" -"Page info", "Page source", "Error console" and other devtools (in-content tab or sidebar) -"Print" Final result should be having no more separated windows in the browser.

Jboriss made a mockup that summarize the current dialogs linked to the preferences : http://people.mozilla.com/~jboriss/temp/preferences_expanded.jpg As said before I would add "clear recent history" concerning preferences and the various other dialogs concerning the global UI.

This patch changes only the Colors dialog in Content pane to to a layered vbox on top of the pane. This is only a proposal to show how it could be done.

Hey Richard, do you think you could pick this back up?

Still a WIP for only the Colors dialog on Content pane. I moved the dialog to a separate dialogs.xul file and added a function to open the dialogs. Jared, what do you think, is this the way we could go?

Oops, this patch needs bug 993369 applied first.

Comment on attachment 8425707 [details] [diff] [review] WIP for only Content/Colors Review of attachment 8425707 [details] [diff] [review]: ----------------------------------------------------------------- Michael, what do you think about this? (See attached screenshot) ::: browser/components/preferences/in-content/dialogs.xul @@ +1,5 @@ > +#if 0 > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this file, > + - You can obtain one at http://mozilla.org/MPL/2.0/. --> > +#endif Use the second style of headers here, http://www.mozilla.org/MPL/headers/. ::: browser/components/preferences/in-content/preferences.js @@ +72,5 @@ > + let dialog = document.getElementById(subDialog); > + if (dialog.hidden) { > + dialog.setAttribute("hidden", "false"); > + } else { > + dialog.setAttribute("hidden", "true"); Change the name of this function to 'toggleDialog' and then the body of the function can be: function toggleDialog(subDialog) { let dialog = document.getElementById(subDialog); dialog.hidden = !dialog.hidden; }

I've added the General/Use Bookmark dialog but it doesn't work, where is no tree shown. The old dialog uses a onload command. I execute this command now after unhiding the dialog and this seems to work. But the dialog uses also a rv argument I don't know how to give this to the tree. Jared please could you help me to initialize this correctly? This should then also help me to open the permissions dialogs to solve bug 993383 through the in-content dialogs.

Comment on attachment 8426498 [details] [diff] [review] WIP with bookmarks dialog added Review of attachment 8426498 [details] [diff] [review]: ----------------------------------------------------------------- I don't see any bookmarks related code in this patch. Can you reupload with the right patch?

However I did take a look at selectBookmark.js anyways. You don't need to worry about giving the rv object to the tree, as the current code just uses that for the return value. You could change the init method to take a callback which will get the urls and names if accept is called. For example: SelectBookmarkDialog.init(function acceptCB(urls) { // 'names' appears to be unused, so left out of the arguments if (!urls) return; var homePage = document.getElementById("browser.startup.homepage"); // XXX still using dangerous "|" joiner! homePage.value = rv.urls.join("|"); }); And then in the accept method, instead of setting window.arguments[0].urls = urls, you would just call the stored callback method with `urls` as the argument.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9) > Comment on attachment 8426498 [details] [diff] [review] > WIP with bookmarks dialog added > > Review of attachment 8426498 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't see any bookmarks related code in this patch. Can you reupload with > the right patch? Sorry somehow the patch wasn't updated. Now it's the correct. (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10) > However I did take a look at selectBookmark.js anyways. You don't need to > worry about giving the rv object to the tree, as the current code just uses > that for the return value. > > You could change the init method to take a callback which will get the urls > and names if accept is called. For example: > SelectBookmarkDialog.init(function acceptCB(urls) { > // 'names' appears to be unused, so left out of the arguments > if (!urls) > return; > var homePage = document.getElementById("browser.startup.homepage"); > // XXX still using dangerous "|" joiner! > homePage.value = rv.urls.join("|"); > }); > > And then in the accept method, instead of setting window.arguments[0].urls = > urls, you would just call the stored callback method with `urls` as the > argument. I added your proposal but the tree isn't filled with the bookmarks. Please can you look why? The accept function doesn't work with window.arguments[0]... and I don't know if it would work as I don't see a bookmark to select. I added all functions from selectBookmark.js to main.js. I'm really a JS noob and appreciate every help.

I made some progress. The bookmarks list is now filled. But the callback method is still not working. Please could you look at in-content/selectBookmark.js line 83? Except this problem I would say the patch is complete for the General and Content panes. The further panes will come in additional patches.

New feedback request. Michael, what do you think about this?

This looks very nice. The dialog miss however the "cancel" and "help" buttons. A close button at the top of the dialog to dismiss it would be even nicer.

Cancel isn't needed as immediate apply is enabled. But I forgot to add the Help button on Colors dialog only.

Michael, what do you think, is a close button on top right needed?

Just a detail but could the help button be next to the ok button as it is now ?

Comment on attachment 8428404 [details] screenshot.png Yes, I believe it is, as it will function as a "Cancel" or "nevermind" for the user.

Comment on attachment 8428404 [details] screenshot.png Let's add the close button. We can use the close tab glyph as our asset.

(In reply to mmaslaney from comment #18) > Comment on attachment 8428404 [details] > screenshot.png > > Yes, I believe it is, as it will function as a "Cancel" or "nevermind" for > the user. The close button as a other possibility to close the dialog is okay. But with instant apply there is no cancel possible after a change. The change is already applied.

(In reply to mmaslaney from comment #19) > Comment on attachment 8428404 [details] > screenshot.png > > Let's add the close button. We can use the close tab glyph as our asset. Except the close button, is the dialog UI as you thought?

(In reply to Richard Marti (:Paenglab) from comment #20) > (In reply to mmaslaney from comment #18) > > Comment on attachment 8428404 [details] > > screenshot.png > > > > Yes, I believe it is, as it will function as a "Cancel" or "nevermind" for > > the user. > > The close button as a other possibility to close the dialog is okay. But > with instant apply there is no cancel possible after a change. The change is > already applied. Right, the close button would really just function as a "get me back to my preferences" action, which is fine. I don't think we need a cancel button, because there are very few changes to make in this dialog box.

(In reply to Richard Marti (:Paenglab) from comment #21) > (In reply to mmaslaney from comment #19) > > Comment on attachment 8428404 [details] > > screenshot.png > > > > Let's add the close button. We can use the close tab glyph as our asset. > > Except the close button, is the dialog UI as you thought? The UI is fine for now, we are making some minor updates at the moment to our inContent UI, which I will post later this this.

I have now all dialogs opening in-content. I'll add them as a patch per pane, but they need to apply in sequence. I only set a f? on the first patch. Please check also these patches. I'll write the special problems of every patch in their comment. This is still the same issue with not inserting back the chosen bookmark.

No problems here except I had to copy translation.js to in-content and remove only the line: const {classes: Cc, interfaces: Ci, utils: Cu} = Components; With this line the console show a redeclaration error and failed to work.

Here I think it's a similar problem as on the bookmarks. The Application details dialog is showing the apps but when I reopen the dialog the apps are doubled. The "Use other" shows all usable apps but I can't choose a app nor by "Okay" nor by double click.

Here I would say all is working.

I had to copy changemp.js, passwordManager.js and removemp.js to work correctly again because of redeclaration errors. And also because I had to chanfe some calls. Is where a possibility to work around this? I had also to copy some labels to preferences.properties. I tried to import them during build but this didn't work. Do you know how this would work? I also found no possibility to make the Master password check dialog in-content. How could I do this?

The only dialog which isn't working is the "Update history". How could I solve this?

The overlays for the certificates made the radio button looking native. Needed to add !important to the radio -moz-binding.

Started a try build to check if I broke some tests: https://tbpl.mozilla.org/?tree=Try&rev=14d4f1670d48 And yes, browser_connection.js, /browser_connection_bug388287.js and browser_proxy_backup.js are failing. I appreciate if someone could help in fixing them as I never wrote a test.

(In reply to Richard Marti (:Paenglab) from comment #31) > Started a try build to check if I broke some tests: > https://tbpl.mozilla.org/?tree=Try&rev=14d4f1670d48 I tried that build: * is it intended that the dialogs are non-resizable anymore? most of all are now, none are with the patch. * under advanced/certificates/view certificates/authorities tab the "Delete or Distrust" option is broken

(In reply to XtC4UaLL [:xtc4uall] from comment #32) > (In reply to Richard Marti (:Paenglab) from comment #31) > I tried that build: > > * is it intended that the dialogs are non-resizable anymore? most of all are > now, none are with the patch. Because the dialogs are in a layer above the content and no more windows I see no possibility to make them resizable. > * under advanced/certificates/view certificates/authorities tab the "Delete > or Distrust" option is broken This is because of the redeclaration errors I had to remove this lines from certManager.js: const nsIDialogParamBlock = Components.interfaces.nsIDialogParamBlock; const nsDialogParamBlock = "@mozilla.org/embedcomp/dialogparam;1"; If someone knows a solution that would be great.

I've also noted an issue in the privacy pane at least on Windows. The "remove individual cookies" doesn't appear when you click on the link. Apart from that the new UI is really nice.

(In reply to Richard Marti (:Paenglab) from comment #33) > (In reply to XtC4UaLL [:xtc4uall] from comment #32) > > (In reply to Richard Marti (:Paenglab) from comment #31) > > I tried that build: > > > * under advanced/certificates/view certificates/authorities tab the "Delete > > or Distrust" option is broken > > This is because of the redeclaration errors I had to remove this lines from > certManager.js: > const nsIDialogParamBlock = Components.interfaces.nsIDialogParamBlock; > const nsDialogParamBlock = "@mozilla.org/embedcomp/dialogparam;1"; > > If someone knows a solution that would be great. I found a solution and this should work now.

(In reply to Guillaume C. [:ge3k0s] from comment #34) > I've also noted an issue in the privacy pane at least on Windows. The > "remove individual cookies" doesn't appear when you click on the link. Fixed, comes now also in-content.

With this patch applied the only dialog that isn't in-content is the clear history dialog. It would be nice to have it in-content too, but there is the issue concerning the current behavior (being able to open it "on top" of every page). Maybe opening the privacy view with the dialog selected (in-content) could be a solution (this behavior is currently implemented in Google Chrome and Opera).

Comment on attachment 8432193 [details] [diff] [review] inContentDialogsPart1.patch - General pane Review of attachment 8432193 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/main.js @@ +109,5 @@ > * UI and the bookmark's address is stored to the home page preference. > */ > setHomePageToBookmark: function () > { > var rv = { urls: null, names: null }; this line should be deleted. @@ +120,2 @@ > // XXX still using dangerous "|" joiner! > homePage.value = rv.urls.join("|"); This needs to be just `urls.join("|");`. The `rv` variable is obsolete and shouldn't be declared or used anymore. @@ +138,5 @@ > + useCurrent.label = useCurrent.getAttribute("label1"); > + > + // In this case, the button's disabled state is set by preferences.xml. > + if (document.getElementById > + ("pref.browser.homepage.disable_button.current_page").locked) Please don't wrap code like this, keep the parens next to the function name. ::: browser/components/preferences/in-content/selectBookmark.js @@ +4,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/** > + * SelectBookmarkDialog controls the user interface for the "Use Bookmark for > + * Home Page" dialog. This file should be created through `hg copy` so that the history of each line is still kept. Otherwise the source control will look like you authored this whole file, which will look cool for you but will hurt when we need to look back at this in the future and can't figure out why a change was made :)

We should make sure that these dialogs are resizable. See bug 1019344 for an example.

I've fixed your comments. But the selected bookmarks are still not filling the textfield in General pane. Please can you look why and how this could be solved?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40) > We should make sure that these dialogs are resizable. See bug 1019344 for an > example. Yeah, but I don't know how this could be done with my approach with a layer above the panes. Every help here would be welcome. Or is a other solution possible?

Comment on attachment 8433517 [details] [diff] [review] inContentDialogsPart1.patch - General pane Review of attachment 8433517 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/main.js @@ +111,5 @@ > setHomePageToBookmark: function () > { > + toggleDialog('configureHomePageDialog'); > + SelectBookmarkDialog.init(function acceptCB(urls) { > + // 'names' appears to be unused, so left out of the arguments The acceptCB will get `rv` as the argument, so you will want to do: let urls = rv.urls; at the beginning of this function. ::: browser/components/preferences/in-content/selectBookmark.js @@ +14,5 @@ > * updating the .names property with an array of names for those URLs before it > * closes. > */ > var SelectBookmarkDialog = { > init: function SBD_init() { In main.js you're passing the callback in to the init function, but you're ignoring the argument here. If aAcceptCallback is not passed in, then you should throw an error. You should store the callback as a property of the this object. init: function SBD_init(aAcceptCallback) { if (!aAcceptCallback) throw new Error("aAcceptDialog is required"); this._acceptCallback = aAcceptCallback; ... } @@ +79,5 @@ > names.push(selectedNode.title); > } > + rv.urls = urls; > + rv.names = names; > + return rv; Then this needs to use the accept callback: this._acceptCallback(rv);

Great, the bookmarks are working now. Thanks a lot for your patience! I'm asking for review to see if this is the way this bug should go. Again, the dialogs aren't resizable and I don't know how to solve this with my approach.

Part 1 is the patch which prepares all and has only one dialog. I'll add part 2 to your review to have more dialogs to check. In part 2 I see also nothing which isn't working.

Comment on attachment 8433618 [details] [diff] [review] inContentDialogsPart1.patch - General pane Review of attachment 8433618 [details] [diff] [review]: ----------------------------------------------------------------- I don't know if we need to make the dialogs resizable if they consume nearly the full width of the browser window. We can get UX feedback on this. ::: browser/components/preferences/in-content/preferences.xul @@ +171,5 @@ > #include security.xul > #ifdef MOZ_SERVICES_SYNC > #include sync.xul > #endif > +#include dialogs.xul The dialogs don't cover the full page, meaning that people can still click on a category here to exit. Please move this so that it will block the full page. ::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd @@ +25,5 @@ > <!ENTITY buttonBack.tooltip "Go back one page"> > > <!ENTITY helpButton.label "Help"> > + > +<!ENTITY button.okay.label "Ok"> This should be "OK" ::: browser/themes/shared/incontentprefs/preferences.css @@ +857,5 @@ > + -moz-box-pack: center; > + top: 0; > + right: 0; > + bottom: 0; > + left: 0; Do these top/right/bottom/left do anything here? They will only work if the element is not position:static;

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #46) > Comment on attachment 8433618 [details] [diff] [review] > inContentDialogsPart1.patch - General pane > > Review of attachment 8433618 [details] [diff] [review]: > ----------------------------------------------------------------- > The dialogs don't cover the full page, meaning that people can still click > on a category here to exit. Please move this so that it will block the full > page. Now the dialogs are covering the whole page. > ::: browser/themes/shared/incontentprefs/preferences.css > @@ +857,5 @@ > > + -moz-box-pack: center; > > + top: 0; > > + right: 0; > > + bottom: 0; > > + left: 0; > > Do these top/right/bottom/left do anything here? They will only work if the > element is not position:static; This was to center the dialogs together with the margin: auto, but with the -moz-box-align/pack: center; they are no more needed.

Comment on attachment 8434266 [details] [diff] [review] inContentDialogsPart1.patch - General pane Review of attachment 8434266 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/dialogs.xul @@ +7,5 @@ > +<script src="chrome://browser/content/preferences/in-content/selectBookmark.js"/> > + > +<vbox id="configureHomePageDialog" > + class="optionsToggle" > + data-category="dialogs" Please don't copy (aka. fork) dialogs for this conversion as I don't think it's necessary and it's a maintenance issue. Please try implementing the new dialog UI by using an <iframe>/xul:browser to display the existing XUL files. This allows the dialogs to also be self-contained which avoids the namespacing issues you're running into and makes them easier to test independently. With this approach I believe you could continue using window.arguments to pass info to the popups so that means less changes to each existing JS file.

I just double-checked and the approach I suggested looks like it will work nicely. The 2nd argument to openDialog is the name of the window/frame to target and simply changing that to be the name of a <xul:browser> in the page nicely loads the XUL document there and upon clicking OK, the return value was successfully set. The only thing that didn't work for free was hiding the browser but that was to be expected and shouldn't be hard to fix. There is likely an event that can be listened for to know when the dialog is ready to be torn down and then the browser can be hidden and unloaded (e.g. set to about:blank).

I'd be willing to make one of the dialogs working for you and setup the framework and then you can do the same for the others if you'd prefer.

This would be great, Matthew. I have now no idea how to do such and this would help a lot.

Comment on attachment 8434266 [details] [diff] [review] inContentDialogsPart1.patch - General pane (In reply to Richard Marti (:Paenglab) from comment #51) > This would be great, Matthew. I have now no idea how to do such and this > would help a lot. I'm doing that work in bug 1021618 since this is a meta bug.

This patch is to apply on top of bug 1021618. It makes all dialogs in-content except the "Show Update History" dialog. This one is not a normal dialog and I don't know how I could move it in-content. On Sync I moved two prompts to alert() and confirm(). Is this okay like this? I haven't touched the old sync prompts, as they will be removed in I hope not so long time. I've added to some dialogs (ColorsDialog, FontsDialog and SanitizeDialog) a height in their XUL. This affects also the old dialogs, but they aren't looking to bad and it's also planned to remove them in future. Matthew, please can you also check if in security.js the 4th argument is correctly implemented for the master password dialogs? I hope I have found all dialogs. There are still some prompts which aren't in-content like the result prompts of master password dialogs, but this would need changes in other files which also would affect the old dialogs and other applications. Is this the right direction or should I change something?

Comment on attachment 8442940 [details] [diff] [review] inContentDialogs.patch Review of attachment 8442940 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. I was waiting for bug 1021618 to be finalized first. I'll try get to the rest tomorrow. ::: browser/components/preferences/colors.xul @@ +17,5 @@ > title="&colorsDialog.title;" > dlgbuttons="accept,cancel,help" > ondialoghelp="openPrefsHelp()" > #ifdef XP_MACOSX > + style="width: &window.macWidth; !important; height: 300px;"> Is this to increase or decrease the height compared to without this change? If it's to decrease it, then we should lower the default but make sure there that doesn't make some others with a tree too short (which is why I used a bigger number for now). We can maybe set a min-height or something on those trees? As Dão said before, the heights should be em, not px.

I've set the default height to 20em and changed the px values to em. I've also set a min-height of 15em to the trees except one which would break the appearance.

Updated to tip.

Missed somehow the translation dialog in last patch. This one is complete.

Comment on attachment 8447714 [details] [diff] [review] inContentDialogs.patch Review of attachment 8447714 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/incontentprefs/preferences.css @@ +885,5 @@ > > #dialogFrame { > /* Default dialog dimensions */ > + height: 20em; > + width: 35em; While you are changing widths, could you switch to using "ch" using for it, please? Ideally, where you are text-dependent, all heights should be "em" uints as that's dependent on the font/line height, while widths should be in "ch" units which depend on character widths (therefore wider fonts will still fit).

On proposal of comment 58 I changed the default dialog width to ch instead of em. I also changed because of bug 1021618 comment 31 the Languages dialog width from 30em to 55ch to fix the width for the increased font size to the normal dialog font size. I also changed the entity name to trigger the change also to the locales.

I made a try run to look if tests fails and bc1 fails. Please could someone with knowledge look at the tests what needs fixes? https://tbpl.mozilla.org/?tree=Try&rev=0dcabc494ebf

The first two test failures look to be bug 979207. The rest of the test failures look to be the fault of an about:preferences tab that was opened but never closed. I retriggered the bc1 suite to run again to see if all of the test failures are consistent.

This is a meta bug (which means it's shouldn't have patches) and the patch here is trying to fix too many things at once. I'm splitting it into separate bugs to make changes easier to track, review and land.

Comment on attachment 8448050 [details] [diff] [review] inContentDialogs.patch Review of attachment 8448050 [details] [diff] [review]: ----------------------------------------------------------------- Please split this patch into separate bugs by section with non-obvious/trivial changes in their own bug. I have included some overall review comments which apply after splitting. ::: browser/components/preferences/in-content/advanced.js @@ +274,5 @@ > */ > showConnections: function () > { > + gSubDialog.open("chrome://browser/content/preferences/connection.xul", > + "modal=yes", null); Omit the last argument when it's null since it's the default value. ::: browser/components/preferences/in-content/content.js @@ +152,5 @@ > * configured. > */ > configureFonts: function () > { > + gSubDialog.open("chrome://browser/content/preferences/fonts.xul", null); Omit null @@ +181,5 @@ > */ > showTranslationExceptions: function () > { > + gSubDialog.open("chrome://browser/content/preferences/translation.xul", > + null); ditto ::: browser/components/preferences/in-content/sync.js @@ +287,5 @@ > let heading = sb.formatStringFromName("firefoxAccountsVerificationSentHeading", > [data.email], 1); > let description = sb.GetStringFromName("firefoxAccountVerificationSentDescription"); > > + alert(heading + "\n\n" + description); Why is this change necessary? ::: browser/themes/shared/incontentprefs/preferences.css @@ +897,5 @@ > + min-height: 15em; > +} > + > +#certMgrTabbox { > + -moz-margin-start: 0; 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. @@ +905,5 @@ > * End sub-dialog > */ > + > +#color-separator { > + width: 1.5em; Ditto

Shouldn't a bug still be filed about the various advanced pane dialogs ?

I'll do it when I'm ready. But if you like you can do it too.

Assumptions on keeping this as a blocker to in-content prefs: 1) Need to be able to close all popups (old popups are non-obvious on some platforms) 2) Strongly desire consistency of these (don't want to port some but not others) 3) Mainly want obviously accessible popups to become in-content, but some of the obscure ones a few levels deep don't matter.

I think this can be marked as fixed ! Thanks to all that worked on it.

Good idea. I agree, thanks to all who helped!