Closed Bug 1216161 Opened 4 years ago Closed 4 years ago

XUL Notifications on Linux can't be closed

Categories

(Toolkit :: Notifications and Alerts, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla44
Tracking Status
firefox43 --- unaffected
firefox44 - fixed

People

(Reporter: marco, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached image screenshot.png
The new style for notifications is really ugly.
Clicking on "X" doesn't close the notification.

This is a recent regression.
Duplicate of bug 1215840 / bug 1211635 caused by bug 1201397.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1215840
Bug 1215840 doesn't cover the notifications not closing.
Blocks: 1201397
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Component: Untriaged → Notifications and Alerts
Product: Firefox → Toolkit
Marco, is there anything in your Browser Console (not Web Console) when you try to close them?
Status: REOPENED → NEW
Flags: needinfo?(mar.castelluccio)
Keywords: regression
Summary: Notifications on Linux are ugly and can't be closed → XUL Notifications on Linux can't be closed
(In reply to Matthew N. [:MattN] from comment #3)
> Marco, is there anything in your Browser Console (not Web Console) when you
> try to close them?

Unfortunately not.
Flags: needinfo?(mar.castelluccio)
[Tracking Requested - why for this release]:
Flags: needinfo?(jaws)
Blocks: 1216585
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Comment on attachment 8676488 [details] [diff] [review]
Patch

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

::: browser/base/content/test/alerts/browser_notification_close.js
@@ +5,5 @@
> +var notificationURL = "http://example.org/browser/browser/base/content/test/alerts/file_dom_notifications.html";
> +var closedTime;
> +
> +function test () {
> +  waitForExplicitFinish();

Please use add_task. We can't keep adding tech-debt copying and pasting boilerplate and I already let it go twice. I would have maybe let it go again if you did a simple `hg copy` and modifications and/or using shared helpers in head.js but this is just copying logic making it harder to read and review.

@@ +11,5 @@
> +  let pm = Services.perms;
> +  registerCleanupFunction(function() {
> +    pm.remove(makeURI(notificationURL), "desktop-notification");
> +    gBrowser.removeTab(tab);
> +    window.restore();

Why window.restore?

::: toolkit/themes/linux/global/alerts/alert.css
@@ +27,5 @@
> +}
> +
> +#alertBox[animate][clicked] {
> +  animation-duration: .6s;
> +  animation-name: alert-clicked-animation;

Are there differences between platforms for all of this added stuff? If not, move it from all platforms to toolkit/themes/shared/alert-common.css
Attachment #8676488 - Flags: review?(MattN+bmo) → review-
Attached patch Patch (obsolete) — Splinter Review
Thanks for the quick review, I switched the test to add_task and moved the styles to alert-common.css.
Attachment #8676488 - Attachment is obsolete: true
Attachment #8676534 - Flags: review?(MattN+bmo)
Comment on attachment 8676534 [details] [diff] [review]
Patch

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

::: browser/base/content/test/alerts/browser_notification_close.js
@@ +1,5 @@
> +"use strict";
> +
> +var tab;
> +var notification;
> +var notificationURL = "http://example.org/browser/browser/base/content/test/alerts/file_dom_notifications.html";

Nit: s/var/let/

::: toolkit/themes/windows/global/alerts/alert.css
@@ -17,5 @@
>    color: -moz-DialogText;
>  }
>  
> -#alertBox[animate] {
> -  animation-timing-function: cubic-bezier(.12,1.23,.48,1.09);

You need to delete this stuff from the osx version too.
Attachment #8676534 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] from comment #9)
> Nit: s/var/let/

Can you give some background as to why you prefer `let` over `var` for a globally-scoped variable? Wasn't there a bug that removed `let` usage from global variables during test runs because they all share the same namespace? There is no advantage to `let` scoping here, so I don't see a downside to `var`.
Flags: needinfo?(MattN+bmo)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> (In reply to Matthew N. [:MattN] from comment #9)
> > Nit: s/var/let/
> 
> Can you give some background as to why you prefer `let` over `var` for a
> globally-scoped variable? Wasn't there a bug that removed `let` usage from
> global variables during test runs because they all share the same namespace?
> There is no advantage to `let` scoping here, so I don't see a downside to
> `var`.

The reason for this is because we always used `let` regardless of whether it made much difference from `var` just so you didn't have to think about it. IIUC, If it doesn't have a problem in this test I would rather continue to use `let` wherever possible. `let` is actually more strict related to things like hoisting which I think is good.
Flags: needinfo?(MattN+bmo)
The last Try push in this bug had a ton of orange. Is there a newer, greener run to look at?
Flags: needinfo?(jaws)
https://hg.mozilla.org/mozilla-central/rev/429ce6f92c32
Status: ASSIGNED → RESOLVED
Closed: 4 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Marco, can you please confirm that this issue is fixed as expected on a latest nightly build? Thanks!
Flags: needinfo?(mar.castelluccio)
While this is a valid regression which has been fixed for a few weeks now, I do not feel the need to track it for 44. Please re-nominate if this bug is reopened.
Yes, it is fixed!
Flags: needinfo?(mar.castelluccio)
(In reply to Marco Castelluccio [:marco] from comment #19)
> Yes, it is fixed!

Thanks! Updating status to "Verified".
Status: RESOLVED → VERIFIED
We're having trouble with this test under docker in bug 1244936 for e10s.
If anyone could have a look there it would be greatly appreciated.
You need to log in before you can comment on or make changes to this bug.