Clean up visuals of password doorhanger notification for saving and updating

RESOLVED FIXED in Firefox 39

Status

()

Firefox for Android
Theme and Visual Design
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: antlam, Assigned: liuche)

Tracking

(Blocks: 4 bugs)

unspecified
Firefox 39
x86
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(14 attachments, 7 obsolete attachments)

10.38 KB, application/zip
Details
172.73 KB, image/png
Details
49.34 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
118.84 KB, image/png
antlam
: review-
Details
39 bytes, text/x-review-board-request
liuche
: review+
Details | Review
39 bytes, text/x-review-board-request
liuche
: review+
Details | Review
39 bytes, text/x-review-board-request
liuche
: review+
Details | Review
39 bytes, text/x-review-board-request
liuche
: review+
Details | Review
39 bytes, text/x-review-board-request
liuche
: review+
Details | Review
39 bytes, text/x-review-board-request
liuche
: review+
Details | Review
39 bytes, text/x-review-board-request
liuche
: review+
Details | Review
39 bytes, text/x-review-board-request
liuche
: review+
Details | Review
39 bytes, text/x-review-board-request
liuche
: review+
Details | Review
39 bytes, text/x-review-board-request
liuche
: review+
Details | Review
(Reporter)

Description

3 years ago
Created attachment 8510519 [details]
Screenshot_2014-08-29-16-41-05.png

One of the common doorhangers that users get. We should clean up the type, spacing, etc.. around this

Comment 1

2 years ago
We use the same visuals for all of our doorhanger notifications, so this bug should really be about updating those shared styles.
Summary: Clean up visuals of "save password" doorhanger → Clean up visuals of doorhanger notifications
(Reporter)

Comment 2

2 years ago
Yes, i definitely want to share the styles as much as possible. I think it's because the original meta was more about visuals so I focused this on a specific one to track work.

Updated

2 years ago
Blocks: 1124232
No longer blocks: 1058818
(Reporter)

Comment 3

2 years ago
Created attachment 8554955 [details]
prev_savepw_update_mock1.png

I've been iterating on these for a while and took heavy inspiration from a lot of the work Stephen's been doing around these things for the Desktop side. I've both adapted those conventions for mobile, and taken into account all of the latest UI changes/ new features we've been working on. 

For symmetry and clarity, I've cleaned up our doorhangers styles here and aimed to maintain the defining qualities of our old ones too specifically around spoof-ability and context (where is this thing coming from?).

This work will be pretty important as a lot of the Passwords UX stuff, tracking protection work, etc happens in the doorhanger.
(Reporter)

Comment 4

2 years ago
Created attachment 8554958 [details]
prev_trackingshield_update_mock1.png

Tracking ID mock
(Reporter)

Comment 5

2 years ago
Created attachment 8554959 [details]
prev_siteID_update_mock1.png

site ID mock
(Assignee)

Updated

2 years ago
Blocks: 1079403
(Assignee)

Comment 6

2 years ago
The canonical source for the mocks is now: https://www.lucidchart.com/documents/edit/87ab1cc8-e708-49d3-8b91-6e2e6da346fb/1
(Reporter)

Comment 7

2 years ago
Created attachment 8566117 [details]
icon_key_dh.zip
(Reporter)

Comment 8

2 years ago
Created attachment 8566119 [details]
prev_pw_save_mock2.png
Attachment #8510519 - Attachment is obsolete: true
Attachment #8554955 - Attachment is obsolete: true
Attachment #8554958 - Attachment is obsolete: true
Attachment #8554959 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Assignee: nobody → liuche
Summary: Clean up visuals of doorhanger notifications → Clean up visuals of Password doorhanger notifications
Blocks: 1118955
See Also: → bug 1129619
Summary: Clean up visuals of Password doorhanger notifications → Clean up visuals of password doorhanger notifications and add affordance to edit captured credentials before saving
Priority: -- → P1
Bug 1129619 is the Desktop version of this.
(Assignee)

Comment 10

2 years ago
Created attachment 8577038 [details]
MozReview Request: bz://1088220/liuche

/r/5303 - Bug 1088220 - WIP Split Doorhanger to allow different Doorhanger types.

Pull down this commit:

hg pull review -r 4cdf5747814bcfc79e7fb5d61df7661b63504926
(Assignee)

Comment 11

2 years ago
Next: non-editable save/update password doorhangers.
(Assignee)

Updated

2 years ago
Attachment #8577038 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 12

2 years ago
Comment on attachment 8577038 [details]
MozReview Request: bz://1088220/liuche

/r/5303 - Bug 1088220 - Split DoorHanger to support other doorhangers. r=margaret
/r/5413 - Bug 1088220 - Rename DoorHanger to Doorhanger. r=margaret

Pull down these commits:

hg pull review -r bbd3712579079c5d60f16c97b0acc39470314be2

Comment 13

2 years ago
https://reviewboard.mozilla.org/r/5413/#review4367

Reviewboard sucks at hg rename, so it's hard to follow this, but as long as this is a staight-up variable reame, rubber stamp from me.

Comment 14

2 years ago
https://reviewboard.mozilla.org/r/5303/#review4369

::: mobile/android/base/resources/layout/site_identity.xml
(Diff revision 2)
> +    <LinearLayout android:layout_width="match_parent"

Adding another nested LinearLayout feels a bit smelly, but probably the bigger issue is the existing view hierarchy we have in here. Something to consider cleaning up in the future if/when we change the site identity appearance (e.g. getting rid of larry).

::: mobile/android/base/resources/layout/site_identity.xml
(Diff revision 2)
> +    <View android:id="@+id/divider_doorhanger"

Adding a divider to SiteIdentityPopup feels a bit out of the scope of this bug, but it doesn't look too complicated, so I'll allow it :)

::: mobile/android/base/widget/DefaultDoorHanger.java
(Diff revision 2)
> +    public static enum Theme {

It looks like we're getting rid of the dark theme, so you can just get rid of this enum.

::: mobile/android/base/widget/DefaultDoorHanger.java
(Diff revision 2)
> +        super(context, tabId, id, Type.DEFAULT);

Are you missing something here? Where is Type defined? I think you may have accidentally omitted changes to DoorHanger.java from this patch.
(Assignee)

Comment 15

2 years ago
Created attachment 8578325 [details] [diff] [review]
Patch: Split Doorhanger.

This is a bit of a nightmare - it looks like reviewboard doesn't handle renames where you re-create a file that was renamed, so I'm just uploading this as a patch. This includes the DoorHanger.java changes.

I'm leaving off the rename patch for now, because for some reason, mercurial is also having a case collision folding error when making only case-changes to file names (even though OSX is a case-sensitive OS!), and it's destroying files that have been committed by deleting them. I don't want to deal with this right now, so I'll change it in a follow-up bug or something.

> ::: mobile/android/base/resources/layout/site_identity.xml
> (Diff revision 2)
> > +    <LinearLayout android:layout_width="match_parent"
> 
> Adding another nested LinearLayout feels a bit smelly, but probably the
> bigger issue is the existing view hierarchy we have in here. Something to
> consider cleaning up in the future if/when we change the site identity
> appearance (e.g. getting rid of larry).

Agreed - this is so that we can add the divider in SiteIdentityPopup, because Anthony says we can remove the dark theme, which greatly simplifies abstracting out the Doorhanger code (which is used in both SiteIdentityPopup and DoorHangerPopup).

When we turn SiteIdentityPopup into a Real Popup that inherits from DoorHanger, we can avoid this, but right now SiteIdentityPopup is hardcoded to load the site_identity layout xml.

> ::: mobile/android/base/widget/DefaultDoorHanger.java
> (Diff revision 2)
> > +        super(context, tabId, id, Type.DEFAULT);
> 
> Are you missing something here? Where is Type defined? I think you may have
> accidentally omitted changes to DoorHanger.java from this patch.

This should be present in this patch, although it wasn't showing up on reviewboard for some reason...
Attachment #8577038 - Attachment is obsolete: true
Attachment #8577038 - Flags: review?(margaret.leibovic)
Attachment #8578325 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

2 years ago
Summary: Clean up visuals of password doorhanger notifications and add affordance to edit captured credentials before saving → Clean up visuals of password doorhanger notification for saving and updating
(Assignee)

Updated

2 years ago
Blocks: 1144385

Comment 16

2 years ago
Comment on attachment 8578325 [details] [diff] [review]
Patch: Split Doorhanger.

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

This is a bit hard to review because there are lots of changes in DefaultDoorHanger.java and DoorHanger.java. I think it might have been easier if the split was done in one patch, and then the logic/renaming changes done in a second. Please make sure you test this with complicated doorhangers, like the WebRTC one.

This is looking good, but I'd like to see a new version and the answers to some questions before giving an r+.

::: mobile/android/base/DoorHangerPopup.java
@@ +228,5 @@
>       * Gets a doorhanger.
>       *
>       * This method must be called on the UI thread.
>       */
>      DoorHanger getDoorHanger(int tabId, String value) {

You could file a follow-up bug to update this parameter to be `identifier` instead.

::: mobile/android/base/widget/DefaultDoorHanger.java
@@ -310,5 @@
> -        Rect rect = new Rect();
> -        spinner.getBackground().getPadding(rect);
> -
> -        // Set the difference in padding to the spinner view to align it with doorhanger text.
> -        view.setPadding(sInputPadding - rect.left - textPadding, 0, rect.right, sInputPadding);

Why is it okay to get rid of all this logic?

::: mobile/android/base/widget/DoorHanger.java
@@ +13,5 @@
>  import android.text.style.ForegroundColorSpan;
>  import android.text.style.URLSpan;
>  import android.view.LayoutInflater;
>  import android.view.View;
> +import android.widget.*;

Please replace this with specific class imports.

@@ +55,2 @@
>  
> +    // TODO: pull these from JSON message.

This doesn't sound like a TODO... we already do this below.

@@ -162,5 @@
>      }
>  
> -    public void setMessage(String message) {
> -        Spanned markupMessage = Html.fromHtml(message);
> -        mTextView.setMovementMethod(LinkMovementMethod.getInstance()); // Necessary for clickable links

Why did you drop this line?

@@ +173,5 @@
>              public void onClick(View v) {
>                  listener.onButtonClick(DoorHanger.this, tag);
>              }
>          });
> +            mButtonsContainer.setVisibility(View.VISIBLE);

Looks like you accidentally copied this line here when you didn't mean to.

@@ -211,2 @@
>              divider.setVisibility(View.VISIBLE);
> -            divider.setBackgroundColor(mDividerColor);

Why can we remove this?
Attachment #8578325 - Flags: review?(margaret.leibovic) → feedback+
(Assignee)

Comment 17

2 years ago
(In reply to :Margaret Leibovic from comment #16)
> Comment on attachment 8578325 [details] [diff] [review]
> Patch: Split Doorhanger.
> 
> Review of attachment 8578325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a bit hard to review because there are lots of changes in
> DefaultDoorHanger.java and DoorHanger.java. I think it might have been
> easier if the split was done in one patch, and then the logic/renaming
> changes done in a second. Please make sure you test this with complicated
> doorhangers, like the WebRTC one.
> 
> This is looking good, but I'd like to see a new version and the answers to
> some questions before giving an r+.
> 
> ::: mobile/android/base/DoorHangerPopup.java
> @@ +228,5 @@
> >       * Gets a doorhanger.
> >       *
> >       * This method must be called on the UI thread.
> >       */
> >      DoorHanger getDoorHanger(int tabId, String value) {
> 
> You could file a follow-up bug to update this parameter to be `identifier`
> instead.

Okay, I might just change this in the later patches for unifying siteIdentity - variable renaming seems so trivial x_x
> 
> ::: mobile/android/base/widget/DefaultDoorHanger.java
> @@ -310,5 @@
> > -        Rect rect = new Rect();
> > -        spinner.getBackground().getPadding(rect);
> > -
> > -        // Set the difference in padding to the spinner view to align it with doorhanger text.
> > -        view.setPadding(sInputPadding - rect.left - textPadding, 0, rect.right, sInputPadding);
> 
> Why is it okay to get rid of all this logic?

I was looking at why all this logic was here in the first place, and it's apparently there to shift the dropdown option menus closer to the edge of the doorhanger so that the text all lines up. This isn't how the UI is intended to work (the edge of the dropdown menu should be aligned with the line of text, not the text of the dropdown menu) - it's a lot of complexity to hack the default intended Android UI. Talked this over with antlam, he says we should use the default behavior.

> @@ -162,5 @@
> >      }
> >  
> > -    public void setMessage(String message) {
> > -        Spanned markupMessage = Html.fromHtml(message);
> > -        mTextView.setMovementMethod(LinkMovementMethod.getInstance()); // Necessary for clickable links
> 
> Why did you drop this line?

There is a method for adding a link that sets the MovementMethod, and I didn't find any doorhangers that had links in the actual text.

> @@ -211,2 @@
> >              divider.setVisibility(View.VISIBLE);
> > -            divider.setBackgroundColor(mDividerColor);
> 
> Why can we remove this?
Since we don't use the dark theme anymore (I checked with Anthony), this can be set through xml instead dynamically.
(Assignee)

Comment 18

2 years ago
Created attachment 8579503 [details] [diff] [review]
Patch: Split Doorhanger v2

Removed TODOs that were unnecessary, updated imports, removed extra mButtonsContainer line that was unnecessary.
Attachment #8578325 - Attachment is obsolete: true
Attachment #8579503 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

2 years ago
Attachment #8579503 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 19

2 years ago
Created attachment 8579529 [details] [diff] [review]
Patch: Split Doorhanger v2

Dammit, I didn't realize that IntelliJ was autocombining my imports for >5 imports from a single component. Updated this in the patch.
Attachment #8579503 - Attachment is obsolete: true
Attachment #8579529 - Flags: review?(margaret.leibovic)

Comment 20

2 years ago
(In reply to Chenxia Liu [:liuche] from comment #17)

> > @@ -162,5 @@
> > >      }
> > >  
> > > -    public void setMessage(String message) {
> > > -        Spanned markupMessage = Html.fromHtml(message);
> > > -        mTextView.setMovementMethod(LinkMovementMethod.getInstance()); // Necessary for clickable links
> > 
> > Why did you drop this line?
> 
> There is a method for adding a link that sets the MovementMethod, and I
> didn't find any doorhangers that had links in the actual text.

This was added in bug 964412 for add-ons to use, so I think we should keep it here.

Updated

2 years ago
Attachment #8579529 - Flags: review?(margaret.leibovic) → review+

Updated

2 years ago
QA Contact: ioana.chiorean
(Assignee)

Comment 21

2 years ago
Comment on attachment 8577038 [details]
MozReview Request: bz://1088220/liuche

/r/5303 - Bug 1088220 - Split DoorHanger to support other doorhangers. r=margaret
/r/5413 - Bug 1088220 - Add key icon resources. r=margaret
/r/5923 - Bug 1088220 - Add Config for Doorhangers. r=margaret
/r/5925 - Bug 1088220 - Switch to using DoorhangerConfig. r=margaret
/r/5927 - Bug 1088220 - Add styling changes for new Login doorhanger. r=margaret
/r/5929 - Bug 1088220 - Add login doorhanger. r=margaret

Pull down these commits:

hg pull review -r f3be63111aefcbc7fa5fecf1e9ac6d5129898c2f
Attachment #8577038 - Attachment is obsolete: false
Attachment #8577038 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 22

2 years ago
Created attachment 8582191 [details]
Screenshot: Doorhangers

Margaret: I know you've already reviewed the first commit in this queue via a patch, but reviewboard is just confused.
Attachment #8582191 - Flags: review?(alam)
(Reporter)

Comment 23

2 years ago
Comment on attachment 8582191 [details]
Screenshot: Doorhangers

This is looking great! few notes:

 - we still need to center align the doorhangers
 - "allimoz" should be in our Link blue? (or is it just not pressable right now?)
 - Affirmative actions should be consistently on the right, while negative actions on the left (swap around Update and Don't update)
 - Can we try to give Affirmative action buttons the blue background (white text)?
 - In the same line, could we give negative action buttons the Toolbar menu dark grey background color?
 - can we remove the orange divider too?

Thanks chenxia!
Flags: needinfo?(liuche)
Attachment #8582191 - Flags: review?(alam) → review-
(Assignee)

Comment 24

2 years ago
Cool, I'll update the dividers to #D7D9D8 and add it to the palette as menu divider grey, and I'll swap the buttons.

> 
>  - we still need to center align the doorhangers
bug 1139551

>  - "allimoz" should be in our Link blue? (or is it just not pressable right
> now?)
bug 1144385

>  - Can we try to give Affirmative action buttons the blue background (white
> text)?
>  - In the same line, could we give negative action buttons the Toolbar menu
> dark grey background color?
bug 1147064, I'll update all the doorhangers at once, since this is just for the login doorhanger.
Flags: needinfo?(liuche)
(Assignee)

Comment 25

2 years ago
(In reply to Chenxia Liu [:liuche] from comment #24)
> Cool, I'll update the dividers to #D7D9D8

I meant #D7D9DB...
(Assignee)

Comment 26

2 years ago
Comment on attachment 8577038 [details]
MozReview Request: bz://1088220/liuche

/r/5303 - Bug 1088220 - Split DoorHanger to support other doorhangers. r=margaret
/r/5413 - Bug 1088220 - Add key icon resources. r=margaret
/r/5923 - Bug 1088220 - Add Config for Doorhangers. r=margaret
/r/5925 - Bug 1088220 - Switch to using DoorhangerConfig. r=margaret
/r/5927 - Bug 1088220 - Add styling changes for new Login doorhanger. r=margaret
/r/5929 - Bug 1088220 - Add login doorhanger. r=margaret
/r/5985 - Bug 1088220 - Update text size for doorhangers. r=margaret

Pull down these commits:

hg pull review -r 07125a25ffaf371f1a6d36229a79a6689e92dfb9
(Assignee)

Comment 27

2 years ago
Addressed Anthony's comments and fixed doorhanger text sizing.

Comment 28

2 years ago
https://reviewboard.mozilla.org/r/5413/#review4939

Ship It!

Comment 29

2 years ago
https://reviewboard.mozilla.org/r/5923/#review4941

::: mobile/android/base/widget/DoorhangerConfig.java
(Diff revision 2)
> +package org.mozilla.gecko.widget;

Add a license header.

::: mobile/android/base/widget/DoorhangerConfig.java
(Diff revision 2)
> +public class DoorhangerConfig {

I feel like we should call this DoorHangerConfig to be consistent with the current DoorHanger objects... but I really don't care that much, since things will just not build if you mess it up :)

::: mobile/android/base/widget/DoorhangerConfig.java
(Diff revision 2)
> +    public void addTabId(int tabId) {

These methods should probably be called `setFoo` not `addFoo`, since they replace any existing thing.

Also, there's no constructor for this class? I haven't looked at the future patches where you start using this yet, but it seems odd that we wouldn't use a constructor to initialize this with all its properties, especially ones that I imagaine shouldn't ever change, like `id`.

Comment 30

2 years ago
https://reviewboard.mozilla.org/r/5925/#review4943

::: mobile/android/base/DoorHangerPopup.java
(Diff revision 2)
> -    void addDoorHanger(final int tabId, final String value, final String message,
> +    void addDoorHanger(DoorhangerConfig config) {

This is definitely a lot nicer than the long argument list.

::: mobile/android/base/DoorHangerPopup.java
(Diff revision 2)
> +        final DoorhangerConfig config = new DoorhangerConfig();

I feel like we should be passing the non-optional properties into a DoorhangerConfig constructor here, and then make them immutable. Right now it feels a bit dangerous that anyone who has their hands on a Doorhanger config object could go around changing its state.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
(Diff revision 2)
> -            message = mContext.getString(R.string.loaded_mixed_content_message);
> +            config.addMessage(mContext.getString(R.string.loaded_mixed_content_message));

Yeah, definitely think these should be `setMessage` not `addMessage`, since right now it looks like it's possible to have a list of messages.

::: mobile/android/base/widget/DoorHanger.java
(Diff revision 2)
> +    public static DoorHanger Get(Context context, DoorhangerConfig config) {

Why `Get` with a captial G?

Comment 31

2 years ago
https://reviewboard.mozilla.org/r/5929/#review4949

::: mobile/android/base/widget/DoorHanger.java
(Diff revision 2)
>                  case PASSWORD:

Should we call this LOGIN to be consistent?

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 2)
>      get _strBundle() {

Let's get a mentor bug on file to fix the indentation in this file :)

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 2)
> -    _showLoginNotification : function (aName, aText, aButtons) {
> +    _showLoginNotification : function (aName, aTitle, aBody, aButtons, aSubtext) {

We should update the documentation here to explain what these parameters are.

::: mobile/android/chrome/content/browser.js
(Diff revision 2)
> -    show: function(aMessage, aValue, aButtons, aTabID, aOptions) {
> +    show: function(aMessage, aValue, aButtons, aTabID, aOptions, aTitle, aSubtext, aCategory) {

I think we should make title/subtext/category part of the options object to avoid having such a long parameter list here.

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 2)
> +        var displayHost = this._getShortDisplayHost(aOldLogin.hostname);

Unless you have a compelling reason not to, you should use `let`, not `var`.

We could also get a mentor bug to un-var-ify this file.

::: mobile/locales/en-US/overrides/passwordmgr.properties
(Diff revision 2)
> -# String is the login's hostname
> +neverButton=Never

You may need to update strings in the robocop tests.

Comment 32

2 years ago
https://reviewboard.mozilla.org/r/5985/#review4951

::: mobile/android/base/resources/values/styles.xml
(Diff revision 1)
> -    <style name="TextAppearance.Widget.DoorHanger.Small.Light"/>
> +    <style name="DoorHangerTextAppearance.Medium.Light"/>

How does this work? Shouldn't this have parent="DoorHangerTestAppearance.Medium"? If we just set a textAppearence to this, will it have the correct font size?
(Assignee)

Updated

2 years ago
Blocks: 1147064
(Assignee)

Updated

2 years ago
Blocks: 1147201
(Assignee)

Comment 33

2 years ago
https://reviewboard.mozilla.org/r/5923/#review4959

> I feel like we should call this DoorHangerConfig to be consistent with the current DoorHanger objects... but I really don't care that much, since things will just not build if you mess it up :)

I'm pretty meh about this, and I want to make everything Doorhanger anyways. Filed bug 1147201. Also, I don't want to go through and rename everything now and then rename it back in that bug...

> These methods should probably be called `setFoo` not `addFoo`, since they replace any existing thing.
> 
> Also, there's no constructor for this class? I haven't looked at the future patches where you start using this yet, but it seems odd that we wouldn't use a constructor to initialize this with all its properties, especially ones that I imagaine shouldn't ever change, like `id`.

Good point - I made a constructor for tabId and id and made them final fields. We still need an empty constructor for SiteIdentityPopup because it doesn't care what tab it's in because it's added/removed on every show/dismissal, so these aren't technically required fields. But that will change in the future! so we can just remove the empty constructor then.
(Assignee)

Comment 34

2 years ago
https://reviewboard.mozilla.org/r/5925/#review4961

> Why `Get` with a captial G?

It's Java convention to use a capitalized Get for static getter methods in this not-really-Factory-but-kinda pattern.
(Assignee)

Comment 35

2 years ago
https://reviewboard.mozilla.org/r/5929/#review4957

> Unless you have a compelling reason not to, you should use `let`, not `var`.
> 
> We could also get a mentor bug to un-var-ify this file.

Filed bug 1147211. I'll make it mentorable once we don't need to touch this file as much.

> Let's get a mentor bug on file to fix the indentation in this file :)

Filed bug 1147197.

> I think we should make title/subtext/category part of the options object to avoid having such a long parameter list here.

I kept category as a top-level argument because we need that to select which doorhanger to create (and this will be useful in the future when we break DefaultDoorHanger out into separate doorhangers).

> You may need to update strings in the robocop tests.

Yep, writing a patch for fixing the tests as a separate commit.
(Assignee)

Comment 36

2 years ago
https://reviewboard.mozilla.org/r/5985/#review4953

> How does this work? Shouldn't this have parent="DoorHangerTestAppearance.Medium"? If we just set a textAppearence to this, will it have the correct font size?

The dot hierarchy in Android styles implies a parent relationship; the reason the old code here explicitly set parent was because the dot hierarchy didn't line up (in fact, our style files mix the parenting hierarchy and some homespun naming hierarchy that is very un-Android). So DHTA.Medium and DHTA.Small both automatically inherit the characteristics set in DoorHangerTextAppearance.
If you try to use a dot hierarchy using an implicit parent that doesn't actually exist, the build will actually fail and complain that a parent can't be found.
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Comment 39

2 years ago
Comment on attachment 8577038 [details]
MozReview Request: bz://1088220/liuche

/r/5303 - Bug 1088220 - Split DoorHanger to support other doorhangers. r=margaret
/r/5413 - Bug 1088220 - Add key icon resources. r=margaret
/r/5923 - Bug 1088220 - Add Config for Doorhangers. r=margaret
/r/5925 - Bug 1088220 - Switch to using DoorhangerConfig. r=margaret
/r/5927 - Bug 1088220 - Add styling changes for new Login doorhanger. r=margaret
/r/6109 - Bug 1088220 - Add login doorhanger. r=margaret
/r/6111 - Bug 1088220 - Style changes. r=margaret
/r/6117 - Bug 1088220 - Update tests with new strings. r=margaret

Pull down these commits:

hg pull review -r f596d2b8641d785c54c20bfdad69d449abf05bb9
(Assignee)

Comment 40

2 years ago
Try run for the updated tests (ran them locally first already): https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c578181c64d
(Assignee)

Comment 41

2 years ago
Switching back to Nightly, I realized that with the new mocks, we're swapping the confirm/dismiss buttons on the doorhanger, and also turning the dismiss into "Never". I wonder if people will accidentally muscle-memory the wrong button (if they tend to hit "Not now") all the time.

...an undo toast could be nice.

Comment 42

2 years ago
(In reply to Chenxia Liu [:liuche] from comment #41)
> Switching back to Nightly, I realized that with the new mocks, we're
> swapping the confirm/dismiss buttons on the doorhanger, and also turning the
> dismiss into "Never". I wonder if people will accidentally muscle-memory the
> wrong button (if they tend to hit "Not now") all the time.
> 
> ...an undo toast could be nice.

antlam, how do you feel about these points?

Users can undo the "never" action by editing site settings, but that's not really discoverable right now. I think the undo toast could be a good idea, maybe we should file a follow-up for that.
Flags: needinfo?(alam)

Comment 43

2 years ago
https://reviewboard.mozilla.org/r/6109/#review5127

::: mobile/android/base/DoorHangerPopup.java
(Diff revision 1)
> -        final int tabId = json.getInt("tabId");
> +        final int tabId = json.getInt("tabID");

This change should really be in the earlier commit where you introduced this, but if you fold these all together to land, that won't matter :)

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +        } catch (JSONException e) {

The try/catch should probably just be around the `getString` calls that will throw.

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +                final String url = titleObj.getString("resource");

If `resource` is indeed optional, this should be `optString`.

I would also name this local variable `resource` to be consistent (or alternately name the JSON property `url`).

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> +   *

I think you should add some more details about the format of this resource URL, since it's not clear what's supported.

Also, it might reduce confusion to rename the `title` property in here to something like `message`, since it's already part of a `title` property.

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 1)
> +     *        String to be displayed below the aBody message

Nice documentation update.

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 1)
> -        } else {
> +        let subtext = this._sanitizeUsername(aLogin.username);

Looks like you'll run into a JS error if aLogin.username is null/undefined:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/LoginManagerPrompter.js#394

I think we should keep the truth-y check around this (or update _sanitizeUsername).
(Reporter)

Comment 44

2 years ago
Switching the confirm/dismiss was really a move to unify these interactions to be more consistent with both themselves (sometimes we're inconsistent) as well as system (on mobile, often right is confirm, left is dismiss). 

But the intent was originally to keep consistent with Desktop copy ("Never" and "Remember"). Also, since tapping anywhere outside dismisses the doorhanger, it seemed alright. However, we can definitely revisit this later if we think it's weird on a mobile platform.

Hm, I'm open to the idea of a confirmation toast with an undo action. 

Something like "Login saved with Firefox | Undo" ?
Flags: needinfo?(alam)

Comment 45

2 years ago
https://reviewboard.mozilla.org/r/5923/#review5129

Ship It!

Comment 46

2 years ago
https://reviewboard.mozilla.org/r/5925/#review5131

Ship It!

Comment 47

2 years ago
https://reviewboard.mozilla.org/r/5927/#review5133

Ship It!

Comment 48

2 years ago
https://reviewboard.mozilla.org/r/6109/#review5135

r+ with issues addressed.

Comment 49

2 years ago
https://reviewboard.mozilla.org/r/6111/#review5137

I would say this should be combined with the other style changes commit, but I don't care enough that you should change that now :)

Comment 50

2 years ago
https://reviewboard.mozilla.org/r/6117/#review5139

::: mobile/android/base/tests/StringHelper.java
(Diff revision 1)
> +    public static final String CONTEXT_MENU_SITE_SETTINGS_SAVE_PASSWORD = "Save Password";

We'll have some other bug to make sure our use of "passowrds" and "logins" is consistent, right?

Updated

2 years ago
Attachment #8577038 - Flags: review?(margaret.leibovic) → review+

Comment 51

2 years ago
Comment on attachment 8577038 [details]
MozReview Request: bz://1088220/liuche

https://reviewboard.mozilla.org/r/5301/#review5141

Ship It!
(Assignee)

Updated

2 years ago
Blocks: 1147984
(Assignee)

Comment 52

2 years ago
https://reviewboard.mozilla.org/r/6109/#review5179

> I think you should add some more details about the format of this resource URL, since it's not clear what's supported.
> 
> Also, it might reduce confusion to rename the `title` property in here to something like `message`, since it's already part of a `title` property.

I wanted to leave this a little open-ended because this might be handled differently by different doorhangers, but for now I made it more specific and added a note about generalizing.
(Assignee)

Comment 53

2 years ago
https://reviewboard.mozilla.org/r/6117/#review5177

> We'll have some other bug to make sure our use of "passowrds" and "logins" is consistent, right?

Bug 1136477
(Assignee)

Comment 54

2 years ago
Comment on attachment 8577038 [details]
MozReview Request: bz://1088220/liuche

/r/5303 - Bug 1088220 - Split DoorHanger to support other doorhangers. r=margaret
/r/5413 - Bug 1088220 - Add key icon resources. r=margaret
/r/5923 - Bug 1088220 - Add Config for Doorhangers. r=margaret
/r/5925 - Bug 1088220 - Switch to using DoorhangerConfig. r=margaret
/r/5927 - Bug 1088220 - Add login doorhanger. r=margaret
/r/6109 - Bug 1088220 - Update tests with new strings. r=margaret

Pull down these commits:

hg pull review -r 9a15d2d5ae25e12c50b379f30b99ecab42820c9e
Attachment #8577038 - Flags: review+
(Assignee)

Comment 55

2 years ago
Comment on attachment 8577038 [details]
MozReview Request: bz://1088220/liuche

Carrying over r+.
Attachment #8577038 - Flags: review+
(Assignee)

Comment 56

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/30d0f12f958c
https://hg.mozilla.org/integration/fx-team/rev/3249fc039d08
https://hg.mozilla.org/integration/fx-team/rev/c8a569d73bb0
https://hg.mozilla.org/integration/fx-team/rev/e734718125b0
https://hg.mozilla.org/integration/fx-team/rev/ebf592c2fa4c
https://hg.mozilla.org/integration/fx-team/rev/28bc7e898980
Target Milestone: --- → Firefox 39
https://hg.mozilla.org/mozilla-central/rev/30d0f12f958c
https://hg.mozilla.org/mozilla-central/rev/3249fc039d08
https://hg.mozilla.org/mozilla-central/rev/c8a569d73bb0
https://hg.mozilla.org/mozilla-central/rev/e734718125b0
https://hg.mozilla.org/mozilla-central/rev/ebf592c2fa4c
https://hg.mozilla.org/mozilla-central/rev/28bc7e898980
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Depends on: 1148857
(Assignee)

Comment 58

2 years ago
Comment on attachment 8577038 [details]
MozReview Request: bz://1088220/liuche
Attachment #8577038 - Attachment is obsolete: true
Attachment #8618443 - Flags: review+
Attachment #8618444 - Flags: review+
Attachment #8618445 - Flags: review+
Attachment #8618446 - Flags: review+
Attachment #8618447 - Flags: review+
Attachment #8618448 - Flags: review+
Attachment #8618449 - Flags: review+
Attachment #8618450 - Flags: review+
Attachment #8618451 - Flags: review+
Attachment #8618452 - Flags: review+
(Assignee)

Comment 59

2 years ago
Created attachment 8618443 [details]
MozReview Request: Bug 1088220 - Update text size for doorhangers. r=margaret
(Assignee)

Comment 60

2 years ago
Created attachment 8618444 [details]
MozReview Request: Bug 1088220 - Add Config for Doorhangers. r=margaret
(Assignee)

Comment 61

2 years ago
Created attachment 8618445 [details]
MozReview Request: Bug 1088220 - Update tests with new strings. r=margaret
(Assignee)

Comment 62

2 years ago
Created attachment 8618446 [details]
MozReview Request: Bug 1088220 - Add login doorhanger. r=margaret
(Assignee)

Comment 63

2 years ago
Created attachment 8618447 [details]
MozReview Request: Bug 1088220 - Add login doorhanger. r=margaret
(Assignee)

Comment 64

2 years ago
Created attachment 8618448 [details]
MozReview Request: Bug 1088220 - Split DoorHanger to support other doorhangers. r=margaret
(Assignee)

Comment 65

2 years ago
Created attachment 8618449 [details]
MozReview Request: Bug 1088220 - Style changes. r=margaret
(Assignee)

Comment 66

2 years ago
Created attachment 8618450 [details]
MozReview Request: Bug 1088220 - Add key icon resources. r=margaret
(Assignee)

Comment 67

2 years ago
Created attachment 8618451 [details]
MozReview Request: Bug 1088220 - Update tests with new strings. r=margaret
(Assignee)

Comment 68

2 years ago
Created attachment 8618452 [details]
MozReview Request: Bug 1088220 - Switch to using DoorhangerConfig. r=margaret
Do these new patches still need to land in Nightly?
Flags: needinfo?(liuche)
(Assignee)

Comment 70

2 years ago
Sorry about this bugspam - I'm not sure at all what this is from, but there are no new patches.
Flags: needinfo?(liuche)
(Reporter)

Comment 71

2 years ago
Liz - this is Resolved, Fixed. :)
You need to log in before you can comment on or make changes to this bug.