Closed Bug 1225563 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

41 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: azrdev, Unassigned, Mentored)

Details

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

Attachments

(1 file, 2 obsolete files)

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.
Mentor: margaret.leibovic
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [lang=js][good first bug]
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 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+
Attachment #8719689 - Attachment is obsolete: true
Assignee: nobody → daniel9618
Status: NEW → ASSIGNED
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+
Keywords: checkin-needed
Attachment #8720973 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e38a16b7d37e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.