1717127 - Fix wrong implementation of the notification element for public bulk mail notification

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect, P3)

Description

[Alessandro Castellani [:aleca]]

The notification for public bulk emails works pretty well, but the implementation has some small mistakes that should be fixed.

1. The message text is added by creating a new div element and appending it inside the notification: https://searchfox.org/comm-central/rev/014b72575f40cf1d2e0cc1d997d8e4bebbd118f9/mail/components/compose/content/MsgComposeCommands.js#5145-5151

This is not necessary since notifications allow adding and updating text pretty easily without creating any extra markup. Also, using a div inside a label element is semantically wrong.

2. We're also adding an ID to the newly created notification: https://searchfox.org/comm-central/rev/014b72575f40cf1d2e0cc1d997d8e4bebbd118f9/mail/components/compose/content/MsgComposeCommands.js#5194

This is incorrect since the notification can be identified by its unique value "warnPublicRecipientsNotification", with the native method getNotificationWithValue().

This is not a severe bug but it's rather a task to clean up the implementation and use a consistent coding style and native selectors of the MozElement.Notification element.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: Bug 1717127 - Fix wrong implementation of the notification element for public bulk mail notification. r=mkmelin

Comment 2

[Alessandro Castellani [:aleca]]

Fixed the bulk notification implementation.
I also updated the tests with the following changes:

• Use more BrowserTestUtils.waitForCondition() when possible.
• Simulate clicks to show Cc field rather than calling the showAddressRow() directly.
• Assert the mailing list amount of detected pills by matching the count in the fluent string. (fixes test failure introduced in bug 1717085)

Try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=c84f687d7ab5466d0ab8ff3d19f45aef2ce9fb2e

Comment 3

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani [:aleca] from comment #0)

1. The message text is added by creating a new div element and appending it inside the notification: https://searchfox.org/comm-central/rev/014b72575f40cf1d2e0cc1d997d8e4bebbd118f9/mail/components/compose/content/MsgComposeCommands.js#5145-5151
   This is not necessary since notifications allow adding and updating text pretty easily without creating any extra markup. Also, using a div inside a label element is semantically wrong.

Yeah, I had seen that too, but decided to ignore ;-) I wasn't entirely sure if perhaps there was some sort of layout reason for the clumsy implementation.

Nice cleanup on top of my bug 1717085, this is in much better shape now.
Thanks for fixing the test failure.

Comment 4

[Alessandro Castellani [:aleca]]

The tests should be good now: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=15bc25ac2b94a51d2f8a7c22759628040495da02

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f561cef02397
Fix wrong implementation of the notification element for public bulk mail notification. r=mkmelin