Closed
Bug 1429861
Opened 7 years ago
Closed 7 years ago
TB: Restore the toolbar bindings/styles after their removal in bug 1428938
Categories
(Thunderbird :: Toolbars and Tabs, defect)
Thunderbird
Toolbars and Tabs
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 59.0
People
(Reporter: Paenglab, Assigned: Paenglab)
References
Details
(Whiteboard: [Thunderbird-testfailure: Z all])
Attachments
(7 files, 1 obsolete file)
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
Bug Bug 1428938 removes the legacy toolbar customization code. removes the legacy toolbar customization code.
Assignee | ||
Comment 1•7 years ago
|
||
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.
Comment 2•7 years ago
|
||
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 :-(
Assignee | ||
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Target Milestone: --- → Thunderbird 59.0
Comment 6•7 years ago
|
||
This appears to break many mozmill tests.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 7•7 years ago
|
||
I haven't built locally, but I assume the UI went broken.
Comment 8•7 years ago
|
||
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: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 9•7 years ago
|
||
Got it, you forgot to replace
+<!ENTITY % customizeToolbarDTD SYSTEM "chrome://global/locale/customizeToolbar.dtd">
That's no longer global.
Comment 10•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6f984a633da6
Follow-up: correct customizeToolbarDTD entity. rs=bustage-fix
Comment 11•7 years ago
|
||
Comment 12•7 years ago
|
||
Strange that it worked locally.
Comment 13•7 years ago
|
||
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.
Comment 14•7 years ago
|
||
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
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [Thunderbird-testfailure: Z all]
Assignee | ||
Comment 15•7 years ago
|
||
I missed one test file to change the link. I hope this is the culprit of the failures.
Attachment #8942284 -
Flags: review?(jorgk)
Comment 16•7 years ago
|
||
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+
Updated•7 years ago
|
Keywords: leave-open
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
Another one we missed, pointed out by Aceman.
Comment 19•7 years ago
|
||
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.
Comment 20•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/03cfb25c0204
Follow-up: fix missed overlay in jar.mn. rs=bustage-fix
Comment 21•7 years ago
|
||
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?
Comment 22•7 years ago
|
||
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.
Comment 23•7 years ago
|
||
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 :-(
Assignee | ||
Comment 24•7 years ago
|
||
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)
Comment 25•7 years ago
|
||
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?
Assignee | ||
Comment 26•7 years ago
|
||
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.
Comment 27•7 years ago
|
||
Great it works!
Checked with Thunderbird/52.5.2 (en-US) and 59.0a1/20180113030201 (also with new profile)
Comment 28•7 years ago
|
||
Comment 29•7 years ago
|
||
(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.
Assignee | ||
Comment 30•7 years ago
|
||
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)
Assignee | ||
Comment 31•7 years ago
|
||
Tried also adding buttons to the menubar and it works now, also after a restart they are there.
Comment 32•7 years ago
|
||
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+
Updated•7 years ago
|
Keywords: leave-open
Comment 33•7 years ago
|
||
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: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 34•7 years ago
|
||
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.
Comment 35•7 years ago
|
||
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 → ---
Comment 36•7 years ago
|
||
And the address book window, too :-(
Assignee | ||
Comment 37•7 years ago
|
||
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 38•7 years ago
|
||
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+
Comment 39•7 years ago
|
||
Actually, what about:
https://dxr.mozilla.org/comm-central/source/mail/themes/windows/mail/messenger.css#69
Comment 40•7 years ago
|
||
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: 7 years ago → 7 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 41•7 years ago
|
||
(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.
Comment 42•7 years ago
|
||
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)
Comment 43•7 years ago
|
||
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)
Assignee | ||
Comment 44•7 years ago
|
||
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 45•7 years ago
|
||
Thanks, let's see what this does:
https://hg.mozilla.org/try-comm-central/pushloghtml?changeset=8a347085493f6bea66793661d523af35a4758681
Comment 46•7 years ago
|
||
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+
Comment 47•7 years ago
|
||
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.
Description
•