1547238 - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-attachment-reminder.js | test-attachment-reminder.js::test_disabling_attachment_reminder

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-attachment-reminder.js | test-attachment-reminder.js::test_disabling_attachment_reminder

https://taskcluster-artifacts.net/GaN3EcB9ShmzoUglKixE_w/0/public/logs/live_backing.log

INFO - EXCEPTION: Couldn't find the requested button on a notification
INFO - at: test-notificationbox-helpers.js line 186
INFO - get_notification_button test-notificationbox-helpers.js:186 9
INFO - test_disabling_attachment_reminder test-attachment-reminder.js:623 23

https://hg.mozilla.org/comm-central/pushloghtml?fromchange=2458408cc87e&tochange=dc7943c0ecc9

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7e40e33da3da2640e965a153254594a234&tochange=7d47e7fa2489550ffa83aae67715c54970

Maybe this one:
936c2d201405 Andrew Swan — Bug 1538983 Convert button xbl binding to a custom element r=bgrins

Comment 1

[Magnus Melin [:mkmelin]]

make -C obj-x86_64-pc-linux-gnu mozmill-one SOLO_TEST=composition/test-attachment-reminder.js

... reproduces it locally gives me

SUMMARY-UNEXPECTED-FAIL | test-attachment-reminder.js | test-attachment-reminder.js::test_disabling_attachment_reminder
EXCEPTION: Couldn't find the requested button on a notification
at: test-notificationbox-helpers.js line 186
get_notification_button test-notificationbox-helpers.js:186 9
test_disabling_attachment_reminder test-attachment-reminder.js:623 23

Yes may have to adjust something for bug 1538983.
I don't see any duplicated label. Will leave this for Alex.

Comment 2

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/4917

Comment 3

[:aceman]

I see a duplicated label when showing the attachment reminder manually.

Comment 4

[Alessandro Castellani [:aleca]]

I'm on it, will upload a patch soon.

Comment 5

[Alessandro Castellani [:aleca]]

That bug 1538983 patch is causing some issues elsewhere as well: https://bugzilla.mozilla.org/show_bug.cgi?id=1547322

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-04-26 10-38-34.png

Not sure if related, but the buttons and sizing in the confirmation dialog are busted.

Comment 7

[Alessandro Castellani [:aleca]]

So, the issue with the attachment reminder button happens when setting the button's attribute type to menu-button.
As soon as that attribute is set, an extra hbox with the button label gets generated.

Since bug 1538983 caused a bunch of regressions that are getting reported, is it worth trying to patch this on our end, or it's better to wait for the Firefox devs to fix this?

Also, the toolbarbuttons with type="menu" are visually busted.
Is that related?

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to Alessandro Castellani (:aleca) from comment #7)

Since bug 1538983 caused a bunch of regressions that are getting reported, is it worth trying to patch this on our end, or it's better to wait for the Firefox devs to fix this?

Jolly good question. Sometimes the TB team serves as Mozilla's secondary fire brigade and fixes things in M-C. But maybe we should wait a bit. In the meantime, I'll take the test down ;-)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fb9349e31a40
temporarily disable test-attachment-reminder.js::test_disabling_attachment_reminder. rs=bustage-fix

Comment 10

[Magnus Melin [:mkmelin]]

(In reply to Alessandro Castellani (:aleca) from comment #7)

So, the issue with the attachment reminder button happens when setting the
button's attribute type to menu-button.
As soon as that attribute is set, an extra hbox with the button label gets
generated.

Since bug 1538983 caused a bunch of regressions that are getting reported,
is it worth trying to patch this on our end, or it's better to wait for the
Firefox devs to fix this?

Let's wait some days to let them iron out the regressions. It's possible bug 1546352 could help for menu-button which is tb specific.

Also, the toolbarbuttons with type="menu" are visually busted.
Is that related?

That can be worth reporting. Is is used anywhere in Firefox?

Comment 11

[Richard Marti (:Paenglab)]

(In reply to Alessandro Castellani (:aleca) from comment #7)

Also, the toolbarbuttons with type="menu" are visually busted.
Is that related?

Like the "Tag" button? Is bug 1546630 applied?

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: Doubled-up notification text.png

So what's responsible for this? I started a message and typed "CV" which is one of the pre-canned words that trigger the reminder.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Alessandro Castellani (:aleca) from comment #6)

Not sure if related, but the buttons and sizing in the confirmation dialog are busted.

Yes, the "Discard" button is missing its label. That's generated here:
https://searchfox.org/comm-central/rev/c1fa40fa8e939b73cebcd10cd5de3351f2e35841/mail/components/compose/content/MsgComposeCommands.js#4129

https://searchfox.org/comm-central/search?q=discardButtonLabel&redirect=false

Comment 14

[:aceman]

(In reply to Jorg K (GMT+2) from comment #13)

(In reply to Alessandro Castellani (:aleca) from comment #6)

Not sure if related, but the buttons and sizing in the confirmation dialog are busted.

Yes, the "Discard" button is missing its label. That's generated here:

I also see it, filed as bug 1547377.

Comment 15

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #12)

Created attachment 9061100 [details]
Doubled-up notification text.png

So what's responsible for this? I started a message and typed "CV" which is one of the pre-canned words that trigger the reminder.

Could it be that it's now a mixed CE- and XBL-button?

Comment 16

[:aceman]

Is it?
In case you embed a CE element inside a XBL defined element/binding, you have to call customElements.upgrade() on the internal Custom element.

Comment 17

[Jorg K (CEST = GMT+2)]

OK, this just landed on M-C:
013a4420b53c Andrew Swan — Bug 1547322 Don't double-insert inner content when <button> is cloned r=bgrins
776c731d7c71 Brian Grinstead — Bug 1547346 - Upgrade custom elements in dialog anonymous XBL content before accessing them r=aswan

I'll check whether the test passes now and whether the duplicate text has disappeared. The missing button label (bug 1547377) is also fixed.

Comment 18

[Jorg K (CEST = GMT+2)]

Well, the test still fails and the label is still doubled up, see screenshot in comment #12. One the positive side I can confirm that he "Discard changes" button label (bug 1547377) is fixed.

Comment 19

[Alessandro Castellani [:aleca]]

Thanks for looking into this.
I'll test it later and see if the fix implemented for Bug 1547346 could work for us:

for (let button in buttons) {
    customElements.upgrade(buttons[button]);
}

Comment 20

[Alessandro Castellani [:aleca]]

Attachment: attachment-reminder.patch

I've been testing and trying to figure out what's happening here with not much luck.
Since the MozElements.NotificationBox() class doesn't handle buttons with type="menu-button", and if I'm not wrong that type is only used by TB, the button hbox that contains the label gets duplicated when that specific type gets applied.

It also happens when you manually add that attribute through the browser inspector. I think something is happening in bug 1538983 that causes the button to regenerate the label box.

I found a super hacky patch that simply deletes the label box. I'm not sure this is an acceptable approach, tho, but it fixes the test and remove the extra text.

I'm thinking we should consider using the toolbarbutton element and append it to the notification reminder, since this is the only location we're using the type="menu-button".

Thoughts?

Comment 21

[Magnus Melin [:mkmelin]]

It's used in a few other places too. I think bug 1546352 will fix it.

Comment 22

[Alessandro Castellani [:aleca]]

(In reply to Magnus Melin [:mkmelin] from comment #21)

It's used in a few other places too. I think bug 1546352 will fix it.

Ah, nice. So my hacky patch is useless :D

Comment 23

[Alessandro Castellani [:aleca]]

Comment on attachment 9061430 [details] [diff] [review]
attachment-reminder.patch

Cancelling this review as bug 1547238 will address this.

Comment 24

[Magnus Melin [:mkmelin]]

Attachment: bug1547238_attachmenht_reminder_tests.patch

The test changes needed once bug 1546352 lands.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9064740 [details] [diff] [review]
bug1547238_attachmenht_reminder_tests.patch

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

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

Comment 26

[Jorg K (CEST = GMT+2)]

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

Comment 27

[:aceman]

Comment on attachment 9064740 [details] [diff] [review]
bug1547238_attachmenht_reminder_tests.patch

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

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +130,5 @@
>    // The manual reminder should be disabled yet.
>    assert_manual_reminder_state(cwc, false);
>  
>    // Click ok to be notified on send if no attachments are attached.
> +  cwc.click(new elementslib.Elem(cwc.e(kBoxId).querySelector(`button[label="Remind Me Later"]`)));

Why does the old method no longer work?

@@ +621,5 @@
>    // Disable the reminder (not just dismiss) using the menuitem
>    // in the notification bar menu-button.
>    let disableButton = get_notification_button(cwc, kBoxId, kNotificationId,
>                                                { popup: "reminderBarPopup" });
> +  cwc.click(new elementslib.Elem(disableButton.querySelector("dropmarker")));

Why not fix get_menu_dropmarker() instead? I don't like poking in internal structures of standard widgets (whether XBL or CE) from random places in tests.

::: mail/test/mozmill/shared-modules/test-notificationbox-helpers.js
@@ +162,5 @@
>        if (name == "popup") {
>          if (button.getAttribute("type") == "menu-button" ||
>              button.getAttribute("type") == "menu") {
>            // The button contains a menupopup as the first child.
> +          matched = button.querySelector("menupopup#" + value);

Why is this better? Does this also check for the firstChild only?

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 9064740 [details] [diff] [review]
bug1547238_attachmenht_reminder_tests.patch

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

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +130,5 @@
>    // The manual reminder should be disabled yet.
>    assert_manual_reminder_state(cwc, false);
>  
>    // Click ok to be notified on send if no attachments are attached.
> +  cwc.click(new elementslib.Elem(cwc.e(kBoxId).querySelector(`button[label="Remind Me Later"]`)));

It's actually possible the old one works. (May be leftover before I fixed a bug.)
But querySelector is way more readable, and standard technology.

@@ +621,5 @@
>    // Disable the reminder (not just dismiss) using the menuitem
>    // in the notification bar menu-button.
>    let disableButton = get_notification_button(cwc, kBoxId, kNotificationId,
>                                                { popup: "reminderBarPopup" });
> +  cwc.click(new elementslib.Elem(disableButton.querySelector("dropmarker")));

Those internal structures you can easily see in the inspector so it's clear what to do. Finding out about a special method is not, nor debugging what problems it may or may not have.

::: mail/test/mozmill/shared-modules/test-notificationbox-helpers.js
@@ +162,5 @@
>        if (name == "popup") {
>          if (button.getAttribute("type") == "menu-button" ||
>              button.getAttribute("type") == "menu") {
>            // The button contains a menupopup as the first child.
> +          matched = button.querySelector("menupopup#" + value);

this is matching on id (the value is the id), so it will match the correct thing. 
It's not explicitly checking firstChild (why should it?) (and it's not the firstChild anymore since the internals are not anonymous now

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 9064740 [details] [diff] [review]
bug1547238_attachmenht_reminder_tests.patch

Let's take it then. Why the these quotes, nothing evaluated here:
```
.querySelector(`button[label="Remind Me Later"]`))
```

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/62160a65ce2a
adjust test-attachment-reminder.js to changes related to button de-xbl. r=jorgk

Comment 31

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #29)

Why the these quotes, nothing evaluated here:

Just so I wouldn't have to escape the normal quotes in there.