Open Bug 767118 Opened 7 years ago Updated 7 years ago

Switch to using XUL Commands instead of inline Javascript

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: mconley, Assigned: mconley)

Details

Attachments

(1 file, 1 obsolete file)

We have a bunch of functions in Thunderbird that follow this pattern:

<someElement onclick="doSomething(this);" />

or

<someElement oncommand="doSomething(this);" ?>

What we should do is actually use XUL Commands (see https://developer.mozilla.org/en/XUL_Tutorial/Commands).

Why this extra level of abstraction? Well, it might help DRY up our code where commands can be triggered from more than one interface.

But the *big* win would be that we could run a Test Pilot study to determine what things people tend to click on in Thunderbird. We could get a heatmap going, like this: http://blog.mozilla.org/ux/2012/06/firefox-heatmap-study-2012-results-are-in/

That'd be HUGE for informing future UX and UI design.
Assignee: nobody → mconley
I think I want to attack this piecemeal, as opposed to one massive patch - so here's File > New.

I'm a little concerned that I'm missing some edge cases with the New Message thing that I'm removing from mailOverlay.js. Any insight would be much appreciated.

Florian - I'm modifying a few IM commands, so I thought I'd have you take a look.

All feedback welcome.
Attachment #635888 - Flags: review?(squibblyflabbetydoo)
Attachment #635888 - Flags: review?(florian)
Will you also do Preferences and Account manager? This will be monster work.
(In reply to :aceman from comment #2)
> Will you also do Preferences and Account manager? This will be monster work.

Yeah, this is a lot of work. Feel like divvy'ing it up?
(In reply to :aceman from comment #2)
> Will you also do Preferences and Account manager? This will be monster work.

Since those don't have menubars, I don't know if there's much practical purpose to doing so.
Just a small fix - I didn't set the command correctly for Virtual Folders.
Attachment #635888 - Attachment is obsolete: true
Attachment #635888 - Flags: review?(squibblyflabbetydoo)
Attachment #635888 - Flags: review?(florian)
Attachment #635894 - Flags: review?(squibblyflabbetydoo)
Attachment #635894 - Flags: review?(florian)
(In reply to Mike Conley (:mconley) from comment #3)
> (In reply to :aceman from comment #2)
> > Will you also do Preferences and Account manager? This will be monster work.
> Yeah, this is a lot of work. Feel like divvy'ing it up?
No, I just worry how you kill all my patches there ;)
Comment on attachment 635894 [details] [diff] [review]
File > New items converted to commands - Patch v2 [checked-in]

The changes touching IM-related code look OK. I'm assuming that you have tested this of course (especially, tested that the "Add Contact" button of the Chat toolbar gets correctly disabled/enabled).
Attachment #635894 - Flags: review?(florian) → review+
Hardware: x86 → All
Comment on attachment 635894 [details] [diff] [review]
File > New items converted to commands - Patch v2 [checked-in]

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

This looks good to me.

Sorry about the delay on this! Hopefully now that I have things a bit more set up re: B2G, I'll have more time for Thunderbird reviews. :)
Attachment #635894 - Flags: review?(squibblyflabbetydoo) → review+
Comment on attachment 635894 [details] [diff] [review]
File > New items converted to commands - Patch v2 [checked-in]

Attachment 635894 [details] [diff] checked in to comm-central as https://hg.mozilla.org/comm-central/rev/f80dc5c5d591
Attachment #635894 - Attachment description: File > New items converted to commands - Patch v2 → File > New items converted to commands - Patch v2 [checked-in]
Backing out Attachment 635894 [details] [diff]: https://hg.mozilla.org/comm-central/rev/dec978837c2c

There were some Mozmill test failures that I should have forseen:

SUMMARY-UNEXPECTED-FAIL | test-newmsg-compose-identity.js | test-newmsg-compose-identity.js::test_compose_from_composer
  EXCEPTION: The From address is not correctly selected: 'Tinderbox <tinderbox@invalid.com>' != ' <x@example.com>'.
    at: test-folder-display-helpers.js line 2829
       assert_true test-folder-display-helpers.js 2829
       assert_equals test-folder-display-helpers.js 2816
       checkCompIdentity test-newmsg-compose-identity.js 50
       test_compose_from_composer test-newmsg-compose-identity.js 69

SUMMARY-UNEXPECTED-FAIL | test-newmailaccount.js | test-newmailaccount.js::test_get_new_account_focuses_existing_ap_tab
  EXCEPTION: Timed out waiting for window open!
    at: utils.js line 447
       TimeoutError utils.js 447
       waitFor utils.js 485
       WindowWatcher_waitForWindowOpen test-window-helpers.js 265
       wait_for_new_window test-window-helpers.js 561
       test_get_new_account_focuses_existing_ap_tab test-newmailaccount.js 1243
SUMMARY-UNEXPECTED-FAIL | test-newmailaccount.js | test-newmailaccount.js::test_per_address_prices
  EXCEPTION: Timeout waiting for modal dialog to open.
    at: utils.js line 447
       TimeoutError utils.js 447
       waitFor utils.js 485
       WindowWatcher_waitForModalDialog test-window-helpers.js 365
       wait_for_modal_dialog test-window-helpers.js 598
       test_per_address_prices test-newmailaccount.js 1260
SUMMARY-PASS | test-newmailaccount.js::teardownModule
Are those failures Linux only? But the first one looks real.
Flags: needinfo?(mconley)
(In reply to :aceman from comment #11)
> Are those failures Linux only? But the first one looks real.

I can't remember, and the patch has bitrotted. :/ If you feel like polishing it up, we can chuck it into try and find out...
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.