1521016 - TEST-UNEXPECTED-FAIL | [snip]/folder-display/test-columns.js - de-xbl threadPaneColumnPickerBindings | No columnpicker popup shown

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

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://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1bbec154fb73b0fa3756a9ed417815c87f&tochange=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.

Comment 1

[Magnus Melin [:mkmelin]]

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.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1521016-disable-subtests.patch

Comment 3

[Magnus Melin [:mkmelin]]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/11acf228786f
Disable failing subtests of folder-display/test-columns.js. rs=bustage-fix

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1521016_colpicker.patch

test-columns.js::test_reset_columns_gloda_collection is still failing, but other than that, this seems to work

Comment 6

[Magnus Melin [:mkmelin]]

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

[Arshad Khan [:arshad]]

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

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to Arshad Khan [:arshad] from comment #7)

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.

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.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: bug1521016_colpicker.patch

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

[Arshad Khan [:arshad]]

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

[Arshad Khan [:arshad]]

Comment on attachment 9037813 [details] [diff] [review]
bug1521016_colpicker.patch

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

treecol-image not showing..

Comment 12

[Jorg K (CEST = GMT+2)]

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 :-(

Comment 13

[Magnus Melin [:mkmelin]]

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).

Comment 15

[Magnus Melin [:mkmelin]]

Attachment: bug1521016_colpicker.patch

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

[Arshad Khan [:arshad]]

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?

Comment 17

[Magnus Melin [:mkmelin]]

(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.

Comment 18

[Magnus Melin [:mkmelin]]

Attachment: bug1521016_colpicker.patch

Comment 19

[Magnus Melin [:mkmelin]]

Let's see what try says - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e74ac47f78992360edd22c4b477f952b32d17593

Comment 20

[Jorg K (CEST = GMT+2)]

👎

Comment 21

[Magnus Melin [:mkmelin]]

Attachment: bug1521016_colpicker.patch

Should be it, works at least locally.
Dunno why try doesn't want to cooperate atm.

Comment 22

[Jorg K (CEST = GMT+2)]

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.

Comment 23

[Jorg K (CEST = GMT+2)]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c77b06a780fb
Follow-up: Add missing import of MailServices (causing linting error). rs=bustage-fix DONTBUILD

Comment 26

[Magnus Melin [:mkmelin]]

(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.

Comment 27

[Jorg K (CEST = GMT+2)]

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.

Comment 28

[Magnus Melin [:mkmelin]]

Attachment: bug1521016_colpicker_moretests.patch

Fix the test expectations, since reset now only resets the order, not to any defined state.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1c9f70c6d6f9624a82885c68853c842b803813cc

Works fine at least locally, let's see if bug 1406717 is actually fixed

Comment 29

[Jorg K (CEST = GMT+2)]

Great (if it works), thanks!

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 9042435 [details] [diff] [review]
bug1521016_colpicker_moretests.patch

Looks good and passed the tests :-)

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bdc549022762
adjust test expectations now that "reset" only resets order, not actual state. r=jorgk

Comment 32

[Jorg K (CEST = GMT+2)]

TB 66 beta:
https://hg.mozilla.org/releases/comm-beta/rev/2d307b1bc4a3fb68e0b19f7b66c1bba2acb490f5