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

RESOLVED FIXED in Thunderbird 67.0

Status

defect
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: jorgk, Assigned: mkmelin)

Tracking

(Blocks 1 bug)

Thunderbird 67.0

Thunderbird Tracking Flags

(thunderbird66 fixed, thunderbird67 fixed)

Details

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

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

4 months ago

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.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(geoff)
(Reporter)

Updated

4 months ago
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]
(Assignee)

Comment 1

4 months 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.

Flags: needinfo?(mkmelin+mozilla)
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]
(Reporter)

Comment 2

4 months ago
(Assignee)

Comment 3

4 months 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.

Assignee: nobody → mkmelin+mozilla
Flags: needinfo?(geoff)
(Reporter)

Updated

4 months ago
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]

Comment 4

4 months 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

4 months ago
Posted patch bug1521016_colpicker.patch (obsolete) — Splinter Review

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

Attachment #9037772 - Flags: review?(arshdkhn1)
(Assignee)

Updated

4 months ago
Status: NEW → ASSIGNED
Summary: TEST-UNEXPECTED-FAIL | [snip]/folder-display/test-columns.js → TEST-UNEXPECTED-FAIL | [snip]/folder-display/test-columns.js - de-xbl threadPaneColumnPickerBindings
(Assignee)

Comment 6

4 months 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 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
Attachment #9037772 - Flags: review?(arshdkhn1) → review-
(Assignee)

Comment 8

4 months ago

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

(Assignee)

Comment 9

4 months ago
Posted patch bug1521016_colpicker.patch (obsolete) — Splinter Review

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

Attachment #9037772 - Attachment is obsolete: true
Attachment #9037813 - Flags: review?(arshdkhn1)

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 on attachment 9037813 [details] [diff] [review]
bug1521016_colpicker.patch

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

treecol-image not showing..
Attachment #9037813 - Flags: review?(arshdkhn1) → review-
(Reporter)

Comment 12

4 months 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

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

Duplicate of this bug: 1521768
Summary: TEST-UNEXPECTED-FAIL | [snip]/folder-display/test-columns.js - de-xbl threadPaneColumnPickerBindings → TEST-UNEXPECTED-FAIL | [snip]/folder-display/test-columns.js - de-xbl threadPaneColumnPickerBindings | No columnpicker popup shown
(Assignee)

Comment 15

4 months ago
Posted patch bug1521016_colpicker.patch (obsolete) — Splinter Review

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.

Attachment #9037813 - Attachment is obsolete: true
Attachment #9040690 - Flags: review?(arshdkhn1)
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?
Attachment #9040690 - Flags: review?(arshdkhn1) → review+
(Assignee)

Comment 17

4 months 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

4 months ago
Posted patch bug1521016_colpicker.patch (obsolete) — Splinter Review
Attachment #9040690 - Attachment is obsolete: true
Attachment #9040701 - Flags: review+
(Reporter)

Comment 20

4 months ago

👎

(Assignee)

Comment 21

3 months ago

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

Attachment #9042270 - Flags: review+
(Assignee)

Updated

3 months ago
Attachment #9040701 - Attachment is obsolete: true
(Reporter)

Comment 22

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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.

Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 28

3 months ago

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

Flags: needinfo?(mkmelin+mozilla)
(Reporter)

Comment 29

3 months ago

Great (if it works), thanks!

(Assignee)

Updated

3 months ago
Attachment #9042435 - Flags: review?(jorgk)
(Reporter)

Comment 30

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

Looks good and passed the tests :-)
Attachment #9042435 - Flags: review?(jorgk) → review+
(Assignee)

Updated

3 months ago
(Reporter)

Updated

3 months ago
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 67.0

Comment 31

3 months 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

Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.