TEST-UNEXPECTED-FAIL | [snip]/folder-display/test-columns.js - de-xbl threadPaneColumnPickerBindings | No columnpicker popup shown
Categories
(Thunderbird :: General, defect)
Tracking
(thunderbird66 fixed, thunderbird67 fixed)
People
(Reporter: jorgk-bmo, Assigned: mkmelin)
References
Details
(Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test])
Attachments
(3 files, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1547771879/build/tests/mozmill/folder-display/test-columns.js | test-columns.js::test_reset_to_inbox
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1547771879/build/tests/mozmill/folder-display/test-columns.js | test-columns.js::test_apply_to_folder_no_children
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1547771879/build/tests/mozmill/folder-display/test-columns.js | test-columns.js::test_apply_to_folder_and_children
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1547771879/build/tests/mozmill/folder-display/test-columns.js | test-columns.js::test_apply_to_folder_no_children_swapped
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1547771879/build/tests/mozmill/folder-display/test-columns.js | test-columns.js::test_apply_to_folder_and_children_swapped
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1547771879/build/tests/mozmill/folder-display/test-columns.js | test-columns.js::test_reset_columns_gloda_collection
M-C last good: 1bbec154fb73b0fa3756a9ed417815c87f
M-C first bad: 081c6ac45c5d2fb2d64465ec5d5f18771f
https://taskcluster-artifacts.net/Iy9kycmbR5C8XRVtdoXLYQ/0/public/logs/live_backing.log
INFO - EXCEPTION: Popup never opened! id=, state=closed
INFO - at: utils.js line 396
INFO - TimeoutError utils.js:396 13
INFO - waitFor utils.js:452 11
INFO - _click_menus test-window-helpers.js:982 9
INFO - invoke_column_picker_option test-columns.js:361 3
Some stuff already broken in folder display, so I'll switch those off.
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Since these tests are about columns, from bug 1507704.
https://searchfox.org/comm-central/source/mail/test/mozmill/folder-display/test-columns.js#357
Moment, I'll attach a likely patch.
Reporter | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Ouch, it's not just the test adjustment. We have https://searchfox.org/comm-central/source/mail/base/content/threadPaneColumnPicker.xml which now wouldn't work. The good news is we could just scrap that now and make a customzied built-in treecolpicker.
It's some hours of work to figure out properly though. I can look at it later today.
Reporter | ||
Updated•6 years ago
|
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/11acf228786f Disable failing subtests of folder-display/test-columns.js. rs=bustage-fix
Assignee | ||
Comment 5•6 years ago
|
||
test-columns.js::test_reset_columns_gloda_collection is still failing, but other than that, this seems to work
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 6•6 years ago
|
||
For reference, this is the widget in the thread pane where you can select which columns to show.
The standard widget does not have an "Apply columns to..." menu item, which Thunderbird has, (re)implemented here.
Comment 7•6 years ago
|
||
Comment on attachment 9037772 [details] [diff] [review] bug1521016_colpicker.patch Review of attachment 9037772 [details] [diff] [review]: ----------------------------------------------------------------- treecol-image is not showing.. Something is messing up. ::: mail/base/content/mailWidgets.js @@ +343,5 @@ > + super.connectedCallback(); > + if (this.delayConnectedCallback()) { > + return; > + } > + ChromeUtils.import("resource://gre/modules/Services.jsm"); If I recall correctly, you prefer moving such imports to top of file? why is that? @@ +368,5 @@ > + (useChildren ? "withChildren." : "noChildren."); > + let confirmed = Services.prompt.confirm(null, > + bundle.getString(stringBase + "title"), > + bundle.getFormattedString(stringBase + "message", [destFolder.prettyName])); > + if (confirmed) I guess you prefere having curly brace style for one statement if block? @@ +383,5 @@ > + confirmApply(event.originalTarget._folder, true); > + }); > + } > + > + // XXX: this shouldn't need to be overridden. ATM, the removal of children XXX should be replaced with something else here, no? @@ +387,5 @@ > + // XXX: this shouldn't need to be overridden. ATM, the removal of children > + // while aPopup.childNodes.length > 2 is forcing us. We have three since we > + // add one menu. > + /** @override */ > + buildPopup(aPopup) { this aArgument style! @@ +392,5 @@ > + // We no longer cache the picker content, remove the old content related to > + // the cols - menuitem and separator should stay. > + this.querySelectorAll("[colindex]").forEach((e) => { e.remove(); }); > + > + var refChild = aPopup.firstChild; let/const? @@ +406,5 @@ > + var columnName = currElement.getAttribute("display") || > + currElement.getAttribute("label"); > + popupChild.setAttribute("label", columnName); > + popupChild.setAttribute("colindex", currCol.index); > + if (currElement.getAttribute("hidden") != "true") one statement if block with curly style maybe? @@ +408,5 @@ > + popupChild.setAttribute("label", columnName); > + popupChild.setAttribute("colindex", currCol.index); > + if (currElement.getAttribute("hidden") != "true") > + popupChild.setAttribute("checked", "true"); > + if (currCol.primary) one statement if block style
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to Arshad Khan [:arshad] from comment #7)
Comment on attachment 9037772 [details] [diff] [review]
bug1521016_colpicker.patchReview of attachment 9037772 [details] [diff] [review]:
treecol-image is not showing.. Something is messing up.
Definitely showing for me. Can you recheck?
- ChromeUtils.import("resource://gre/modules/Services.jsm");
If I recall correctly, you prefer moving such imports to top of file? why is
that?
Pretty soon it will anyway become something like normal es imports. Those tend to be on top. Services is really global so there is no win trying to import it "conditionally" or late. It's always set. I'll move this to top too.
Re buildPopup and XXX - I intend to sumbit a patch for m-c to fix this method (the < 2 check) so it works for us too and we can get rid of this pointless override. The new code would look like the what i've included now.
I don't want to do any style changes in that patch, since they might not want that.
Assignee | ||
Comment 9•6 years ago
|
||
Refreshed, adjusted, and the mozmill test is now fine at least locally.
Sent to try just now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=732fd86007024d5284979c65b84584ce5d63bb6f
Comment 10•6 years ago
|
||
I am getting errors like https://bugzilla.mozilla.org/show_bug.cgi?id=1515953 had for the elements that you have added. We should fix this first, wdys?
Comment 11•6 years ago
|
||
Comment on attachment 9037813 [details] [diff] [review] bug1521016_colpicker.patch Review of attachment 9037813 [details] [diff] [review]: ----------------------------------------------------------------- treecol-image not showing..
Reporter | ||
Comment 12•6 years ago
|
||
I've tested at the patch without looking at the code. I don't know what the treecol-image
is but the column picker has certainly reappeared :-)
After enabling the tests again, four tests still fail. Note that three tests are disabled on Linux. The failing ones are:
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\folder-display\test-columns.js | test-columns.js::test_apply_to_folder_and_children
EXCEPTION: Found visible column 'sizeCol' but was expecting 'tagsCol'!
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\folder-display\test-columns.js | test-columns.js::test_apply_to_folder_no_children_swapped
EXCEPTION: Found visible column 'sizeCol' but was expecting 'undefined'!
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\folder-display\test-columns.js | test-columns.js::test_apply_to_folder_and_children_swapped
EXCEPTION: Found visible column 'sizeCol' but was expecting 'undefined'!
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\folder-display\test-columns.js | test-columns.js::test_reset_columns_gloda_collection
EXCEPTION: Found visible column 'accountCol' but was expecting 'locationCol'!
So not ready for prime time yet :-(
Assignee | ||
Comment 13•6 years ago
|
||
I would assume treecol-image missing is just due to bug 1515953 (which maybe is more prominent now).
I'll have to look more at reset, but I think it's basically due to different expectations: the toolkit widget will only reset column order, while ours used to reset state (order and which ones).
Updated•6 years ago
|
Assignee | ||
Comment 15•6 years ago
|
||
I would say we're alright with just resetting the column order and using the functionality toolkit already provides.
The test_reset_columns_gloda_collection test is somewhat nonsense now, but moving around the columns in the test requires more code which I don't have time to do atm.
I think we can solve the treecol-image not showing the icon on Mac issue later if that's still a problem.
Comment 16•6 years ago
|
||
Comment on attachment 9040690 [details] [diff] [review] bug1521016_colpicker.patch Review of attachment 9040690 [details] [diff] [review]: ----------------------------------------------------------------- The icons are now visible somehow and the errro in console is gone as well. ::: mail/base/content/mailWidgets.js @@ +9,5 @@ > /* global onClickEmailStar */ > /* global onClickEmailPresence */ > +/* global gFolderDisplay */ > + > +var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm"); `const` can be used here? @@ +481,5 @@ > customElements.define("mail-newsgroups-headerfield", MozMailNewsgroupsHeaderfield); > customElements.define("mail-messageid", MozMailMessageid); > customElements.define("mail-emailaddress", MozMailEmailaddress); > customElements.define("mail-emailheaderfield", MozMailEmailheaderfield); > + is an extra empty line added?
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to Arshad Khan [:arshad] from comment #16)
+var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
const
can be used here?
Nope, the file is not a module so you'd get redeclaration errors.
Assignee | ||
Comment 18•6 years ago
|
||
Assignee | ||
Comment 19•6 years ago
|
||
Let's see what try says - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e74ac47f78992360edd22c4b477f952b32d17593
Assignee | ||
Comment 21•6 years ago
|
||
Should be it, works at least locally.
Dunno why try doesn't want to cooperate atm.
Assignee | ||
Updated•6 years ago
|
Reporter | ||
Comment 22•6 years ago
|
||
Well, three tests are disabled on Linux:
Line 466: test_apply_to_folder_and_children.EXCLUDED_PLATFORMS = ['linux']; // See bug 1406717.
Line 513: test_apply_to_folder_no_children_swapped.EXCLUDED_PLATFORMS = ['linux']; // See bug 1406717.
Line 554: test_apply_to_folder_and_children_swapped.EXCLUDED_PLATFORMS = ['linux']; // See bug 1406717.
These three still fail in Windows. I'll land this now, but we still need to look into these failures.
Reporter | ||
Comment 23•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #21)
Dunno why try doesn't want to cooperate atm.
Because you're not at tip locally?
Comment 24•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/029ec755385f remove threadPaneColumnPicker and instead use customzied built-in thread-pane-treecols and thread-pane-treecolpicker for the purpose. r=arshad
Comment 25•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/c77b06a780fb Follow-up: Add missing import of MailServices (causing linting error). rs=bustage-fix DONTBUILD
Assignee | ||
Comment 26•6 years ago
|
||
(In reply to Jorg K (GMT+1) from comment #23)
Because you're not at tip locally?
Indeed, that tree was not up to date. Too late for my brain to catch that apparently.
Reporter | ||
Comment 27•6 years ago
|
||
TB 66 beta:
https://hg.mozilla.org/releases/comm-beta/rev/86cb9d0c8982
I landed what we had after rebasing it due to the import changes that hit 67 but not 66. That also took care of the follow-up.
Magnus, can we please get the tests mentioned in comment #22 fixed.
Assignee | ||
Comment 28•6 years ago
|
||
Fix the test expectations, since reset now only resets the order, not to any defined state.
Works fine at least locally, let's see if bug 1406717 is actually fixed
Assignee | ||
Updated•6 years ago
|
Reporter | ||
Comment 30•6 years ago
|
||
Comment on attachment 9042435 [details] [diff] [review] bug1521016_colpicker_moretests.patch Looks good and passed the tests :-)
Assignee | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Comment 31•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bdc549022762
adjust test expectations now that "reset" only resets order, not actual state. r=jorgk
Reporter | ||
Comment 32•6 years ago
|
||
Description
•