Option to Open the Preferences in a Tab

RESOLVED FIXED in Thunderbird 37.0

Status

--
enhancement
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

(Depends on: 2 bugs)

unspecified
Thunderbird 37.0
Dependency tree / graph
Bug Flags:
in-moztrap ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ux-feature-wanted-38])

Attachments

(2 attachments, 26 obsolete attachments)

66.50 KB, patch
Paenglab
: review+
Details | Diff | Splinter Review
63.95 KB, patch
Paenglab
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Thunderbird should open the preferences in a tab instead a own window like the Firefox in content preferences.
(Assignee)

Comment 1

5 years ago
Created attachment 815856 [details] [diff] [review]
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);
Attachment #815856 - Flags: feedback?(mconley)
Attachment #815856 - Flags: feedback?(acelists)

Comment 2

5 years ago
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")
(Assignee)

Comment 3

5 years ago
(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.
It's not default in ff, but behind the browser.preferences.inContent pref. We should probably have mail.preferences.inContent
Assignee: nobody → richard.marti
Severity: normal → enhancement
Summary: Open the Preferences in a Tab → Option to Open the Preferences in a Tab
(Assignee)

Comment 5

5 years ago
Created attachment 828857 [details] [diff] [review]
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 ;).
Attachment #815856 - Attachment is obsolete: true
Attachment #815856 - Flags: feedback?(mconley)
Attachment #815856 - Flags: feedback?(acelists)
Attachment #828857 - Flags: feedback?(mconley)
(Assignee)

Comment 6

5 years ago
Created attachment 829734 [details] [diff] [review]
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?
Attachment #828857 - Attachment is obsolete: true
Attachment #828857 - Flags: feedback?(mconley)
Attachment #829734 - Flags: feedback?(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. :)
Attachment #829734 - Flags: feedback?(mconley) → feedback+
(Assignee)

Comment 8

5 years ago
Created attachment 8340684 [details] [diff] [review]
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?
Attachment #829734 - Attachment is obsolete: true
Attachment #8340684 - Flags: feedback?(mconley)
(Assignee)

Comment 9

5 years ago
Created attachment 8346124 [details] [diff] [review]
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.
Ah, found it - see bug 675555.

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

Comment 11

5 years ago
Comment on attachment 8340684 [details] [diff] [review]
WIP v4

Clearing the feedback request. I'm on implementing a preferencesTabType.
Attachment #8340684 - Flags: feedback?(mconley)
(Assignee)

Comment 12

5 years ago
Created attachment 8347724 [details] [diff] [review]
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?
Attachment #8340684 - Attachment is obsolete: true
Attachment #8346124 - Attachment is obsolete: true
Flags: needinfo?(mconley)
(Assignee)

Comment 13

5 years ago
Created attachment 8363197 [details] [diff] [review]
WIP v6

Patch updated to Nightly.
Attachment #8347724 - Attachment is obsolete: true
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?
Flags: needinfo?(mconley)
Created attachment 8374837 [details] [diff] [review]
fix.diff

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

Comment 16

4 years ago
Created attachment 8473195 [details] [diff] [review]
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.
Attachment #8363197 - Attachment is obsolete: true
Attachment #8374837 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Created attachment 8473199 [details] [diff] [review]
WIP v7 in-content dialogs

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

Comment 18

4 years ago
Created attachment 8505884 [details] [diff] [review]
WIP v8

Latest version of my patch.
Attachment #8473195 - Attachment is obsolete: true
Created attachment 8505905 [details] [diff] [review]
Open the Preferences in a Tab
(Assignee)

Comment 20

4 years ago
Created attachment 8506161 [details] [diff] [review]
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.
Attachment #8505884 - Attachment is obsolete: true
Attachment #8505905 - Attachment is obsolete: true
Attachment #8506161 - Flags: review?(mconley)
(Assignee)

Comment 21

4 years ago
Created attachment 8506162 [details] [diff] [review]
inContentDialog.patch

Patch to enable the in-content dialogs like in Firefox with the latest styling.
Attachment #8473199 - Attachment is obsolete: true
Attachment #8506162 - Flags: review?(mconley)
(Assignee)

Comment 22

4 years ago
Created attachment 8506193 [details] [diff] [review]
PrefsInTab.patch

The tab title on OS X should be now always the same.
Attachment #8506161 - Attachment is obsolete: true
Attachment #8506161 - Flags: review?(mconley)
Attachment #8506193 - Flags: review?(mconley)
(Assignee)

Comment 23

4 years ago
Created attachment 8506252 [details] [diff] [review]
PrefsInTab.patch

This patch should really stop now the title changes on OS X by overriding the _selectPane method.
Attachment #8506193 - Attachment is obsolete: true
Attachment #8506193 - Flags: review?(mconley)
Attachment #8506252 - Flags: review?(mconley)
Created attachment 8507419 [details] [diff] [review]
Open the Preferences in a Tab
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?
Attachment #8506252 - Flags: review?(mconley)
(Assignee)

Comment 26

4 years ago
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.

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.
Attachment #8506252 - Attachment is obsolete: true
Attachment #8507419 - Attachment is obsolete: true
Attachment #8509046 - Flags: review?(mconley)
(Assignee)

Comment 27

4 years ago
Created attachment 8509047 [details] [diff] [review]
InContentDialog.patch

Updated to the new file names (aboutPreferences.*). This is more or less a copy of the FX patches.
Attachment #8506162 - Attachment is obsolete: true
Attachment #8506162 - Flags: review?(mconley)
Attachment #8509047 - Flags: review?(mconley)
(Assignee)

Comment 28

4 years ago
(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.
Whiteboard: [ux-feature-wanted-38]
(Assignee)

Comment 29

4 years ago
Created attachment 8519393 [details] [diff] [review]
PrefsInTab.patch

Unbitrotted.

Mike, please read comment 26 and comment 28.
Attachment #8509046 - Attachment is obsolete: true
Attachment #8509046 - Flags: review?(mconley)
Attachment #8519393 - Flags: review?(mconley)
(Assignee)

Comment 30

4 years ago
Created attachment 8519394 [details] [diff] [review]
inContentDialog.patch

Unbitrotted.
Attachment #8509047 - Attachment is obsolete: true
Attachment #8509047 - Flags: review?(mconley)
Attachment #8519394 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Depends on: 1096006
(Assignee)

Comment 31

4 years ago
Created attachment 8523387 [details] [diff] [review]
inContentDialog.patch

Updated the styling after landing of FX bug 1089812.
Attachment #8519394 - Attachment is obsolete: true
Attachment #8519394 - Flags: review?(mconley)
Attachment #8523387 - Flags: review?(mconley)
(Assignee)

Comment 32

4 years ago
Created attachment 8524786 [details] [diff] [review]
InContentDialog.patch

Updated to tip.
Attachment #8523387 - Attachment is obsolete: true
Attachment #8523387 - Flags: review?(mconley)
Attachment #8524786 - Flags: review?(mconley)
(Assignee)

Comment 33

4 years ago
Created attachment 8536694 [details]
MozReview Request: bz://925746/Paenglab
Attachment #8536694 - Flags: review?(mconley)
(Assignee)

Comment 34

4 years ago
/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
(Assignee)

Updated

4 years ago
Attachment #8519393 - Attachment is obsolete: true
Attachment #8519393 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Attachment #8524786 - Attachment is obsolete: true
Attachment #8524786 - Flags: review?(mconley)
(Assignee)

Comment 35

4 years ago
(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.
(Assignee)

Comment 36

4 years ago
/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
(Assignee)

Comment 37

4 years ago
(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.
(Assignee)

Comment 38

4 years ago
/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
(Assignee)

Comment 39

4 years ago
Sorry, found a trailing white space.

Comment 40

4 years ago
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.
(Assignee)

Comment 41

4 years ago
Created attachment 8539796 [details] [diff] [review]
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.
(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?
(Assignee)

Comment 43

4 years ago
/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
(Assignee)

Comment 44

4 years ago
(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.
(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.
(Assignee)

Comment 46

4 years ago
(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?
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.
(Assignee)

Comment 48

4 years ago
Created attachment 8543029 [details] [diff] [review]
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.
Attachment #8539796 - Attachment is obsolete: true
(Assignee)

Comment 49

4 years ago
Oops, forgot to thank you for your help and patience.
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
(Assignee)

Comment 51

4 years ago
/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
(Assignee)

Comment 52

4 years ago
(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.
Status: NEW → ASSIGNED
Attachment #8536694 - Flags: review?(mconley) → review+
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.
(Assignee)

Comment 54

4 years ago
Created attachment 8543504 [details] [diff] [review]
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.
Attachment #8536694 - Attachment is obsolete: true
Attachment #8543029 - Attachment is obsolete: true
Flags: needinfo?(mconley)
Attachment #8543504 - Flags: review+
(Assignee)

Comment 55

4 years ago
Created attachment 8543505 [details] [diff] [review]
inContentDialog.patch for check-in
Attachment #8543505 - Flags: review+
Yes that sounds fine to me. Thanks Paenglab!
Flags: needinfo?(mconley)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 37.0

Comment 58

4 years ago
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?
(Assignee)

Comment 59

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

Updated

4 years ago
Depends on: 1120179

Updated

4 years ago
Depends on: 1120181

Updated

4 years ago
Depends on: 1120182

Updated

4 years ago
Depends on: 1120183

Updated

4 years ago
Depends on: 1120184

Updated

4 years ago
Depends on: 1120187
(Assignee)

Updated

4 years ago
Flags: in-moztrap?

Comment 60

2 years ago
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).
(Assignee)

Comment 61

2 years ago
Where is no plan to remove the ability to open the prefs in a window.
You need to log in before you can comment on or make changes to this bug.