925746 - Option to Open the Preferences in a Tab

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, enhancement)

Description

[Richard Marti (:Paenglab)]

Thunderbird should open the preferences in a tab instead a own window like the Firefox in content preferences.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: WIP

This patch uses only a slightly changed preferences.xul file to open in a tab. This let's open also third party preferences like Lightning in this tab without any change.

Mike, aceman: I don't know how to deliver the aPaneID and aTabID to the tab to be able to jump directly to the desired pane/tab. Please could you look what is needed to do this? The code which delivered it to the pref window I removed was:

  let prefWindow = win.document.getElementById("MailPreferences");
  win.selectPaneAndTab(prefWindow, aPaneID, aTabID);

Comment 2

[:aceman]

Comment on attachment 815856 [details] [diff] [review]
WIP

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

Another uglyfication that we have to copy from Firefox or Chrome? :)

There are still some style problems you can fix in the css:
If the prefs are in tabs, the background is grey. If the pref pane does not have any tabs, the background is all white and it looks ugly.
The font is too big for my taste.

I'm not yet sure how to send the arguments for the preferences.js to pick up. I am new to this tabs stuff.

::: mail/base/content/mailCore.js
@@ +393,5 @@
> +  if (!tabmail) {
> +    // Try opening new tabs in an existing 3pane window
> +    let mail3PaneWindow = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> +                                    .getService(Components.interfaces.nsIWindowMediator)
> +                                    .getMostRecentWindow("mail:3pane");

This should probably be Services.wm.getMostRecentWindow("mail:3pane")

Comment 3

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #2)
> Comment on attachment 815856 [details] [diff] [review]
> WIP
> 
> Review of attachment 815856 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are still some style problems you can fix in the css:
> If the prefs are in tabs, the background is grey. If the pref pane does not
> have any tabs, the background is all white and it looks ugly.
> The font is too big for my taste.

Yes, I know, and it's still a WIP ;)

> I'm not yet sure how to send the arguments for the preferences.js to pick
> up. I am new to this tabs stuff.
> 
> ::: mail/base/content/mailCore.js
> @@ +393,5 @@
> > +  if (!tabmail) {
> > +    // Try opening new tabs in an existing 3pane window
> > +    let mail3PaneWindow = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> > +                                    .getService(Components.interfaces.nsIWindowMediator)
> > +                                    .getMostRecentWindow("mail:3pane");
> 
> This should probably be Services.wm.getMostRecentWindow("mail:3pane")

Thank you for this tip. And it's working.

Comment 4

[Magnus Melin [:mkmelin]]

It's not default in ff, but behind the browser.preferences.inContent pref. We should probably have mail.preferences.inContent

Comment 5

[Richard Marti (:Paenglab)]

Attachment: WIP v2

This patch implements the pref mail.preferences.inContent to choose if the preferences should open in a tab or a dialog.
The in tab appearance should be now like in FX.

The only this which didn't work is the aPaneID and aTabID thing.

Mike I'd appreciate if you could help me here, I'm totally lost here. And naturally also if you have a better tab opening code ;).

Comment 6

[Richard Marti (:Paenglab)]

Attachment: WIP v3

I've got the direct opening of pane/tab working through passing them via the URL. But now when a pref tab is open and call for a other pane/tab is done, a new pref tab opens. Only calls to the same pane/tab is using the already open tab. It looks like it's looking for a open tab with the whole URL with arguments.

Mike, do you know a solution to only look for the first part of the URL or a other way to check for a already open pref tab? Or do you have a better solution for the direct opening?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 829734 [details] [diff] [review]
WIP v3

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

I think this is a good way forward - just have a few suggestions - see below!

::: mail/app/profile/all-thunderbird.js
@@ +249,5 @@
>  pref("browser.preferences.animateFadeIn", false);
>  #endif
>  
> +// load the Preferences in a tab
> +pref("mail.preferences.inContent", true);

I'd be OK landing this work sooner if this was set to false, and we could iterate on it / polish it while it's disabled behind this pref.

::: mail/base/content/mailCore.js
@@ +387,5 @@
>   * @param aOtherArgs  other prefpane specific arguments
>   */
>  function openOptionsDialog(aPaneID, aTabID, aOtherArgs)
>  {
> +  let gLoadInContent = Services.prefs.getBoolPref("mail.preferences.inContent");

The "g" prefix is usually reserved for globals, which gLoadInContent is not. So this should be loadInContent.

@@ +391,5 @@
> +  let gLoadInContent = Services.prefs.getBoolPref("mail.preferences.inContent");
> +  // Load the prefs in a tab?
> +  if (gLoadInContent) {
> +    // Yes, load the prefs in a tab
> +    let pref = "chrome://messenger/content/preferences/preferencesInContent.xul";

Should be about:preferences. You can map an about: URI to this URI via mail/components/aboutRedirector.js.

@@ +413,5 @@
> +  
> +    if (tabmail) {
> +      tabmail.openTab("contentTab", {contentPage: pref});
> +    } else {
> +      // No 3pane window and a dialog must be created

This repeats lines 437-445...

And I'm not sure it's the right next step. I think if we weren't able to find a 3pane window, we should open a 3pane window, and then open the about:preferences tab in it.

::: mail/base/content/specialTabs.js
@@ +246,5 @@
>  const kTelemetryPromptRev   = 2;
>  
>  var contentTabBaseType = {
> +  inContentWhitelist: ['about:addons',
> +                       'chrome://messenger/content/preferences/preferencesInContent.xul'],

To mimic Firefox, this should be about:preferences, and we should register an about: handler for this URI.

::: mail/themes/linux/mail/preferences/preferencesInContent.css
@@ +80,5 @@
> +  background-image: linear-gradient(-moz-dialog, -moz-dialog);
> +}
> +
> +caption {
> +  font-size: 1.667rem;

Wow, today I learned about rem. :)

Comment 8

[Richard Marti (:Paenglab)]

Attachment: WIP v4

This patch uses now "about:preferences" but with this instead of "chrome://messenger/content/preferences/preferencesInContent.xul" the arguments aren't passed to the tab. in preferences.js I added a line to show in console what arguments are passed (Application.console.log("In=" + window.location.search);).

Mike, please can you tell me what is missing or wrong in my patch?

Comment 9

[Richard Marti (:Paenglab)]

Attachment: WIP using the prefs to passing aPane and aTab

This patch uses the prefs (setCharPref() and getCharPref() and naturally clear it afterwards) to commit the preference's pane and tab.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Ah, found it - see bug 675555.

I think we'll need to manually extract the values from window.location.href.

Comment 11

[Richard Marti (:Paenglab)]

Comment on attachment 8340684 [details] [diff] [review]
WIP v4

Clearing the feedback request. I'm on implementing a preferencesTabType.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: WIP v6

This patch uses now a preferencesTab. For testing I defined aPaneID = "paneCompose"; and set a condole output in utilityOverlay.js, preferencesTab.js and preferences.js to see where the PaneID is defined. Up to preferencesTab.js it works but preferences.js doesn't get it. I think it has something to do with aArgs.postData in preferencesTab.js and the correct formatting in utilityOverlay.js (I haven't added this here because the copy of the webSearchTab didn't work and gave errors.

Mike, please can you look into this? When you find a solution, can you also say how I then can get this data in preferences.js?

Comment 13

[Richard Marti (:Paenglab)]

Attachment: WIP v6

Patch updated to Nightly.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

First of all, a huge apology for taking practically forever to get to this. :/

I *think* the solution is, instead of passing the paneID directly to openTab, we pass in an onLoad function that fires as soon as preferences.xul is done loading, and the onLoad handler switches to the proper pane.

Some of the tab types in specialTabs.js make an onLoad parameter available... perhaps we can do the same?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: fix.diff

Just a little thing I fiddled with to get a pref pane opened via argument to openTab.

Comment 16

[Richard Marti (:Paenglab)]

Attachment: WIP v7

This is the latest patch with all in-content prefs styles in it. The next patches are based on global/shared/in-content styles from bug 989469.

This patch has still the two issues of not showing the correct category/tab/sub-dialog and the instantApply can't be overridden. It needs the prefs change and this isn't optimal when the user switches to the normal prefs in separate window on Windows.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: WIP v7 in-content dialogs

This patch adds the in-content dialogs like FX has them now.

Comment 18

[Richard Marti (:Paenglab)]

Attachment: WIP v8

Latest version of my patch.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: Open the Preferences in a Tab

Comment 20

[Richard Marti (:Paenglab)]

Attachment: PrefsInTab.patch

Patch ready for review where everything should work. I've set mail.preferences.inContent to true in this patch for easier review. I change this for check-in because without Account Manager in this tab (bug will be filed shortly) it makes no sense to enable it.

Comment 21

[Richard Marti (:Paenglab)]

Attachment: inContentDialog.patch

Patch to enable the in-content dialogs like in Firefox with the latest styling.

Comment 22

[Richard Marti (:Paenglab)]

Attachment: PrefsInTab.patch

The tab title on OS X should be now always the same.

Comment 23

[Richard Marti (:Paenglab)]

Attachment: PrefsInTab.patch

This patch should really stop now the title changes on OS X by overriding the _selectPane method.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: Open the Preferences in a Tab

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8507419 [details] [diff] [review]
Open the Preferences in a Tab

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

This is very, very cool stuff Paenglab. I'm really excited by it - probably the biggest change to the TB UI since JosiahOne's Compose work. :)

I don't have any huge architectural problems or suggestions, but some guidance on how best to communicate with preferences.js, and some general nits / fixes, and the usual suggestions.

::: mail/app/profile/all-thunderbird.js
@@ +249,5 @@
>  pref("browser.preferences.animateFadeIn", false);
>  #endif
>  
> +// load the Preferences in a tab
> +pref("mail.preferences.inContent", true);

We should probably disable this by default until the account manager gets merged in.

::: mail/base/content/mailCore.js
@@ +406,2 @@
>      }
> +    let tabmail = document.getElementById("tabmail");

What's tabmail being used for here?

@@ +406,3 @@
>      }
> +    let tabmail = document.getElementById("tabmail");
> +    openPreferencesTab(url, aPaneID, aTabID, subdialogID);

Instead of stripping out the rest of the aOtherArgs, can we just pass aOtherArgs to openPreferencesTab as the fourth parameter?

Also, I wonder - if you take my suggestion, and openOptionsDialog takes the same arguments as openOptionsDialog, then I wonder if we need both functions. Likely the code from openPreferencesTab can just be moved in here?

::: mail/base/content/utilityOverlay.js
@@ +285,5 @@
> +    contentPage: url,
> +    paneID: paneID,
> +    tabID: tabID,
> +    subdialogID: subdialogID,
> +    onLoad: function(aEvent, aBrowser) {

Maybe I've been spending too much time in Electrolysis land, but reaching down into the contentWindow of a browser is something I don't think we have to do.

I think we can get away with doing something like:

openTab("preferencesTab", params);

and then in preferencesTab.js's openTab method, have it use the browser messageManager to send the parameters down into the about:preferences browser.

In preferencesTab.js, that'd be like this:

let mm = aTab.browser.messageManager;
mm.sendAsyncMessage("Mail:Preferences:Open", params, true);

// "Mail:Preferences" is the name of the message we're
// sending down to the about:preferences browser, which
// at this point, is likely still in the process of
// loading.

Then, in preferences.js (which runs in the about:preferences browser), do this:

addMessageListener("Mail:Preferences:Open", (message) => {
  let params = message.data;
  if (params) {
    addEventListener("paneload", function onPaneLoad() {
      removeEventListener("paneload", onPaneLoad);
      let prefWindow = document.getElementById("MailPreferences");
      selectPaneAndTab(prefWindow, params.paneID, params.tabID, params.subdialogID);
    });
  }
});

::: mail/components/aboutRedirector.js
@@ +21,5 @@
>                 flags: (Ci.nsIAboutModule.ALLOW_SCRIPT |
>                         Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT)},
>      "support": {url: "chrome://messenger/content/about-support/aboutSupport.xhtml",
>                  flags: Ci.nsIAboutModule.ALLOW_SCRIPT},
> +    "preferences": {url: "chrome://messenger/content/preferences/preferencesInContent.xul",

To follow convention, we should probably call this aboutPreferences.xul.

::: mail/components/preferences/preferences.xml
@@ +62,5 @@
> +      <method name="_selectPane">
> +        <parameter name="aPaneElement"/>
> +        <body>
> +        <![CDATA[
> +          var helpButton = document.documentElement.getButton("help");

let, instead of var please. Please use let throughout this function.

@@ +63,5 @@
> +        <parameter name="aPaneElement"/>
> +        <body>
> +        <![CDATA[
> +          var helpButton = document.documentElement.getButton("help");
> +          if (aPaneElement.helpTopic)

helpButton.hidden = !aPaneElement.helpTopic; // If there's no topic, we don't need the help button.

@@ +71,5 @@
> +
> +          // Find this pane's index in the deck and set the deck's 
> +          // selectedIndex to that value to switch to it.
> +          var prefpanes = this.preferencePanes;
> +          for (var i = 0; i < prefpanes.length; ++i) {

We never actually use the index that we're iterating over here. We're really just checking to make sure that the aPaneElement is contained within preferencePanes.

So we can do something like:

if (Array.indexOf(this.preferencePanes, aPaneElement) == -1) {
  // Probably report some error to the console with Components.utils.reportError
}

And then instead of setting the selectedIndex on the _paneDeck, use:

this._paneDeck.selectedPanel = aPaneElement;

::: mail/components/preferences/preferencesInContent.xul
@@ +16,5 @@
> +<?xml-stylesheet href="chrome://messenger/content/preferences/handlers.css"?>
> +<?xml-stylesheet href="chrome://messenger/skin/preferences/applications.css"?>
> +<?xml-stylesheet href="chrome://messenger/skin/preferences/preferences.css"?>
> +<?xml-stylesheet href="chrome://global/skin/in-content/common.css"?>
> +<?xml-stylesheet href="chrome://messenger/skin/preferences/preferencesInContent.css"?>

This should also probably be called aboutPreferences.css

::: mail/components/preferences/preferencesTab.js
@@ +41,5 @@
> +  shouldSwitchTo: function shouldSwitchTo(aArgs) {
> +    if (!gPrefTab) {
> +      return -1;
> +    }
> +    let prefWindow = gPrefTab.browser.contentDocument.getElementById("MailPreferences");

Like elsewhere, I think we should avoid reaching into the content document and content window to do things.

Instead, we can pass messages, like this:

let mm = gPrefTab.browser.messageManager;
mm.sendAsyncMessage("Mail:Preferences:Switch", aArgs, true);

Then, in preferences.js, have it listen for it:

addMessageListener("Mail:Preferences:Switch", (message) => {
  let prefWindow = document.getElementById("MailPreferences");
  let params = message.data;
  selectPaneAndTab(prefWindow, params.paneID, params.tabID, params.subdialogID);
});

@@ +70,5 @@
> +
> +    aTab.browser.setAttribute("id", "preferencesTabBrowser" + this.lastBrowserId);
> +
> +    aTab.browser.addEventListener("DOMLinkAdded", DOMLinkHandler, false);
> +    gPluginHandler.addEventListeners(aTab.browser);

It's highly unlikely that we'll ever add plugins to about:preferences - I think we can skip this bit.

@@ +85,5 @@
> +    this._setUpTitleListener(aTab);
> +    this._setUpCloseWindowListener(aTab);
> +    this._setUpBrowserListener(aTab);
> +
> +    if ("onLoad" in aArgs) {

As I mentioned elsewhere, I think this logic can be moved into preferences.js, and a message can be sent to trigger that function instead.

@@ +113,5 @@
> +           };
> +  },
> +
> +  restoreTab: function onRestoreTab(aTabmail, aPersistedState) {
> +    aTabmail.openTab("preferencesTab",

I generally prefer this style:

aTabmail.openTab("preferencesTab", {
  contentPage: aPersistedState.tabURI,
  paneID: aPersistedState.paneID,
  tabID: aPersistedState.tabID,
  subdialogID: aPersistedState.subdialogID,
});

@@ +124,5 @@
> +
> +  _setUpBrowserListener: function setUpBrowserListener(aTab) {
> +    // Browser navigation (front/back) does not cause onDOMContentLoaded,
> +    // so we have to use nsIWebProgressListener
> +    this.progressListener = {

We don't end up doing much with this progress listener, so I don't think we need to add it.

::: mail/components/preferences/preferencesTab.xul
@@ +12,5 @@
> +  <hbox id="tabmail-container">
> +    <vbox id="preferencesTab" collapsed="true">
> +      <vbox flex="1">
> +        <notificationbox flex="1">
> +          <browser id="dummycontentbrowser" type="content-targetable" flex="1"

We already have a browser with id "dummycontentbrowser" at http://hg.mozilla.org/comm-central/file/804eb95ff8f5/mail/base/content/specialTabs.xul#l26. I suggest we just name this preferencesbrowser or something like that.

@@ +16,5 @@
> +          <browser id="dummycontentbrowser" type="content-targetable" flex="1"
> +                   disablehistory="true" autocompletepopup="PopupAutoComplete"
> +                   context="mailContext"/>
> +        </notificationbox>
> +        <findbar browserid="dummycontentbrowser"/>

Same as above.

::: mail/locales/en-US/chrome/messenger/preferences/preferences.dtd
@@ +3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!ENTITY  prefWindow.titleWin     "Options">
>  <!ENTITY  prefWindow.titleGNOME   "&brandShortName; Preferences">
> +<!ENTITY  prefWindow.titleMAC     "Preferences">

Wait... what were we using before for this? Did we ever figure that out?

::: mail/themes/linux/mail/preferences/preferencesInContent.css
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +@import url("chrome://messenger/skin/shared/in-content/preferences.css");
> +
> +checkbox {

Can I assume that there's not enough common between the 3 distinct copies of preferencesInContent.css that would allow us to put the common things in a shared css file?

Comment 26

[Richard Marti (:Paenglab)]

Attachment: PrefsInTab.patch

I could almost every comment address, except the

let mm = aTab.browser.messageManager;
mm.sendAsyncMessage("Mail:Preferences:Open", params, true);

and

let mm = gPrefTab.browser.messageManager;
mm.sendAsyncMessage("Mail:Preferences:Switch", aArgs, true);

and also both addMessageListener in preferences.js. I couldn't made it to work. If you could fix this it would really helpful.

In preferences.xml I changed the index to:

// Is the aPaneElement contained within preferencePanes?
if (Array.indexOf(this.preferencePanes, aPaneElement) == -1) {
  Components.utils.reportError;
}
this._paneDeck.selectedPanel = aPaneElement;
}

As you can see, the last bracket to much and get the error below. But without the bracket the panels aren't shown. How can this be fixed?

Error: SyntaxError: unexpected garbage after function body, starting with '}'
Source File: chrome://messenger/content/preferences/preferences.xml
Line: 99

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #25)
> Comment on attachment 8507419 [details] [diff] [review]
> Open the Preferences in a Tab
> 
> Review of attachment 8507419 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: mail/locales/en-US/chrome/messenger/preferences/preferences.dtd
> @@ +3,5 @@
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >  
> >  <!ENTITY  prefWindow.titleWin     "Options">
> >  <!ENTITY  prefWindow.titleGNOME   "&brandShortName; Preferences">
> > +<!ENTITY  prefWindow.titleMAC     "Preferences">
> 
> Wait... what were we using before for this? Did we ever figure that out?

Yes, by overriding the _selectPane method...which introduced the problem I have now with the bracket.

> ::: mail/themes/linux/mail/preferences/preferencesInContent.css
> @@ +3,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +@import url("chrome://messenger/skin/shared/in-content/preferences.css");
> > +
> > +checkbox {
> 
> Can I assume that there's not enough common between the 3 distinct copies of
> preferencesInContent.css that would allow us to put the common things in a
> shared css file?

I would say 336 lines should be enough to be in a shared file.

Comment 27

[Richard Marti (:Paenglab)]

Attachment: InContentDialog.patch

Updated to the new file names (aboutPreferences.*). This is more or less a copy of the FX patches.

Comment 28

[Richard Marti (:Paenglab)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #25)
> Comment on attachment 8507419 [details] [diff] [review]
> Open the Preferences in a Tab
> ::: mail/locales/en-US/chrome/messenger/preferences/preferences.dtd
> @@ +3,5 @@
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >  
> >  <!ENTITY  prefWindow.titleWin     "Options">
> >  <!ENTITY  prefWindow.titleGNOME   "&brandShortName; Preferences">
> > +<!ENTITY  prefWindow.titleMAC     "Preferences">
> 
> Wait... what were we using before for this? Did we ever figure that out?

I tried it with a try build. Actually it doesn't work because of the failure with the bracket. With this it works:

        <![CDATA[
          let helpButton = document.documentElement.getButton("help");
          // If there's no topic, we don't need the help button.
          helpButton.hidden = !aPaneElement.helpTopic;

          // Find this pane's index in the deck and set the deck's 
          // selectedIndex to that value to switch to it.
          var prefpanes = this.preferencePanes;
          for (var i = 0; i < prefpanes.length; ++i) {
            if (prefpanes[i] == aPaneElement) {
              this._paneDeck.selectedIndex = i;
            }
          }
        ]]>

With the bracket error solved it should also work with the newer version.

Comment 29

[Richard Marti (:Paenglab)]

Attachment: PrefsInTab.patch

Unbitrotted.

Mike, please read comment 26 and comment 28.

Comment 30

[Richard Marti (:Paenglab)]

Attachment: inContentDialog.patch

Unbitrotted.

Comment 31

[Richard Marti (:Paenglab)]

Attachment: inContentDialog.patch

Updated the styling after landing of FX bug 1089812.

Comment 32

[Richard Marti (:Paenglab)]

Attachment: InContentDialog.patch

Updated to tip.

Comment 33

[Richard Marti (:Paenglab)]

Attachment: MozReview Request: bz://925746/Paenglab

Comment 34

[Richard Marti (:Paenglab)]

/r/1477 - Bug 925746 - Open the Preferences in a Tab.
/r/1479 - Bug 925746 - Show subdialogs in a layer when mail.preferences.inContent.

Pull down these commits:

hg pull review -r d2bfd176239a6ef574328423d8a2e1c10466fb89

Comment 35

[Richard Marti (:Paenglab)]

(In reply to Richard Marti (:Paenglab) from comment #34)
> /r/1477 - Bug 925746 - Open the Preferences in a Tab.
> /r/1479 - Bug 925746 - Show subdialogs in a layer when
> mail.preferences.inContent.
> 
> Pull down these commits:
> 
> hg pull review -r d2bfd176239a6ef574328423d8a2e1c10466fb89

This are updated patches with all changes from FX included.

I fixed comment 28 but don't use the if (Array.indexOf(this.preferencePanes, aPaneElement) == -1) {} (it never worked here).

Still also not using the let mm = aTab.browser.messageManager; things. I'll appreciate every help here.

mail.preferences.inContent is still on true in this patch for easier testing.

Comment 36

[Richard Marti (:Paenglab)]

/r/1477 - Bug 925746 - Open the Preferences in a Tab.
/r/1479 - Bug 925746 - Show subdialogs in a layer when mail.preferences.inContent.

Pull down these commits:

hg pull review -r 498367ae0a8663cbc497cc6c4f44009c8a2d0ab8

Comment 37

[Richard Marti (:Paenglab)]

(In reply to Richard Marti (:Paenglab) from comment #36)
> /r/1477 - Bug 925746 - Open the Preferences in a Tab.
> /r/1479 - Bug 925746 - Show subdialogs in a layer when
> mail.preferences.inContent.
> 
> Pull down these commits:
> 
> hg pull review -r 498367ae0a8663cbc497cc6c4f44009c8a2d0ab8

- Updated to the last FX in-content pref changes.

- In preferences.xml on <method name="_selectPane"> I had to add some code which is needed as minimal set, like the remember the last selected pane or the pane height calculation (without it the Account Manager would only be around 100px tall)

- On subdialogs.js I had to add the change of the pref instantApply to true and reverting it to the previously stored value after closing the dialog. Without it the changes aren't applied on Windows with default to false. Maybe it's not a clean way doing this but I saw no other solution. The only issue I'm seeing here is, the pref can't be changed in about:config dialog.

And to repeat comment 35: Still not using the let mm = aTab.browser.messageManager; things. I can't make it working with this. I'll appreciate every help here.

Comment 38

[Richard Marti (:Paenglab)]

/r/1477 - Bug 925746 - Open the Preferences in a Tab.
/r/1479 - Bug 925746 - Show subdialogs in a layer when mail.preferences.inContent.

Pull down these commits:

hg pull review -r 6fc77ccaea362495a439e238f03e58793f80c9d0

Comment 39

[Richard Marti (:Paenglab)]

Sorry, found a trailing white space.

Comment 40

[Nomis101]

I've tested your latest patch on Mac and apart from some slight glitches, it looks awesome. But, I think in your latest patch you have missed to include all png files from /mail/incontentprefs/. Is this by purpose? I had to get them from an older patch.

Comment 41

[Richard Marti (:Paenglab)]

Attachment: PrefsInTab.patch

(In reply to Nomis101 from comment #40)
> I've tested your latest patch on Mac and apart from some slight glitches, it
> looks awesome. But, I think in your latest patch you have missed to include
> all png files from /mail/incontentprefs/. Is this by purpose? I had to get
> them from an older patch.

Yeah, "Download Diff" in Reviewboard gives only a diff without the png files. Mike is this a Reviewboard bug?

hg pull review -r 6fc77ccaea362495a439e238f03e58793f80c9d0 is adding the files correctly.

I attached the PrefsInTab.patch with all files for easier testing. The inContentDialog.patch can still downloaded form Reviewboard as it contains no images.

Comment 42

[Mike Conley (:mconley) (:⚙️)]

(In reply to Richard Marti (:Paenglab) from comment #26)
> Created attachment 8509046 [details] [diff] [review]
> PrefsInTab.patch
> 
> I could almost every comment address, except the
> 
> let mm = aTab.browser.messageManager;
> mm.sendAsyncMessage("Mail:Preferences:Open", params, true);
> 
> and
> 
> let mm = gPrefTab.browser.messageManager;
> mm.sendAsyncMessage("Mail:Preferences:Switch", aArgs, true);
> 
> and also both addMessageListener in preferences.js. I couldn't made it to
> work. If you could fix this it would really helpful.

Could you go into more detail about how you couldn't make it work? Were there error messages? Was it something to do with messageManager not being defined? If so, can you try setting the "type" attribute on the browser to "content-primary" in openTab in preferencesTab.js?

> 
> In preferences.xml I changed the index to:
> 
> // Is the aPaneElement contained within preferencePanes?
> if (Array.indexOf(this.preferencePanes, aPaneElement) == -1) {
>   Components.utils.reportError;
> }
> this._paneDeck.selectedPanel = aPaneElement;
> }
> 
> As you can see, the last bracket to much and get the error below. But
> without the bracket the panels aren't shown. How can this be fixed?

The panels aren't shown... is an error reported?

Comment 43

[Richard Marti (:Paenglab)]

/r/1477 - Bug 925746 - Open the Preferences in a Tab.
/r/1479 - Bug 925746 - Show subdialogs in a layer when mail.preferences.inContent.

Pull down these commits:

hg pull review -r c9cfa31deb653751947bd3facd330fbb45e88254

Comment 44

[Richard Marti (:Paenglab)]

(In reply to Richard Marti (:Paenglab) from comment #43)
> /r/1477 - Bug 925746 - Open the Preferences in a Tab.
> /r/1479 - Bug 925746 - Show subdialogs in a layer when
> mail.preferences.inContent.
> 
> Pull down these commits:
> 
> hg pull review -r c9cfa31deb653751947bd3facd330fbb45e88254

I've got it working with let mm = aTab.browser.messageManager; but I get this error:

Error: ReferenceError: addMessageListener is not defined
Source file: chrome://messenger/content/preferences/preferences.js
Line: 41

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #42)
> (In reply to Richard Marti (:Paenglab) from comment #26)
> > 
> > In preferences.xml I changed the index to:
> > 
> > // Is the aPaneElement contained within preferencePanes?
> > if (Array.indexOf(this.preferencePanes, aPaneElement) == -1) {
> >   Components.utils.reportError;
> > }
> > this._paneDeck.selectedPanel = aPaneElement;
> > }
> > 
> > As you can see, the last bracket to much and get the error below. But
> > without the bracket the panels aren't shown. How can this be fixed?
> 
> The panels aren't shown... is an error reported?

This issue is obsolete as I needed other code to work correctly.

Comment 45

[Mike Conley (:mconley) (:⚙️)]

(In reply to Richard Marti (:Paenglab) from comment #44)
> (In reply to Richard Marti (:Paenglab) from comment #43)
> > /r/1477 - Bug 925746 - Open the Preferences in a Tab.
> > /r/1479 - Bug 925746 - Show subdialogs in a layer when
> > mail.preferences.inContent.
> > 
> > Pull down these commits:
> > 
> > hg pull review -r c9cfa31deb653751947bd3facd330fbb45e88254
> 
> I've got it working with let mm = aTab.browser.messageManager; but I get
> this error:
> 
> Error: ReferenceError: addMessageListener is not defined
> Source file: chrome://messenger/content/preferences/preferences.js
> Line: 41

Bah - it looks like addMessageListener isn't recognized for script loaded in a .xul document. :/ Alright, well, instead of using messageManager, can you perhaps use postMessage instead? I really want to avoid reaching directly into content if possible.


> 
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #42)
> > (In reply to Richard Marti (:Paenglab) from comment #26)
> > > 
> > > In preferences.xml I changed the index to:
> > > 
> > > // Is the aPaneElement contained within preferencePanes?
> > > if (Array.indexOf(this.preferencePanes, aPaneElement) == -1) {
> > >   Components.utils.reportError;
> > > }
> > > this._paneDeck.selectedPanel = aPaneElement;
> > > }
> > > 
> > > As you can see, the last bracket to much and get the error below. But
> > > without the bracket the panels aren't shown. How can this be fixed?
> > 
> > The panels aren't shown... is an error reported?
> 
> This issue is obsolete as I needed other code to work correctly.

Ok, good.

Comment 46

[Richard Marti (:Paenglab)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #45)
> 
> Bah - it looks like addMessageListener isn't recognized for script loaded in
> a .xul document. :/ Alright, well, instead of using messageManager, can you
> perhaps use postMessage instead? I really want to avoid reaching directly
> into content if possible.

In preferences.js I have now this without errors:

window.addEventListener("Mail:Preferences:Open", openTab);

function openTab(message) {
  let params = message.data;
  if (params) {
    addEventListener("paneload", function onPaneLoad() {
      removeEventListener("paneload", onPaneLoad);
      let prefWindow = document.getElementById("MailPreferences");
      selectPaneAndTab(prefWindow, params.paneID, params.tabID,
      params.subdialogID);
    });
  }
};

window.addEventListener("Mail:Preferences:Switch", switchTab);

function switchTab(message) {
  let params = message.data;
  let prefWindow = document.getElementById("MailPreferences");
  selectPaneAndTab(prefWindow, params.paneID, params.tabID,
  params.subdialogID);
};

Is this looking okay?

But on preferencesTab.js I can't make it working. I tried it with:
postMessage("Mail:Preferences:Open", aArgs);

All I get is a error:

Error: SyntaxError: An invalid or illegal string was specified
Source file: resource://app/modules/errUtils.js
Line: 35

Please can you show me how to do this correctly?

Comment 47

[Mike Conley (:mconley) (:⚙️)]

postMessage doesn't fire an event named after the message that's being sent - instead, the receiver of the message gets a "message" event, where the event.data is the message string.

See https://developer.mozilla.org/en-US/docs/Web/API/window.postMessage?redirectlocale=en-US&redirectslug=DOM%2Fwindow.postMessage, for examples.

Can you post your latest work? I can try to poke at it.

Comment 48

[Richard Marti (:Paenglab)]

Attachment: Patch for poke

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #47)
> postMessage doesn't fire an event named after the message that's being sent
> - instead, the receiver of the message gets a "message" event, where the
> event.data is the message string.
> 
> See
> https://developer.mozilla.org/en-US/docs/Web/API/window.
> postMessage?redirectlocale=en-US&redirectslug=DOM%2Fwindow.postMessage, for
> examples.

I was on this page today but what I posted above was what I could do with this examples.

> Can you post your latest work? I can try to poke at it.

Okay, here it is with my latest changes.

Comment 49

[Richard Marti (:Paenglab)]

Oops, forgot to thank you for your help and patience.

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Bah - postMessage requires us to pass a domain as the second argument, and we're discouraged from doing that for chrome pages[1]. :/

I give up on the message passing bits. Let's just reach into content like you were doing before - sorry for leading you down the garden path.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/window.postMessage?redirectlocale=en-US&redirectslug=DOM%2Fwindow.postMessage#Using_window.postMessage_in_extensions

Comment 51

[Richard Marti (:Paenglab)]

/r/1477 - Bug 925746 - Open the Preferences in a Tab.
/r/1479 - Bug 925746 - Show subdialogs in a layer when mail.preferences.inContent.

Pull down these commits:

hg pull review -r d767d35e76147e899827ed6f07788ec8ec4643df

Comment 52

[Richard Marti (:Paenglab)]

(In reply to Richard Marti (:Paenglab) from comment #51)
> /r/1477 - Bug 925746 - Open the Preferences in a Tab.
> /r/1479 - Bug 925746 - Show subdialogs in a layer when
> mail.preferences.inContent.
> 
> Pull down these commits:
> 
> hg pull review -r d767d35e76147e899827ed6f07788ec8ec4643df

Okay, this is the version which works for me on all situations.

Comment 53

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/1475/#review1253

Thanks for your huge amount of work and patience, Paenglab - I know you've worked on this for what seems like forever.

Just a few final nits, but I don't think it'll require a second review. Just address my notes, and I think you should be good to land. Then we can see if there are any bugs to shake out (and pref off if need be).

::: mail/base/content/mailCore.js
(Diff revision 5)
> +    openPreferencesTab(url, aPaneID, aTabID, aOtherArgs);

Why does about:preferences need to get passed to openPreferencesTab? Should openPreferencesTab already know which URI to use?

::: mail/components/preferences/connection.xul
(Diff revision 5)
> -            style="width: &window.width; !important;">
> +            style="">

Rather we just remove this than leave it blank.

::: mail/components/preferences/display.js
(Diff revision 5)
> +      var dialog = gSubDialog.open("chrome://messenger/content/newTagDialog.xul",

let not var

::: mail/components/preferences/display.js
(Diff revision 5)
> +        var dialog = gSubDialog.open("chrome://messenger/content/newTagDialog.xul",

let, not var

::: mail/components/preferences/preferencesTab.xul
(Diff revision 5)
> +

Nit - remove extra space

::: mail/components/preferences/preferencesTab.js
(Diff revision 5)
> +    return { tabURI: aTab.url,
> +             paneID: aTab.paneID,
> +             tabID: aTab.tabID,
> +             otherArgs: aTab.otherArgs,
> +           };

Style nit:

```return {
  tabURI: aTab.url,
  paneID: aTab.paneID,
  tabID: aTab.tabID,
  otherArgs: aTag.otherArgs,
};
```

::: mail/components/preferences/subdialogs.js
(Diff revision 5)
> +let gSubDialog = {

If we wanted to be neater about things, we could have the callers of gSubDialog not care about whether or not we were using in-content prefs or not. gSubDialog would take care of knowing whether or not to use the inline browser for the dialog, or whether to open an actual dialog.

This patch, however, has languished in review hell for too long, so I won't insist.

Comment 54

[Richard Marti (:Paenglab)]

Attachment: PrefsInTab.patch for check-in

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #53)
> https://reviewboard.mozilla.org/r/1475/#review1253
> 
> Just a few final nits, but I don't think it'll require a second review. Just
> address my notes, and I think you should be good to land. Then we can see if
> there are any bugs to shake out (and pref off if need be).

I preffed it off now until bug 1096006 lands. Without this bug it makes the in-content prefs unusable when you have to change a pref from open Account Manager.

> ::: mail/base/content/mailCore.js
> (Diff revision 5)
> > +    openPreferencesTab(url, aPaneID, aTabID, aOtherArgs);
> 
> Why does about:preferences need to get passed to openPreferencesTab? Should
> openPreferencesTab already know which URI to use?

I moved let url = "about:preferences"; to utilityOverlay.js. Like this the URL is directly defined where it needs and the callers don't need to know this. Okay?

> ::: mail/components/preferences/connection.xul
> (Diff revision 5)
> > -            style="width: &window.width; !important;">
> > +            style="">
> 
> Rather we just remove this than leave it blank.

Yes, this was a leftover from a bug from subdialogs.js (from FX) which is fixed now. Removed.

> ::: mail/components/preferences/display.js
> (Diff revision 5)
> > +      var dialog = gSubDialog.open("chrome://messenger/content/newTagDialog.xul",
> 
> let not var
> 
> ::: mail/components/preferences/display.js
> (Diff revision 5)
> > +        var dialog = gSubDialog.open("chrome://messenger/content/newTagDialog.xul",
> 
> let, not var

Both done.

> ::: mail/components/preferences/preferencesTab.xul
> (Diff revision 5)
> > +
> 
> Nit - remove extra space

Done.

> ::: mail/components/preferences/preferencesTab.js
> (Diff revision 5)
> > +    return { tabURI: aTab.url,
> > +             paneID: aTab.paneID,
> > +             tabID: aTab.tabID,
> > +             otherArgs: aTab.otherArgs,
> > +           };
> 
> Style nit:
> 
> ```return {
>   tabURI: aTab.url,
>   paneID: aTab.paneID,
>   tabID: aTab.tabID,
>   otherArgs: aTag.otherArgs,
> };
> ```

Done

> ::: mail/components/preferences/subdialogs.js
> (Diff revision 5)
> > +let gSubDialog = {
> 
> If we wanted to be neater about things, we could have the callers of
> gSubDialog not care about whether or not we were using in-content prefs or
> not. gSubDialog would take care of knowing whether or not to use the inline
> browser for the dialog, or whether to open an actual dialog.

If you're okay I'll try this in a follwup bug.
n-i for this and if it's okay with the let url = "about:preferences"; move.

Comment 55

[Richard Marti (:Paenglab)]

Attachment: inContentDialog.patch for check-in

Comment 56

[Mike Conley (:mconley) (:⚙️)]

Yes that sounds fine to me. Thanks Paenglab!

Comment 57

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/85cf757356da
https://hg.mozilla.org/comm-central/rev/831b26828a50

Comment 58

[Sören Hentzschel]

There are a few styling issues with the in-content preferences on OS X (for example missing color on the main navigation, jumping icons on panel change, styling differences compared with Firefox). Is there a bugzilla ticket to follow the remaining work?

Comment 59

[Richard Marti (:Paenglab)]

Sören, no followup bug now. Please open new bugs blocking this bug.

Comment 60

[Charles]

Is this going to be optional? Meaning, I can set a preference to continue opening the Options in a separate window?

Most everyone I know who uses TB has serious dislike for tabs in Thunderbird (I love them in Firefox).

Comment 61

[Richard Marti (:Paenglab)]

Where is no plan to remove the ability to open the prefs in a window.