Closed Bug 1429861 Opened 6 years ago Closed 6 years ago

TB: Restore the toolbar bindings/styles after their removal in bug 1428938

Categories

(Thunderbird :: Toolbars and Tabs, defect)

Product:
Thunderbird
Component:
Toolbars and Tabs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 59.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

(Whiteboard: [Thunderbird-testfailure: Z all])

Attachments

(7 files, 1 obsolete file)

toolbar.patch
80.34 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1429861-follow-up.patch
5.57 KB, patch
Details | Diff | Splinter Review
1429861-testfix.patch
1.25 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1429861-jar.patch
1.11 KB, patch
Details | Diff | Splinter Review
menubar-binding.patch
2.83 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
Bug1429861-Follow-up.patch
2.35 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
Bug1429861-Linux-Follow-up.patch
1.09 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review 
Bug Bug 1428938 removes the legacy toolbar customization code. removes the legacy toolbar customization code.
Attached patch toolbar.patch — — Splinter Review 
I copied the whole toolbar.xml to c-c to have already everything in one file. The m-c toolbar.xml remnant is so small, this does not really count.

I had more time to think about the implementation and ended in a separate bindings.css file where we can add all bindings like xul.css did. I'm planning to move the datetimepicker bindings also to this file in a new bug.

Jörg, this patch should be testable also without the m-c removal (but then you can't really say the bindings/styles comes from m-c or c-c).

Stefan, the toolbar.xml has now paths to messenger but this should be no problem for the browser part of SM.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8941913 - Flags: review?(jorgk)
Can you explain in three sentences what those "legacy" toolsbars are about? In FF they haven't been supported since Australis, so maybe we could remove them, too. Lately we've been moving a whole lot of code to C-C :-(
The toolbar below the tabs with the icons and the search bar is such a toolbar. In short every toolbar you can customize with adding/deleting buttons spacer separators, etc. is such a legacy toolbar. Fx does this now with the files in the browser/components\/customizableui folder. This is also a lot of code. ;) ...and needs porting.
Comment on attachment 8941913 [details] [diff] [review]
toolbar.patch

Too late to try now :-(
Attachment #8941913 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/66e477f2094c
TB: Restore the toolbar bindings/styles after their removal in bug 1428938. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
Blocks: 1429956
This appears to break many mozmill tests.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I haven't built locally, but I assume the UI went broken.
Hmm, my local build works, the UI is fine and also customising a toolbar works. So I think the bug achieved what it set out to do and we pursue the negative consequences in bug 1429956.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Got it, you forgot to replace
+<!ENTITY % customizeToolbarDTD SYSTEM "chrome://global/locale/customizeToolbar.dtd">
That's no longer global.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6f984a633da6
Follow-up: correct customizeToolbarDTD entity. rs=bustage-fix
Strange that it worked locally.
Not at all. The file in the previous location was still there, no one had done a clobber. After removing that file, I got the same failure. And I made sure is worked with the patch, well, at least I started once. I wanted that landed before the M-C merge in the morning.
I'm still seeing a heap of test failures:

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515750144/build/tests/mozmill/tabmail/test-tabmail-customize.js | test-tabmail-customize.js::test_redirects_toolbarbutton_drops [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515750144/build/tests/mozmill/tabmail/test-tabmail-customize.js | test-tabmail-customize.js::teardownModule [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515750144/build/tests/mozmill/tabmail/test-tabmail-dragndrop.js | test-tabmail-dragndrop.js::test_tab_reorder_tabbar [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515750144/build/tests/mozmill/tabmail/test-tabmail-dragndrop.js | test-tabmail-dragndrop.js::test_tab_reorder_detach [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515750144/build/tests/mozmill/tabmail/test-tabmail-dragndrop.js | test-tabmail-dragndrop.js::test_tab_undo [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515750144/build/tests/mozmill/tabmail/test-tabmail-dragndrop.js | test-tabmail-dragndrop.js::test_tab_recentlyClosed [log…] 

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-header-toolbar.js | test-header-toolbar.js::test_customize_header_toolbar_check_default [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-header-toolbar.js | test-header-toolbar.js::test_customize_header_toolbar_reorder_buttons [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-header-toolbar.js | test-header-toolbar.js::test_customize_header_toolbar_separate_window [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-header-toolbar.js | test-header-toolbar.js::test_customize_header_toolbar_remove_buttons [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-header-toolbar.js | test-header-toolbar.js::test_customize_header_toolbar_dialog_style [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-header-toolbar.js | test-header-toolbar.js::test_customize_header_toolbar_change_button_style [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_add_tag_with_really_long_label [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_a11y_attrs [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_more_button_with_many_recipients [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_clicking_star_opens_inline_contact_editor [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_msg_id_context_menu [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_address_book_switch_disabled_on_contact_in_mailing_list [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_that_msg_without_date_clears_previous_headers [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_more_widget [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_show_all_header_mode [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_more_widget_with_maxlines_of_3 [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_more_widget_with_disabled_more [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-phishing-bar.js | test-phishing-bar.js::test_ignore_phishing_warning_from_message [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-phishing-bar.js | test-phishing-bar.js::test_no_phishing_warning_for_ip_sameish_text [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-phishing-bar.js | test-phishing-bar.js::test_no_phishing_warning_for_subdomain [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-phishing-bar.js | test-phishing-bar.js::test_phishing_warning_for_local_domain [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-reply-identity.js | test-reply-identity.js::test_reply_no_matching_identity [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-reply-identity.js | test-reply-identity.js::test_reply_matching_only_deliveredto [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-reply-identity.js | test-reply-identity.js::test_reply_matching_subaddress [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-reply-identity.js | test-reply-identity.js::test_reply_to_matching_second_id [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-reply-identity.js | test-reply-identity.js::test_deliveredto_to_matching_only_parlty [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-reply-identity.js | test-reply-identity.js::test_reply_to_self_second_id [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-reply-to-list-from-address-selection.js | test-reply-to-list-from-address-selection.js::test_Reply_To_List_From_Address [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-return-receipt.js | test-return-receipt.js::test_no_mdn_for_normal_msgs [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-return-receipt.js | test-return-receipt.js::test_basic_mdn_shown [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-return-receipt.js | test-return-receipt.js::test_basic_mdn_shown_nonrfc [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-return-receipt.js | test-return-receipt.js::test_mdn_when_from_and_disposition_to_differs [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-return-receipt.js | test-return-receipt.js::test_mdn_when_from_and_disposition_to_differs_nonrfc [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-return-receipt.js | test-return-receipt.js::test_mdn_when_disposition_to_multi [log…]
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1515751587/build/tests/mozmill/message-header/test-return-receipt.js | test-return-receipt.js::test_mdn_when_disposition_to_multi_nonrfc [log…] 

We have two failing tests:
tabmail/test-tabmail-customize.js
message-header/test-header-toolbar.js

I ran test-tabmail-customize.js locally and saw:
TEST-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\tabmail\test-tabmail-customize.js | test-tabmail-customize.js::test_redirects_toolbarbutton_drops
TEST-START | c:\mozilla-source\comm-central\mail\test\mozmill\tabmail\test-tabmail-customize.js | teardownModule
TEST-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\tabmail\test-tabmail-customize.js | test-tabmail-customize.js::teardownModule
JavaScript error: chrome://messenger/content/customizeToolbar.js, line 114: TypeError: elts[i] is undefined
--DOCSHELL 10497000 == 12 [pid = 6904] [id = {cf48c47b-4c21-442a-b619-d943ed47a7f1}]

I also ran message-header/test-header-toolbar.js. The Customise Toolbar panel come up and then nothing happens. I see test after test fail and in the end I get a heap of:
EXCEPTION: Timed out waiting for window open!

The other ones that fail are subsequent failures, I ran them locally and they passed.
tabmail/test-tabmail-dragndrop.js
message-header/test-phishing-bar.js
message-header/test-reply-identity.js
message-header/test-reply-to-list-from-address-selection.js
message-header/test-return-receipt.js
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [Thunderbird-testfailure: Z all]
I missed one test file to change the link. I hope this is the culprit of the failures.
Attachment #8942284 - Flags: review?(jorgk)
Comment on attachment 8942284 [details] [diff] [review]
1429861-testfix.patch

The fix looks correct, however, it doesn't fix the test.
mozmake SOLO_TEST=tabmail/test-tabmail-customize.js mozmill-one
still fails. I haven't tried the other one again.
Attachment #8942284 - Flags: review?(jorgk) → review+
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a39c71c8bd8a
Follow-up: Fix the style link in one missed test file. r=jorgk
Another one we missed, pointed out by Aceman.
Alright, with the latest patch and the patch from bug 1430280 message-header/test-header-toolbar.js now passes.

tabmail/test-tabmail-customize.js goes further and gets to:

SUMMARY-PASS | test-tabmail-customize.js::setupModule
SUMMARY-PASS | test-tabmail-customize.js::test_open_context_menu
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\tabmail\test-tabmail-customize.js | test-tabmail-customize.js::test_redirects_toolbarbutton_drops
  EXCEPTION: a != b: 'undefined' != 'menubar-items,spring'.
    at: test-folder-display-helpers.js line 2917
       assert_true test-folder-display-helpers.js:2917 11
       assert_equals test-folder-display-helpers.js:2904 3
       CustomizeDialogHelper_restoreDefaultButtons test-customization-helpers.js:110 5
       test_redirects_toolbarbutton_drops test-tabmail-customize.js:59 3
       Runner.prototype.wrapper frame.js:589 9
       Runner.prototype._runTestModule frame.js:659 9
       Runner.prototype.runTestModule frame.js:705 3
       Runner.prototype.runTestFile frame.js:538 3
       runTestFile frame.js:717 3
       Bridge.prototype._execFunction server.js:177 10
       Bridge.prototype.execFunction server.js:181 16
       Session.prototype.receive server.js:280 3
       AsyncRead.prototype.onDataAvailable server.js:88 3

So we're still missing something somewhere, but it's getting better.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/03cfb25c0204
Follow-up: fix missed overlay in jar.mn. rs=bustage-fix
So toolbar.currentSet returns undefined? Is the function currentSet() missing, or something inside it is not set? You can probably trace that. Also, does customizeToolbar{Overlay}.xul link in the bindings.css?
After the last push we're down to on failure on Linux and Mac:

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/tabmail/test-tabmail-customize.js | test-tabmail-customize.js::test_redirects_toolbarbutton_drops [log…]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/tabmail/test-tabmail-customize.js | test-tabmail-customize.js::teardownModule [log…]

On Windows there is one more:

TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\message-window\test-autohide-menubar.js | test-autohide-menubar.js::test_autohidden_menubar_3pane [log…]
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\message-window\test-autohide-menubar.js | test-autohide-menubar.js::test_autohidden_menubar_message_window [log…]

That doesn't appear to be a subsequent failure since it's from a different directory. Locally I see:

SUMMARY-PASS | test-autohide-menubar.js::setupModule
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\message-window\test-autohide-menubar.js | test-autohide-menubar.js::test_autohidden_menubar_3pane
  EXCEPTION: Menu entry 'menuitem[toolbarid="mail-toolbar-menubar2"]' not found.
    at: controller.js line 223
       getItem controller.js:223 13
       set_autohide_menubar test-autohide-menubar.js:45 18
       help_test_autohide test-autohide-menubar.js:65 3
       test_autohidden_menubar_3pane test-autohide-menubar.js:79 3
       Runner.prototype.wrapper frame.js:589 9
       Runner.prototype._runTestModule frame.js:659 9
       Runner.prototype.runTestModule frame.js:705 3
       Runner.prototype.runTestFile frame.js:538 3
       runTestFile frame.js:717 3
       Bridge.prototype._execFunction server.js:177 10
       Bridge.prototype.execFunction server.js:181 16
       Session.prototype.receive server.js:280 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\message-window\test-autohide-menubar.js | test-autohide-menubar.js::test_autohidden_menubar_message_window
  EXCEPTION: Menu entry 'menuitem[toolbarid="mail-toolbar-menubar2"]' not found.
    at: controller.js line 223
       getItem controller.js:223 13
       set_autohide_menubar test-autohide-menubar.js:45 18
       help_test_autohide test-autohide-menubar.js:65 3
       test_autohidden_menubar_message_window test-autohide-menubar.js:90 3
       Runner.prototype.wrapper frame.js:589 9
       Runner.prototype._runTestModule frame.js:659 9
       Runner.prototype.runTestModule frame.js:705 3
       Runner.prototype.runTestFile frame.js:538 3
       runTestFile frame.js:717 3
       Bridge.prototype._execFunction server.js:177 10
       Bridge.prototype.execFunction server.js:181 16
       Session.prototype.receive server.js:280 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-PASS | test-autohide-menubar.js::test_autohidden_menubar_compose_window
SUMMARY-PASS | test-autohide-menubar.js::test_autohidden_menubar_address_book
SUMMARY-PASS | test-autohide-menubar.js::teardownModule

(In reply to :aceman from comment #21)
> So toolbar.currentSet returns undefined?
Yes.

> Is the function currentSet() missing, or something inside it is not set?
I'd have to check. I see no such function. In the ported common/bindings/toolbar.xml we have many mentions of currentSet, for example here:
https://hg.mozilla.org/comm-central/rev/66e477f2094c#l5.86

> Also, does customizeToolbar{Overlay}.xul link in the bindings.css?
No, should it? Looks like the only way the toolbar.xml stuff gets into the code is via those bindings. Let me add them to see.
Adding the binding to both customizeToolbar{Overlay}.xul doesn't fix any of the two tests.

This is a nice lesson on forked code: Now get get to debug the stuff we copied :-(
Attached patch menubar-binding.patch (obsolete) — — Splinter Review 
I hope this patch fixes the test-autohide-menubar.js.

Without this patch the menu bar isn't in the toolbar list (right click on a toolbar). With patch it's here again. The xul.css binding points to the m-c toolbar.xml and this one extends the castrated toolbar binding. But we need our fully working toolbar binding. I hope this is the last part we need to do.

Our problem is, there is still code in m-c that we need to override with our forked code.
Attachment #8942450 - Flags: review?(jorgk)
Just to be sure, on the "Customize Toolbar" icons for an addon (Reminderfox) are missing, text/label is OK. Once such a "button" is d&d to the toolbar the icons are displayed. Is this covered by this bug also?
Günther, you need now something like this in your chrome.manifest:

style	chrome://global/content/customizeToolbar.xul	chrome://reminderfox/skin/toolbar.css	appversion<=58.0b3
style	chrome://messenger/content/customizeToolbar.xul	chrome://reminderfox/skin/toolbar.css	appversion>=59.0a1

I haven't tested this but it should work. Please test this and give feedback. When it works, we can add this to the Wiki.
Great it works! 
Checked with Thunderbird/52.5.2 (en-US) and 59.0a1/20180113030201 (also with new profile)
(In reply to Richard Marti (:Paenglab) from comment #24)
> Without this patch the menu bar isn't in the toolbar list (right click on a
> toolbar). With patch it's here again.
Not for me. Right-click on the toolbar of the main window gives me: Mail Toolbar, Folder Pane Toolbar / Customise.

On the compose window and on the address box there is a Menu Bar item.
This should do it now. My first patch worked only with autohide menubar.
Attachment #8942450 - Attachment is obsolete: true
Attachment #8942450 - Flags: review?(jorgk)
Attachment #8942461 - Flags: review?(jorgk)
Tried also adding buttons to the menubar and it works now, also after a restart they are there.
Comment on attachment 8942461 [details] [diff] [review]
menubar-binding.patch

This brings the "Menu Bar" item back to the menu and FIXES BOTH TESTS \O/ !!
Attachment #8942461 - Flags: review?(jorgk) → review+
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b5cb90c44461
Follow-up: Add our own binding to the menubar. r=jorgk
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Test results have just come in:

Linux:
tabmail/test-tabmail-customize.js still failing.

New failures in
addrbook/test-update-mailing-list.js
addrbook/test-address-book.js
test-attachment-reminder.js

Mac:
tabmail/test-tabmail-customize.js NOT failing.

New failures as above plus composition/test-send-button.js

In the logs we have:
EXCEPTION: aCwc.e(...) is null
EXCEPTION: el is null
EXCEPTION: controller.window.document.documentElement.getButton is not a function
EXCEPTION: Timed out waiting for window open!
EXCEPTION: Not a send error dialog; title=Write: (no subject)
and more.

I'll have to wait for the Windows results and then investigate. The new failures are most likely unrelated, but the M-C changes don't look so bad:
M-C last good: 4c4d9381b668fdc9da35a978b36aa313b8
M-C first bad: 04c0a07b8de21300856ec89b7d118d4be9
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c4d9381b668fdc9da35a978b36aa313b8&tochange=04c0a07b8de21300856ec89b7d118d4be9
I can't see any culprit there.
addrbook/test-update-mailing-list.js fails locally, without the latest M-C changeset.
addrbook/test-address-book.js fails, composition/test-attachment-reminder.js fails.

Oh, looking at the tests, Richard, the compose window has lost its buttons :-(
Status: RESOLVED → REOPENED
Flags: needinfo?(richard.marti)
Resolution: FIXED → ---
And the address book window, too :-(
Ahh shoot, with removing the :not([type="menubar"]) in previous patch the .toolbar-primary rule was more specific and set the binding back to m-c toolbar. I don't see why we need this rule -> removed it. I file a bug to remove this class entirely. AB, composer and tabs-toolbar use this class and I don't know for what it's good for.

Found two other occurrences which point to m-c toolbar binding and changed them.
Flags: needinfo?(richard.marti)
Attachment #8942490 - Flags: review?(jorgk)
Comment on attachment 8942490 [details] [diff] [review]
Bug1429861-Follow-up.patch

I've checked the main window, address book window, compose window, message stand-alone window and chat. All appear fine. I ran
mozmake SOLO_TEST=tabmail/test-tabmail-customize.js mozmill-one
mozmake SOLO_TEST=message-window/test-autohide-menubar.js mozmill-one
mozmake SOLO_TEST=addrbook/test-update-mailing-list.js mozmill-one
mozmake SOLO_TEST=addrbook/test-address-book.js mozmill-one
mozmake SOLO_TEST=composition/test-attachment-reminder.js mozmill-one
mozmake SOLO_TEST=composition/test-send-button.js mozmill-one
manually, all passed.
Attachment #8942490 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/73858a618068
Follow-up: Remove the .toolbar-primary rule. r=jorgk
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
(In reply to Jorg K (GMT+1) from comment #39)
> Actually, what about:
> https://dxr.mozilla.org/comm-central/source/mail/themes/windows/mail/
> messenger.css#69

AB and composer don't need it but the tabs-toolbar. I'll change this when I remove this class.
Mac is green now and I expect Windows to be green as well. Linux still failing:
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/tabmail/test-tabmail-customize.js | test-tabmail-customize.js::test_redirects_toolbarbutton_drops.

Maybe Aceman can check that locally.
Flags: needinfo?(acelists)
Richard, can you please check this on your Linux VM. We're still getting errors on Linux:
https://public-artifacts.taskcluster.net/RTl3Ch64QruwqwIUQ3ASAg/0/public/logs/live_backing.log
INFO -    EXCEPTION: a != b: 'undefined' != 'menubar-items,spring'.
INFO -      at: test-folder-display-helpers.js line 2917
INFO -         assert_true test-folder-display-helpers.js:2917 11
INFO -         assert_equals test-folder-display-helpers.js:2904 3
INFO -         CustomizeDialogHelper_restoreDefaultButtons test-customization-helpers.js:110 5
INFO -         test_redirects_toolbarbutton_drops test-tabmail-customize.js:59 3

We had the same issue on the other platforms but there is got fixed, so something must be going on for Linux.
Flags: needinfo?(richard.marti)
Now there was a rule in global.css we need to override. I put it in bindings.css to have it everywhere where the bindings are needed. Checked also on Mac and Windows that this rule doesn't regress.

I don't ask aceman for review as IIRC he is on a older revision for an other bug.
Flags: needinfo?(richard.marti)
Attachment #8942712 - Flags: review?(jorgk)
Comment on attachment 8942712 [details] [diff] [review]
Bug1429861-Linux-Follow-up.patch

This fixes the Linux failure and doesn't regress anything for Mac. Thanks. Hopefully we're finally done here :-)
Flags: needinfo?(acelists)
Attachment #8942712 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/98f69558bff6
Follow-up: Add the toolbar-drag to bindings.css for Linux. r=jorgk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: