Closed Bug 1147064 Opened 5 years ago Closed 5 years ago

Update doorhanger button styling

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- verified
fennec 41+ ---

People

(Reporter: liuche, Assigned: liuche)

References

Details

Attachments

(9 files, 1 obsolete file)

96.80 KB, image/png
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
157.73 KB, image/png
Details
See https://bug1088220.bugzilla.mozilla.org/attachment.cgi?id=8566119

right button (affirmatives): link_blue background, white text
left button (dismissal): toolbar_menu_grey, black text
This depends on the login doorhangers bug, but that should land very soon.

This could also (optionally) include a refactor to redo doorhanger buttons, because the click handler is currently in DoorHangerPopup instead of within each Doorhanger type.
Assignee: nobody → ally
Depends on: 1088220
Leaving a note here: font should be Roboto regular 16 sp, should not be all caps either
(In reply to Anthony Lam (:antlam) from comment #2)
> Leaving a note here: font should be Roboto regular 16 sp, should not be all
> caps either

can we make that into an additional bug? That doesn't really relate to layout. You can assign it to me
Flags: needinfo?(alam)
I think it can be the same bug, it's touching the same code, and adding stuff to styles. Fewer review requests for me :)
Flags: needinfo?(alam)
:liuche has noted that refactoring so there is a nice little api for addDismissButton() & addConfirmButton() would be nifty in her book.
Summary: Update doorhanger button colors → Update doorhanger button styling
tracking-fennec: --- → ?
tracking-fennec: ? → 39+
found it. even works on evil gingerbread.
Assignee: ally → liuche
Right now, the buttons in the doorhangers are a little inconsistent in which button (affirm/dismiss) is on the right. For the most part though, it seems like the "suggested" button is on the right.

Anthony, you mentioned that the right button (affirmative) should be blue - are we concerned that we are emphasizing which button to click by highlighting it?

For example, consider popups, where the affirmative, highlighted button is "Show" and the negative (that people usually want) is "Don't show". Are we concerned that we might be emphasizing or hinting at what the user should click based on color? or is the goal to simply make the coloring consistent with affirm/dismiss?

It's also a little interesting because the text of the buttons can be shaped to give a positive/negative bent to the meaning.

...or is your plan to cover this in a different bug?
Flags: needinfo?(alam)
(In reply to Chenxia Liu [:liuche] from comment #8)
> or is the goal to simply make the coloring consistent
> with affirm/dismiss?

This! :) The goal is to be consistent within ourselves. Desktop side has plans to do the same (last I talked to Shorlander) and so it was half a consistency-play as well.
Flags: needinfo?(alam)
Forgot to update this from my discussions with antlam a while back, this should be tracking 40. We won't be chasing aurora with this.
tracking-fennec: 39+ → ?
This should track 41, along with the rest of the "Doorhanger content styling and layout" work.
Depends on: 1126608
tracking-fennec: ? → 41+
Attached file MozReview Request: bz://1147064/liuche (obsolete) —
/r/8867 - Bug 1147064 - Abstract out shared doorhanger layout. r=margaret
/r/8869 - Bug 1147064 - Remove extra layer of layout for overdraw. r=margaret
/r/8871 - Bug 1147064 - Use margins for spacing instead of padding for site identity. r=margaret
/r/8873 - Bug 1147064 - Make negative/positive button order consistent. r=margaret

Pull down these commits:

hg pull -r 1fcbe2f9a6b1cb8504a082378141daf94f76d9ac https://reviewboard-hg.mozilla.org/gecko/
Comment #12 is WIP for button styling, a few more parts to come.
Mostly done! But needs some more work for click handlers and button selectors.

Update on button colors:
Right (affirmative):
- unpressed = link_blue
- pressed = link_blue_pressed = #0082C9

Left (negative):
- unpressed = toolbar_menu_dark_grey
- pressed = toolbar_grey_pressed
Comment on attachment 8607235 [details]
MozReview Request: bz://1147064/liuche

/r/8867 - Bug 1147064 - Abstract out shared doorhanger layout. r=margaret
/r/8869 - Bug 1147064 - Remove extra layer of layout for overdraw. r=margaret
/r/8871 - Bug 1147064 - Use margins for spacing instead of padding for site identity. r=margaret
/r/8873 - Bug 1147064 - Make negative/positive button order consistent. r=margaret
/r/9233 - Bug 1147064 - Add positive/negative to buttons. r=margaret
/r/9235 - Bug 1147064 - Add colored buttons. r=margaret

Pull down these commits:

hg pull -r 107b6d18b4cacb0d4e0aa5b63bac3dea01b389f9 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8607235 [details]
MozReview Request: bz://1147064/liuche

/r/8867 - Bug 1147064 - Abstract out shared doorhanger layout. r=margaret
/r/8869 - Bug 1147064 - Remove extra layer of layout for overdraw. r=margaret
/r/8871 - Bug 1147064 - Make negative/positive button order consistent. r=margaret
/r/8873 - Bug 1147064 - Add positive/negative to buttons. r=margaret
/r/9233 - Bug 1147064 - Add colored buttons. r=margaret
/r/9235 - Bug 1147064 - Handle callback ids for Login doorhanger dialogs. r=margaret
/r/9315 - Bug 1147064 - Clean up misc styling from feedback. r=margaret

Pull down these commits:

hg pull -r aa1ac98cff6a3e30fafef3d6acc3dbe836c66134 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8607235 - Flags: review?(margaret.leibovic)
Ugh, finally got this out the door. This definitely cleans a lot of the button handling code by moving them to the shared doorhanger instead of in each individual doorhanger.
https://reviewboard.mozilla.org/r/8867/#review8105

::: mobile/android/base/resources/layout/default_doorhanger.xml:10
(Diff revision 3)
> +                  android:orientation="vertical">

If this content is all going to go inside another LinearLayout, do we need this one to wrap the content here as well?

::: mobile/android/base/resources/layout/doorhanger.xml:20
(Diff revision 3)
> -        <TextView android:id="@+id/doorhanger_message"
> +        <LinearLayout android:id="@+id/content"

Or maybe instead of a LinearLayout this could be a ViewStub?

::: mobile/android/base/widget/DoorHanger.java:63
(Diff revision 3)
> -    private final TextView mMessage;
> +    private final LinearLayout mContent;

Doesn't look like this needs to be a member variable, since it's only used in the constructor.
https://reviewboard.mozilla.org/r/8869/#review8107

Haha, this addressed my comments from the last patch!
https://reviewboard.mozilla.org/r/8871/#review8111

I assume you got approval from antlam for these changes.

It would also be good to update the doorhanger API docs on MDN to note this pattern for negative/positive buttons:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/NativeWindow/doorhanger

Looks like this documentation could use some love in general...
https://reviewboard.mozilla.org/r/8873/#review8113

::: mobile/android/chrome/content/PluginHelper.js:39
(Diff revision 3)
> +        positive: false

Can we just default to not positive, and omit this property from the negative buttons?

You should also include this new value in the MDN docs.
https://reviewboard.mozilla.org/r/9233/#review8115

::: mobile/android/base/widget/DoorhangerConfig.java:89
(Diff revision 2)
> -    /**
> +    public void setButton(String label, int callbackId, boolean isPositive) {

Maybe this would be better as 2 methods, `setPostiveButton` and `setNegativeButton`, since I don't like boolean flags :)

::: mobile/android/base/widget/LoginDoorHanger.java:42
(Diff revision 2)
> +    // TODO: Fix callback for pos/neg button.

Can you elaborate on what you plan to do to address this TODO? Is there a follow-up bug?

::: mobile/android/base/DoorHangerPopup.java:120
(Diff revision 2)
> +        for (int i = 0; i < buttonArray.length(); i++) {

If we are only going to support 2 buttons, we should throw an exception here if there are more than 2 buttons supplied.

::: mobile/android/base/widget/DoorHanger.java
(Diff revision 2)
> -    private void addButtonToLayout(String text, int id) {

By removing this generic logic in favor of 2 hard-coded buttons, we remove support for more than 2 buttons. We used to have some doorhangers that had 3 buttons, but I think those went away when added support for a checkbox. Did you audit all the doorhanger to make sure none had more than buttons?

I assume a single button still works fine, since there's logic above for creating single buttons in the SiteIdentityPopup.

This is also an API that we expose to add-ons, so we should make sure we don't break any existings add-ons. It would be good to do a quick search through the add-ons MXR to see if any add-ons are trying to create more buttons, but we should definitely update MDN and perhaps also make post to mobile-firefox-dev about these API changes.
https://reviewboard.mozilla.org/r/9235/#review8117

::: mobile/android/base/widget/LoginDoorHanger.java:218
(Diff revision 2)
> -                                    response.put("callback", SiteIdentityPopup.ButtonType.COPY.ordinal());
> +                                    response.put("callback", mCallbackID);

Can you explain this change? How was this working properly before?
https://reviewboard.mozilla.org/r/8867/#review8259

::: mobile/android/base/widget/DoorHanger.java:63
(Diff revision 3)
> -    private final TextView mMessage;
> +    private final LinearLayout mContent;

Good call, I also changed this to a ViewStub (wrt your earlier comment), which simplifies this code - we don't have to inflate the layout separately. I didn't realize that ViewStub has a setLayoutResource() method and thought it had to be set in xml, so this is an improvement!
https://reviewboard.mozilla.org/r/9235/#review8263

::: mobile/android/base/widget/LoginDoorHanger.java:218
(Diff revision 2)
> -                                    response.put("callback", SiteIdentityPopup.ButtonType.COPY.ordinal());
> +                                    response.put("callback", mCallbackID);

Before, we weren't able to differentiate between the positive callback and the negative callback, so we had a hack (which I removed in the last patch -- see "HACK").
This code used to override createButtonInstance in LoginDoorhangers so that it saved the callback id of the second button added as mCallbackID, based on the assumption that the second button added was the "positive" button.
Now that we can get the positive/negative button configs, we can just set the callback in loadConfig.

This specific change is just to make this code more consistent, and use the callback id for the positive callback instead of hard-coding the callback id.
https://reviewboard.mozilla.org/r/8873/#review8273

> Can we just default to not positive, and omit this property from the negative buttons?
> 
> You should also include this new value in the MDN docs.

Updated this to default to negative. I'll update the MDN docs before landing.
https://reviewboard.mozilla.org/r/9233/#review8275

> Can you elaborate on what you plan to do to address this TODO? Is there a follow-up bug?

Fixed in the next commit.

> Maybe this would be better as 2 methods, `setPostiveButton` and `setNegativeButton`, since I don't like boolean flags :)

I thought about doing it this way, but that means that when creating buttons from JSON, the caller has to check the boolean and then decide which method to call - I think it's better to have the config handle that and throw errors if necessary, but if you feel strongly, I can change it.

> If we are only going to support 2 buttons, we should throw an exception here if there are more than 2 buttons supplied.

See comment below on throwing vs logging.

> By removing this generic logic in favor of 2 hard-coded buttons, we remove support for more than 2 buttons. We used to have some doorhangers that had 3 buttons, but I think those went away when added support for a checkbox. Did you audit all the doorhanger to make sure none had more than buttons?
> 
> I assume a single button still works fine, since there's logic above for creating single buttons in the SiteIdentityPopup.
> 
> This is also an API that we expose to add-ons, so we should make sure we don't break any existings add-ons. It would be good to do a quick search through the add-ons MXR to see if any add-ons are trying to create more buttons, but we should definitely update MDN and perhaps also make post to mobile-firefox-dev about these API changes.

I went through all the addons that call NativeWindow.doorhanger.show, and the only one that (might) have more than two buttons is https://mxr.mozilla.org/addons/source/456016/bootstrap.js#197 .

I'm inclined to Log an error (rather than throw an uncaught exception) and only add the first two buttons so we fail gracefully. I'm also updating the MDN docs.
Bug 1147064 - Abstract out shared doorhanger layout. r=margaret
Attachment #8611604 - Flags: review?(margaret.leibovic)
Bug 1147064 - Remove extra layer of layout for overdraw. r=margaret
Attachment #8611605 - Flags: review?(margaret.leibovic)
Bug 1147064 - Make negative/positive button order consistent. r=margaret
Attachment #8611606 - Flags: review?(margaret.leibovic)
Bug 1147064 - Add positive/negative to buttons. r=margaret
Attachment #8611607 - Flags: review?(margaret.leibovic)
Bug 1147064 - Add colored buttons. r=margaret
Attachment #8611608 - Flags: review?(margaret.leibovic)
Bug 1147064 - Handle callback ids for Login doorhanger dialogs. r=margaret
Attachment #8611609 - Flags: review?(margaret.leibovic)
Bug 1147064 - Clean up misc styling from feedback. r=margaret
Attachment #8611610 - Flags: review?(margaret.leibovic)
Attachment #8607235 - Attachment is obsolete: true
Attachment #8607235 - Flags: review?(margaret.leibovic)
Comment on attachment 8611604 [details]
MozReview Request: Bug 1147064 - Abstract out shared doorhanger layout. r=margaret

https://reviewboard.mozilla.org/r/8867/#review8599

Ship It!
Attachment #8611604 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8611605 [details]
MozReview Request: Bug 1147064 - Remove extra layer of layout for overdraw. r=margaret

https://reviewboard.mozilla.org/r/8869/#review8601

Ship It!
Attachment #8611605 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8611606 [details]
MozReview Request: Bug 1147064 - Make negative/positive button order consistent. r=margaret

https://reviewboard.mozilla.org/r/8871/#review8603

Ship It!
Attachment #8611606 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8611607 [details]
MozReview Request: Bug 1147064 - Add positive/negative to buttons. r=margaret

https://reviewboard.mozilla.org/r/8873/#review8605

Ship It!
Attachment #8611607 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8611608 [details]
MozReview Request: Bug 1147064 - Add colored buttons. r=margaret

https://reviewboard.mozilla.org/r/9233/#review8609

Ship It!
Attachment #8611608 - Flags: review?(margaret.leibovic) → review+
https://reviewboard.mozilla.org/r/9233/#review8607

> I went through all the addons that call NativeWindow.doorhanger.show, and the only one that (might) have more than two buttons is https://mxr.mozilla.org/addons/source/456016/bootstrap.js#197 .
> 
> I'm inclined to Log an error (rather than throw an uncaught exception) and only add the first two buttons so we fail gracefully. I'm also updating the MDN docs.

Thanks for investiagting. I think updating the MDN docs and making a post to mobile-firefox-dev should cover our bases here.
Attachment #8611609 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8611609 [details]
MozReview Request: Bug 1147064 - Handle callback ids for Login doorhanger dialogs. r=margaret

https://reviewboard.mozilla.org/r/9235/#review8611

Ship It!
Comment on attachment 8611610 [details]
MozReview Request: Bug 1147064 - Clean up misc styling from feedback. r=margaret

https://reviewboard.mozilla.org/r/9315/#review8613

Ship It!
Attachment #8611610 - Flags: review?(margaret.leibovic) → review+
I've updated the docs [1] but I want to update the image (which looks like it's all the way back from Gingerbread!) before I make a post to mobile-firefox-dev.

[1] https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/NativeWindow/doorhanger
Backed out in https://hg.mozilla.org/integration/fx-team/rev/fc8a4f4ac9b8 for java.lang.NullPointerException at org.mozilla.gecko.widget.DoorHanger.<init>(DoorHanger.java:79) in mochitest-3 and mochitest-11, https://treeherder.mozilla.org/logviewer.html#?job_id=3290466&repo=fx-team, https://treeherder.mozilla.org/logviewer.html#?job_id=3290459&repo=fx-team
Whoops, I somehow deleted the line where the layout was inflated. Put it back and relanded.
Tested with:
Device: Moto X (Androd 4.4.4)
Build: Firefox for Android 41.0a1 (2015-06-04)
You need to log in before you can comment on or make changes to this bug.