Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: azrdev, Unassigned, Mentored)

Tracking

41 Branch
Firefox 47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151103081951

Steps to reproduce:

In an addon, call NativeWindow.doorhanger.show("label", "const id", [button1, button2], ...); where the buttons are objects with `label` and `callback` members, but *not* a `positive:true`.
As per <https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/NativeWindow/doorhanger#show%28%29>, only one of the Buttons is shown.


Actual results:

There is no indication why the button(s) have been dropped from display.


Expected results:

A log message would be useful.

Updated

3 years ago
Mentor: margaret.leibovic
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [lang=js][good first bug]

Updated

3 years ago
Mentor: liuche
We do log an error, but it goes to Logcat in Android. Since this mainly affects Addon authors, this notification should be in JS.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DoorHangerPopup.java#131
Summary: NativeWindow.doorhanger.show() with multiple buttons silently discards all but one → Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided
Updating these links:

In the Java code, we log an error if there are more than two buttons.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/DoorHangerPopup.java#133

We should do this same check in JS when sending the message to create doorhanger buttons, and log an error if there are more than two buttons.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2300

Comment 3

3 years ago
Created attachment 8719689 [details] [diff] [review]
Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided
Comment on attachment 8719689 [details] [diff] [review]
Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided

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

One nit, but otherwise looks good!  If you'll post an updated patch, we'll mark it checkin-needed.  (That's a Bugzilla keyword; I'm not sure you have the rights to set it just yet.)

::: mobile/android/chrome/content/browser.js
@@ +2288,5 @@
>        if (aButtons == null) {
>          aButtons = [];
>        }
>  
> +      if(aButtons.length > 2) {

nit: space between if and (.
Attachment #8719689 - Flags: review+

Comment 5

3 years ago
Created attachment 8720973 [details] [diff] [review]
Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided

Updated

3 years ago
Attachment #8719689 - Attachment is obsolete: true

Comment 6

3 years ago
Created attachment 8722662 [details] [diff] [review]
Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided
Attachment #8722662 - Flags: review?(nalexander)

Updated

3 years ago
Assignee: nobody → daniel9618
Status: NEW → ASSIGNED

Updated

3 years ago
Assignee: daniel9618 → nobody
Status: ASSIGNED → NEW
Comment on attachment 8722662 [details] [diff] [review]
Log error in JS that Doorhangers only support one positive and one negative button if invalid buttons are provided

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

Looks good.
Attachment #8722662 - Flags: review?(nalexander) → review+

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Attachment #8720973 - Attachment is obsolete: true

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e38a16b7d37e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.