1220337 - Don't show alternate actions on OS X 10.8

Status: VERIFIED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Lina Butler [:lina]]

OS X 10.8 doesn't support alternate actions for notifications.

Comment 1

[Lina Butler [:lina]]

Attachment: 0001-Bug-1220337-Don-t-show-alternate-actions-on-OS-X-10..patch

Comment 2

[Matthew N. [:MattN]]

Can you explain what the user-facing benefit is? Is it that 10.8 respects _showsButtons but only shows "Close"?

The other question is whether we really want to leave 10.8 users without these actions or if we should just stop supporting native notifications on 10.8 and use XUL which will have these options?

Comment 3

[Lina Butler [:lina]]

(In reply to Matthew N. [:MattN] from comment #2)
> Can you explain what the user-facing benefit is? Is it that 10.8 respects
> _showsButtons but only shows "Close"?

10.8 doesn't support `_showsButtons` or alternate actions at all; you can only click on the notification to trigger the default action. It doesn't look like you can use any of the other private APIs, either.

I think the notification center integration is still helpful, but it's your call if you'd like to use XUL notifications.

Comment 4

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1220337 - Don't show alternate actions on OS X 10.8. r?MattN

Bug 1220337 - Don't show alternate actions on OS X 10.8. r?MattN

Comment 5

[Matthew N. [:MattN]]

(In reply to Kit Cambridge [:kitcambridge] from comment #3)
> (In reply to Matthew N. [:MattN] from comment #2)
> > Can you explain what the user-facing benefit is? Is it that 10.8 respects
> > _showsButtons but only shows "Close"?
> 
> 10.8 doesn't support `_showsButtons` or alternate actions at all; you can
> only click on the notification to trigger the default action. It doesn't
> look like you can use any of the other private APIs, either.

Are you saying there is no user facing benefit of this patch then?

> I think the notification center integration is still helpful, but it's your
> call if you'd like to use XUL notifications.

I think that's up to product. Do you know if 10.9 supports the actions?

Comment 6

[Lina Butler [:lina]]

(In reply to Matthew N. [:MattN] from comment #5)
> Are you saying there is no user facing benefit of this patch then?

I guess the user-facing benefit is we use the notification center on 10.8, instead of falling back to XUL. Also, it looks like system notifications (like the new update alert) still use Cocoa notifications. But clicking on one crashes Firefox, because 10.8 doesn't support `_alternateActionIndex`.

> I think that's up to product. Do you know if 10.9 supports the actions?

It does, yes.

Comment 7

[Matthew N. [:MattN]]

(In reply to Kit Cambridge [:kitcambridge] from comment #6)
> (In reply to Matthew N. [:MattN] from comment #5)
> > Are you saying there is no user facing benefit of this patch then?
> 
> I guess the user-facing benefit is we use the notification center on 10.8,
> instead of falling back to XUL. Also, it looks like system notifications
> (like the new update alert) still use Cocoa notifications. But clicking on
> one crashes Firefox, because 10.8 doesn't support `_alternateActionIndex`.

I meant the user-facing benefit of your patch as-is. It seems like the user-facing benefit is not crashing but it sounds like that wasn't the original reason for this patch?

Comment 8

[Lina Butler [:lina]]

(In reply to Matthew N. [:MattN] from comment #7)
> I meant the user-facing benefit of your patch as-is. It seems like the
> user-facing benefit is not crashing but it sounds like that wasn't the
> original reason for this patch?

Ah, sorry. The original reason for the patch was using Cocoa notifications for both web and system notifications on 10.8. I noticed we're still using Cocoa for the latter, but bug 1219835 indicates web notifications use XUL.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8681568 [details]
MozReview Request: Bug 1220337 - Don't show alternate actions on OS X 10.8. r?MattN

https://reviewboard.mozilla.org/r/23863/#review21635

We may be forced to stop supporting native notifications on 10.8 if we can't get the functionality we need.

::: widget/cocoa/OSXNotificationCenter.mm:288
(Diff revision 1)
>        notification.hasActionButton = YES;
>        notification.otherButtonTitle = nsCocoaUtils::ToNSString(closeButtonTitle);
>        notification.actionButtonTitle = nsCocoaUtils::ToNSString(actionButtonTitle);
> +
> +      // OS X 10.8 doesn't support action buttons.
> +      if ([notification respondsToSelector:@selector(set_showsButtons:)] &&

I think the three lines above should probably be within the `if` block depending on what you find when you switch to the `alert` style.

Comment 10

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/fx-team/rev/6d77cb7457881637d99755e8343b86e64ea64871
Bug 1220337 - Don't show alternate notification actions on OS X 10.8. r=MattN

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d77cb745788

Comment 13

[Lina Butler [:lina]]

Attachment: 10.8.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 	1205399.
[User impact if declined]: Two issues: 1. Users on OS X 10.8 will see XUL alerts for web notifications, and native alerts for Firefox notifications (e.g., new updates or Hello room status changes). 2. Clicking on a native alert will crash Firefox.
[Describe test coverage new/current, TreeHerder]: No automated tests for native notifications. Manually verified on 10.8.5.
[Risks and why]: Medium risk due to lack of tests. Requesting uplift because users are likely to click on both types of notifications, and we shouldn't show broken XUL alerts or crash if that happens.
[String/UUID change made/needed]: None.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Kit, Matt: Is this a recent regression? Given that we do not have good automated test coverage for this, I would prefer letting this one ride the trains. 

If during the remainder of 44 (aurora/beta) cycle we notice a crash spike and are able to root cause it to this bug, we can then consider uplifting it to Aurora/Beta as needed. This is just due diligence on my part to ensure only high-end-user-impact bug fixes are considered and ultimately ensuring we ship at high quality. Please let me know.

Comment 15

[Matthew N. [:MattN]]

(In reply to Ritu Kothari (:ritu) from comment #14)
> Kit, Matt: Is this a recent regression? Given that we do not have good
> automated test coverage for this, I would prefer letting this one ride the
> trains. 

Beta is unaffected. This is a straightforward crash fix for 44 that I don't think we can ship without as we are guaranteed to crash upon clicking the notification on affected versions.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Matthew N. [:MattN] from comment #15)
> (In reply to Ritu Kothari (:ritu) from comment #14)
> > Kit, Matt: Is this a recent regression? Given that we do not have good
> > automated test coverage for this, I would prefer letting this one ride the
> > trains. 
> 
> Beta is unaffected. This is a straightforward crash fix for 44 that I don't
> think we can ship without as we are guaranteed to crash upon clicking the
> notification on affected versions.

Ok. I will request QE team to verify the fix and given that this is a bit early in the Aurora cycle, we could take the fix and still have time to deal with regressions, if any.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8686138 [details] [diff] [review]
10.8.patch

This fix has been in Nightly for a few days so should be safe to uplift to Aurora44.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Bogdan, since this is an issue that can be tested using Hello conversations, would you be able to verify the fix and make sure there are no expected fall outs? Thanks!

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm hitting some conflicts uplifting this. Can we get a rebased patch for aurora?

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

User error on my part, disregard.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef1e3a6411d7

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ef1e3a6411d7

Comment 23

[Bogdan Maris, Desktop QA]

(In reply to Ritu Kothari (:ritu) from comment #18)
> Bogdan, since this is an issue that can be tested using Hello conversations,
> would you be able to verify the fix and make sure there are no expected fall
> outs? Thanks!

Sure Ritu!
I reproduced the initial issue using old Nightly (2015-10-24), each time I clicked on the notification (Hello, Update), Firefox crashed. I verified that the crash did not occur anymore (using Hello and Update) and the notification did not contain any more actions using demos[1] on latest Firefox Developer Edition 44.0a2 and latest Nightly 45.0a1 using Mac OS X 10.8.5. 

[1] - https://people.mozilla.org/~mnoorenberghe/w3c_notifications.htm
http://mdn.github.io/to-do-notifications/
http://mdn.github.io/emogotchi/
https://web.skype.com/