1565789 - Remove the aTabID in openPreferencesTab()

Status: RESOLVED FIXED

(Thunderbird :: Preferences, task)

Description

[Richard Marti (:Paenglab)]

With bug 1498332 we remove the tabs in the pref panes. This makes the aTabID no more needed and we should remove it.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1565789-update-prefs-callers.patch

I had a better idea. Instead of removing the tabID, use it as an ID which scrolls into the view of the prefs (focusID). Like this the user doesn't need to find the correct section to work on it. Especially with long panes this is a big help.

Comment 2

[Richard Marti (:Paenglab)]

Attachment: 1565789-update-prefs-callers.patch

Forgot a period.

Comment 3

[Geoff Lankow (:darktrojan)]

Comment on attachment 9077987 [details] [diff] [review]
1565789-update-prefs-callers.patch

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

This is good but consider using "scroll" ("scrollTarget"?) where you've used "focus" since we're not focussing anything.

::: mail/base/content/mailWindowOverlay.js
@@ +2910,5 @@
>   *  The dialog window will take care of reloading the message when
>   *  invoked in instantApply mode.
>   */
>  function OpenPhishingSettings() {
> +  openOptionsDialog("panePrivacy");

Second argument missing?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2222,5 @@
>    // Construct the notification as we don't have one.
>    let msg = document.createXULElement("hbox");
>    msg.setAttribute("flex", "100");
>    msg.onclick = function(event) {
> +    openOptionsDialog("paneCompose",

Second argument missing.

::: mail/components/preferences/preferences.js
@@ +108,3 @@
>   * @param otherArgs.subdialog ID of button to activate, opening a subdialog
>   */
> +function selectPaneAndTab(prefWindow, paneID, focusID, otherArgs) {

This should be renamed. Maybe just selectPane.

@@ +127,3 @@
>   * @param subdialogID  ID of button to activate, opening a subdialog
>   */
> +function showTab(pane, focusID, subdialogID) {

This should be renamed too (scrollPaneTo?), and the first argument is now unused.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: 1565789-update-prefs-callers.patch

I'm using now scrollPaneTo instead of focusID.
(In reply to Geoff Lankow (:darktrojan) from comment #3)

Comment on attachment 9077987 [details] [diff] [review]
1565789-update-prefs-callers.patch

Review of attachment 9077987 [details] [diff] [review]:

This is good but consider using "scroll" ("scrollTarget"?) where you've used
"focus" since we're not focussing anything.

::: mail/base/content/mailWindowOverlay.js
@@ +2910,5 @@

• The dialog window will take care of reloading the message when
• invoked in instantApply mode.
  */
  function OpenPhishingSettings() {

• openOptionsDialog("panePrivacy");

Second argument missing?

Fixed.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2222,5 @@

// Construct the notification as we don't have one.
let msg = document.createXULElement("hbox");
msg.setAttribute("flex", "100");
msg.onclick = function(event) {

• openOptionsDialog("paneCompose",

Second argument missing.

Not really needed as the target is the subdialog. But it's better when we show the area with the button.

::: mail/components/preferences/preferences.js
@@ +108,3 @@

• @param otherArgs.subdialog ID of button to activate, opening a subdialog
  */
  +function selectPaneAndTab(prefWindow, paneID, focusID, otherArgs) {

This should be renamed. Maybe just selectPane.

Renamed it to selectPrefPane to be unique. selectPane exists already at other places.

@@ +127,3 @@

• @param subdialogID ID of button to activate, opening a subdialog
  */
  +function showTab(pane, focusID, subdialogID) {

This should be renamed too (scrollPaneTo?), and the first argument is now
unused.

Done.

Comment 5

[Richard Marti (:Paenglab)]

This needs to land together with bug 1498332.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a912193380b5
Update the preferences callers after the prefs redesign in bug 1498332. r=darktrojan