Don't show alternate actions on OS X 10.8

VERIFIED FIXED in Firefox 44, Firefox OS v2.5

Status

()

VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox43 unaffected, firefox44 verified, firefox45 verified, b2g-v2.5 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
OS X 10.8 doesn't support alternate actions for notifications.
(Assignee)

Comment 1

3 years ago
Created attachment 8681565 [details] [diff] [review]
0001-Bug-1220337-Don-t-show-alternate-actions-on-OS-X-10..patch
Attachment #8681565 - Flags: review?(MattN+bmo)
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?
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Comment 4

3 years ago
Created attachment 8681568 [details]
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
Attachment #8681568 - Flags: review?(MattN+bmo)
(Assignee)

Updated

3 years ago
Attachment #8681565 - Flags: review?(MattN+bmo)
(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?
(Assignee)

Comment 6

3 years ago
(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.
(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?
(Assignee)

Comment 8

3 years ago
(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 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.
Attachment #8681568 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/6d77cb7457881637d99755e8343b86e64ea64871
Bug 1220337 - Don't show alternate notification actions on OS X 10.8. r=MattN
Duplicate of this bug: 1219835

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d77cb745788
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Comment 13

3 years ago
Created attachment 8686138 [details] [diff] [review]
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.
Attachment #8681568 - Attachment is obsolete: true
Attachment #8686138 - Flags: approval-mozilla-aurora?
status-firefox43: --- → unaffected
status-firefox44: --- → affected
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.
Flags: needinfo?(kcambridge)
Flags: needinfo?(MattN+bmo)
(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.
Flags: needinfo?(kcambridge)
Flags: needinfo?(MattN+bmo)
(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 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.
Attachment #8686138 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(bogdan.maris)
I'm hitting some conflicts uplifting this. Can we get a rebased patch for aurora?
Flags: needinfo?(kcambridge)
User error on my part, disregard.
Flags: needinfo?(kcambridge)

Comment 21

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/ef1e3a6411d7
status-firefox44: affected → fixed
(Assignee)

Updated

3 years ago
Blocks: 1224738
(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/
Status: RESOLVED → VERIFIED
status-firefox44: fixed → verified
status-firefox45: fixed → verified
Flags: needinfo?(bogdan.maris)
You need to log in before you can comment on or make changes to this bug.