1454106 - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_clicking_star_opens_inline_contact_editor and more

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_clicking_star_opens_inline_contact_editor

TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_address_book_switch_disabled_on_contact_in_mailing_list

Log
https://taskcluster-artifacts.net/NQXFUiXjQaeNsOsBC0A8tQ/0/public/logs/live_backing.log
says:

INFO -  SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_clicking_star_opens_inline_contact_editor
INFO -    EXCEPTION: Timeout waiting for contactPanel to open; state=closed
INFO -      at: utils.js line 406
INFO -         TimeoutError utils.js:406 13
INFO -         waitFor utils.js:444 11
INFO -         MozMillController.prototype.waitFor controller.js:687 3
INFO -         test_clicking_star_opens_inline_contact_editor test-message-header.js:376 3

INFO -  SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_address_book_switch_disabled_on_contact_in_mailing_list
INFO -    EXCEPTION: Timeout waiting for contactPanel to open; state=closed
INFO -      at: utils.js line 406
INFO -         TimeoutError utils.js:406 13
INFO -         waitFor utils.js:444 11
INFO -         MozMillController.prototype.waitFor controller.js:687 3
INFO -         test_address_book_switch_disabled_on_contact_in_mailing_list test-message-header.js:464 3

M-C last good: 8a94faa5cc60495da5d80d4b3c07bf5877
M-C first bad: 6547c27303bc4d8961b11e656751e83980

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8a94faa5cc60495da5d80d4b3c07bf5877&tochange=6547c27303bc4d8961b11e656751e83980

Looks like a bug for Aceman. Nothing obvious in the range, maybe Bug 1445912 looks vaguely suspicious.

Here's a try run with half the regression range, so at M-C at rev fd41fb43d24c:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b257ca599474ee0fbfa390c474b1d05070c4cc04

Comment 1

[Jorg K (CEST = GMT+2)]

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

Comment 2

[Jorg K (CEST = GMT+2)]

Already broken at M-C rev fd41fb43d24c. So:
M-C last good: 8a94faa5cc60
M-C first bad: fd41fb43d24c
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8a94faa5cc60&tochange=fd41fb43d24c

So that hardens the suspicion that Bug 1445912 caused this. I'll try a local build.

Comment 3

[Jorg K (CEST = GMT+2)]

Confirming that M-C rev 7c5124a18c41, Bug 1445912 caused this.

Running
  mozmake SOLO_TEST=message-header/test-message-header.js mozmill-one
locally shows the following in the console:

JavaScript error: chrome://messenger/content/editContactPanel.js, line 165: TypeError: this.panel.boxObject.setConsumeRollupEvent is not a function

That line is:
    this.panel.boxObject.setConsumeRollupEvent(PopupBoxObject.ROLLUP_CONSUME);

So that was removed in https://hg.mozilla.org/mozilla-central/rev/7c5124a18c41, but what is the replacement?

Simply removing the line fixes the test. Is that a valid fix? I'm not even sure what this function was meant to do:
https://hg.mozilla.org/mozilla-central/rev/7c5124a18c41#l1.24

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1454106-no-more-setConsumeRollupEvent.patch

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/360fe1f89a9c
Port bug 1445912: remove call to setConsumeRollupEvent(PopupBoxObject.ROLLUP_CONSUME). rs=bustage-fix

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8967937 [details] [diff] [review]
1454106-no-more-setConsumeRollupEvent.patch

Let me know if you want to implement a replacement.

Comment 7

[:aceman]

Comment on attachment 8967937 [details] [diff] [review]
1454106-no-more-setConsumeRollupEvent.patch

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

Can you replace it with the "consumeoutsideclicks" attribute? See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/panel .

Comment 8

[Jorg K (CEST = GMT+2)]

Surely removing the "faulty" line was the right thing to do, so please reconsider the r- :-)
Or would you like me to back it out and restore the functional and test failure?

Would you like to make a suggestion?

Comment 9

[Dão Gottwald [:dao]]

(In reply to Jorg K (GMT+1) from comment #3)
> JavaScript error: chrome://messenger/content/editContactPanel.js, line 165:
> TypeError: this.panel.boxObject.setConsumeRollupEvent is not a function
> 
> That line is:
>    
> this.panel.boxObject.setConsumeRollupEvent(PopupBoxObject.ROLLUP_CONSUME);
> 
> So that was removed in
> https://hg.mozilla.org/mozilla-central/rev/7c5124a18c41, but what is the
> replacement?

The consumeoutsideclicks attribute.

Comment 10

[:aceman]

(In reply to Jorg K (GMT+1) from comment #8)
> Surely removing the "faulty" line was the right thing to do, so please
> reconsider the r- :-)
> Or would you like me to back it out and restore the functional and test
> failure?

No :) Yeah, we can also play it the way that the landed patch is OK and make a new one adding the replacement attribute.

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to Dão Gottwald [::dao] from comment #9)
> The consumeoutsideclicks attribute.
Thanks for the hint. Aceman also mentioned https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL

None of us can see any difference if the attribute is set or not set. I can see your the specialist here since you've done the replacement work in FF a while ago:
https://hg.mozilla.org/mozilla-central/rev/09e29c4688ad

Would so kind as to explain what to look for? What's the behavioural change achieved with the attribute?

We use this attribute once in TB
https://dxr.mozilla.org/comm-central/rev/c61d7d8e0fc57f689f54dad226a3f0dd6a21b020/mail/components/compose/content/messengercompose.xul#576
and changing the value from false to true makes no perceivable difference.

Comment 12

[:aceman]

Attachment: 1454106.patch use attribute (WIP)

This sets the attribute on the panel instead of calling the removed function.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8968079 [details] [diff] [review]
1454106.patch use attribute (WIP)

Great, thanks, but the question remains: Does this make any difference?

Comment 14

[Dão Gottwald [:dao]]

See the documentation here: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/panel#a-consumeoutsideclicks

Comment 15

[:aceman]

Yes, we see the documentation (and it looks the same as of the removed function). But we do not understand what the feature does.

What is the visual difference of "the event that caused the popup to be automatically dismissed (or "rolled up") should be consumed" vs. "the event that caused the popup to be automatically dismissed (or "rolled up") should be dispatched as a normal event". How can we see by clicking on such a panel whether the event was consumed or not? None of the values seems to prevent closing the panel when clicking outside of it (despite the name of the attribute). So what exactly does it do?

Comment 16

[Dão Gottwald [:dao]]

Whether or not you'll see a difference depends on your OS. As the documentation says: "If it is not set, it defaults to the platform behavior."

Comment 17

[Jorg K (CEST = GMT+2)]

Thanks, but we tried true and false and also saw no difference on Linux and Windows. What should the observed difference be exactly?

Comment 18

[Dão Gottwald [:dao]]

The difference is between the event just dismissing the popup and the event both dismissing the popup and also being consumed outside of the popup.

Comment 19

[Jorg K (CEST = GMT+2)]

Thanks, yes, that's our understanding from bug 395314. An outside click closed some panel but caused it to re-appear straight away.

On Windows, clicking on the item which makes the panel appear, the "address star", and then clicking there again dismisses the panel and doesn't make it reappear. So to me this setting is not required.

Richard, could to try with and without the second patch to see whether there is any difference on Mac. I believe the test is to click onto the star while the panel is open.

Comment 20

[Richard Marti (:Paenglab)]

There is no difference with or without patch. Clicking a second time on the star closes the panel. Also clicking somewhere else closes it.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1454106.patch - add comment

How is this? The beauty is that the preprocessor takes the whole nonsense out :-)

Comment 22

[:aceman]

Comment on attachment 8968326 [details] [diff] [review]
1454106.patch - add comment

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

OK, at least we have a trace if something goes wrong in the future.
I would have added the attribute without disabling it, but we can try without it and see when the obscure problems appear.

::: mail/base/content/editContactPanel.inc
@@ +6,5 @@
>          type="arrow"
>          orient="vertical"
>          ignorekeys="true"
>          hidden="true"
> +#if 0

Make sure this can be 0 and not a boolean false.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5ca608b15314
add comment re. 'consumeoutsideclicks' attribute on the edit contact panel after removal of setConsumeRollupEvent(PopupBoxObject.ROLLUP_CONSUME). r=aceman