Closed Bug 885962 Opened 11 years ago Closed 11 years ago

Polish mixed content blocking UI

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox23 unaffected, firefox24+ disabled, firefox25 verified, firefox26 verified, fennec25+)

VERIFIED FIXED
Firefox 25
Tracking Status
firefox23 --- unaffected
firefox24 + disabled
firefox25 --- verified
firefox26 --- verified
fennec 25+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(5 files, 3 obsolete files)

This is a follow-up to bug 860581.

Issues to address:
* Update orange triangle icons
* Create gray Larry icon
* Add icon to notification (probably want to do this by adding support for it to DoorHanger)
* Move Larry to the left side of the popup (and it looks like we'll want a bigger Larry icon)
* Make font size in the notification smaller

needinfo? to ibarlow for icons
* Change link text color
Flags: needinfo?(ibarlow)
Was confused about the double caution symbols visible on disabling protection. I think this should track-24 so that this case is clear to the user.
(In reply to Aaron Train [:aaronmt] from comment #1)
> Was confused about the double caution symbols visible on disabling
> protection. I think this should track-24 so that this case is clear to the
> user.

Yeah, we definitely need to fix this bug (or at least most of the issues here) before shipping this feature. As I mentioned in bug 860581, I just stole the desktop icon, that is not the intended icon.
Assignee: nobody → margaret.leibovic
Also, here are some specs for text size and colour: http://cl.ly/image/3s1H2T2i1e3u
Flags: needinfo?(ibarlow)
Attached patch WIP (obsolete) — Splinter Review
Some questions for ibarlow:
* Do we always want a gray larry now, regardless of the security state? (that's what was implied by the set of icons that were uploaded, so that's what I did here for now)
* If we do want a gray larry always, do we also want to get rid of the blue/green text in the popup?
* What color values would you prefer for the background/border of the bottom part of the popup?
Attachment #767484 - Flags: feedback?(sriram)
(In reply to :Margaret Leibovic from comment #4)

> Some questions for ibarlow:
> * Do we always want a gray larry now, regardless of the security state?
> (that's what was implied by the set of icons that were uploaded, so that's
> what I did here for now)

Always grey, please. 

> * If we do want a gray larry always, do we also want to get rid of the
> blue/green text in the popup?

Yup :)

> * What color values would you prefer for the background/border of the bottom
> part of the popup?

bottom background: dde43a
bottom button border: b3c2ce

Those screenshots are lookin great btw
(In reply to Ian Barlow (:ibarlow) from comment #8)

> bottom background: dde43a

Is there a typo here? This is yellow...
Comment on attachment 767484 [details] [diff] [review]
WIP

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

I'll give feedback based on the clarification for my doubts.

::: mobile/android/base/SiteIdentityPopup.java
@@ +146,5 @@
>          }
> +        TextView messageView = mMixedContentNotification.setMessage(message);
> +        messageView.setTextSize(mResources.getDimension(R.dimen.mixed_content_textsize));
> +        messageView.setLinkTextColor(0xFF2AA1FE);
> +

All these should go into a TextAppearance in styles.xml.

@@ +155,4 @@
>              mMixedContentNotification.addButton(mActivity.getString(R.string.disable_protection), "disable", this);
>              mMixedContentNotification.addButton(mActivity.getString(R.string.keep_blocking), "keepBlocking", this);
>          } else {
> +            mMixedContentNotification.setIcon(R.drawable.warning_doorhanger);

Can these be made into a single drawable with levels? Are they different from site_security_level.xml drawable?

::: mobile/android/base/resources/layout/doorhanger.xml
@@ +22,5 @@
> +                  android:layout_width="fill_parent"
> +                  android:layout_height="wrap_content"
> +                  android:textAppearance="?android:attr/textAppearanceMedium"
> +                  android:textColor="@color/doorhanger_text"
> +                  android:textColorLink="@color/doorhanger_link"/>

If you are setting all these here, why are you setting them again in SiteIdentityPopup.java? Are they different?
Attached patch patch (obsolete) — Splinter Review
I kept #FFDDE4EA for the notification background, since the color ibarlow gave me was yellow. But I can easily update this before landing if he gets back to me with a different color.

I also updated the link color to better match the blue from the mockups. I found that this is actually the only place we're putting links in a DoorHanger now, so it won't affect any other existing popups.
Attachment #767484 - Attachment is obsolete: true
Attachment #767484 - Flags: feedback?(sriram)
Attachment #768052 - Flags: review?(sriram)
(In reply to Sriram Ramasubramanian [:sriram] from comment #10)

> @@ +155,4 @@
> >              mMixedContentNotification.addButton(mActivity.getString(R.string.disable_protection), "disable", this);
> >              mMixedContentNotification.addButton(mActivity.getString(R.string.keep_blocking), "keepBlocking", this);
> >          } else {
> > +            mMixedContentNotification.setIcon(R.drawable.warning_doorhanger);
> 
> Can these be made into a single drawable with levels? Are they different
> from site_security_level.xml drawable?

Yeah, these are the big icons that go in the notification. But that sounds like a good idea, I'll make a different drawable with levels for this.

> ::: mobile/android/base/resources/layout/doorhanger.xml
> @@ +22,5 @@
> > +                  android:layout_width="fill_parent"
> > +                  android:layout_height="wrap_content"
> > +                  android:textAppearance="?android:attr/textAppearanceMedium"
> > +                  android:textColor="@color/doorhanger_text"
> > +                  android:textColorLink="@color/doorhanger_link"/>
> 
> If you are setting all these here, why are you setting them again in
> SiteIdentityPopup.java? Are they different?

These are the default styles for normal doorhangers. I found I didn't actually need to change the textColorLink, so I got rid of that part, but I found that I needed the text size to be smaller in this mixed content doorhanger than it usually is in regular doorhangers. Is there a better way to do that?
Comment on attachment 768052 [details] [diff] [review]
patch

Clearing review while I work on addressing comments.
Attachment #768052 - Flags: review?(sriram)
(In reply to :Margaret Leibovic from comment #9)
> (In reply to Ian Barlow (:ibarlow) from comment #8)
> 
> > bottom background: dde43a
> 
> Is there a typo here? This is yellow...

Gah... it should be #dde4Ea sorry!
tracking-fennec: ? → 24+
(In reply to :Margaret Leibovic from comment #12)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> 
> > @@ +155,4 @@
> > >              mMixedContentNotification.addButton(mActivity.getString(R.string.disable_protection), "disable", this);
> > >              mMixedContentNotification.addButton(mActivity.getString(R.string.keep_blocking), "keepBlocking", this);
> > >          } else {
> > > +            mMixedContentNotification.setIcon(R.drawable.warning_doorhanger);
> > 
> > Can these be made into a single drawable with levels? Are they different
> > from site_security_level.xml drawable?
> 
> Yeah, these are the big icons that go in the notification. But that sounds
> like a good idea, I'll make a different drawable with levels for this.

Actually, what is the benefit of having these in a single drawable with levels, as opposed to just setting a different image based on the case? Because I'm using a generic DoorHanger for this, I can't specify the drawable in the XML, so I would still need to set the drawable, then set a level depending on the case.
Attached patch patch v2 (obsolete) — Splinter Review
I moved the colors to colors.xml to clean things up a bit.

I know we normally avoid setting styles like this in Java as opposed to XML, but I think it's worth it here so that we can reuse the existing DoorHanger view, especially since this UI isn't going to come up very often (only when the user taps on this icon on a mixed content page).
Attachment #768052 - Attachment is obsolete: true
Attachment #768679 - Flags: review?(sriram)
(In reply to :Margaret Leibovic from comment #16)
> Created attachment 768679 [details] [diff] [review]
> patch v2
> 
> I moved the colors to colors.xml to clean things up a bit.
> 
> I know we normally avoid setting styles like this in Java as opposed to XML,
> but I think it's worth it here so that we can reuse the existing DoorHanger
> view, especially since this UI isn't going to come up very often (only when
> the user taps on this icon on a mixed content page).

Couple of doubts:
1. Do we really need two different text sizes for the mixed content? This is making things hard to keep track of. (This is for Ian).
2. I see you setting a different divider color for choices. Why is that? Will this affect default behavior? Could you try going to m.google.com/maps and post a screenshot? (It'll have 2 doorhangers I guess). (This is for margaret).
Flags: needinfo?(ibarlow)
(In reply to Sriram Ramasubramanian [:sriram] from comment #17)

> 2. I see you setting a different divider color for choices. Why is that?
> Will this affect default behavior? Could you try going to m.google.com/maps
> and post a screenshot? (It'll have 2 doorhangers I guess). (This is for
> margaret).

This is just for these doorhangers, because they have a different background color (so they need a different divider color). Regular doorhangers won't look any different (it's the same default color, I just factored it out to a resource color, since we were already using it in two places).
(In reply to Sriram Ramasubramanian [:sriram] from comment #17)
> (In reply to :Margaret Leibovic from comment #16)
> > Created attachment 768679 [details] [diff] [review]
> > patch v2
> > 
> > I moved the colors to colors.xml to clean things up a bit.
> > 
> > I know we normally avoid setting styles like this in Java as opposed to XML,
> > but I think it's worth it here so that we can reuse the existing DoorHanger
> > view, especially since this UI isn't going to come up very often (only when
> > the user taps on this icon on a mixed content page).
> 
> Couple of doubts:
> 1. Do we really need two different text sizes for the mixed content? This is
> making things hard to keep track of. (This is for Ian).

we can try using 14sp all the way through if you want. Though two sizes makes for a more useful hierarchy of information

> 2. I see you setting a different divider color for choices. Why is that?
> Will this affect default behavior? Could you try going to m.google.com/maps
> and post a screenshot? (It'll have 2 doorhangers I guess). (This is for
> margaret).

as margaret said, the different colour is to make the dividers more visible on darker backgrounds.
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #19)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #17)
> > (In reply to :Margaret Leibovic from comment #16)
> > > Created attachment 768679 [details] [diff] [review]
> > > patch v2
> > > 
> > > I moved the colors to colors.xml to clean things up a bit.
> > > 
> > > I know we normally avoid setting styles like this in Java as opposed to XML,
> > > but I think it's worth it here so that we can reuse the existing DoorHanger
> > > view, especially since this UI isn't going to come up very often (only when
> > > the user taps on this icon on a mixed content page).
> > 
> > Couple of doubts:
> > 1. Do we really need two different text sizes for the mixed content? This is
> > making things hard to keep track of. (This is for Ian).
> 
> we can try using 14sp all the way through if you want. Though two sizes
> makes for a more useful hierarchy of information

I don't have a strong opinion here, but I feel like this doesn't make the code much more complex, so if we prefer the design with multiple text sizes, I think we should just go ahead and do that. I can add some more comments if that makes things clearer.
Comment on attachment 768679 [details] [diff] [review]
patch v2

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

I am still not sure if this is a right approach. Ideally DoorHanger should expose methods that others should use. The way this patch does is, "hacking" DoorHanger -- which doesn't feel good.

My approach would be:
1. Add a theme to DoorHanger. setTheme(DoorHanger.LIGHT) or setTheme(DoorHanger.DARK);
Where LIGHT is the default.
2. When the theme is Dark, automatically switch the text size of the message, and flip the colors of the buttons.
These should all be done by DoorHanger and not be exposed by it to others.

Other than that, I don't see any major changes in existing doorhangers.

::: mobile/android/base/SiteIdentityPopup.java
@@ +145,5 @@
>              message = mActivity.getString(R.string.loaded_mixed_content_message);
>          }
> +        TextView messageView = mMixedContentNotification.setMessage(message);
> +        messageView.setTextSize(mResources.getDimension(R.dimen.mixed_content_textsize));
> +

This is not a good approach. I don't like sending back the TextView so someone else can play with it. It's better to expose another method "setTextSize()" on the DoorHanger, that takes care of this.

---
mMixedContentNotification.setMessage(message);
mMixedContentNotification.setTextSize(...);
---

@@ +150,3 @@
>          mMixedContentNotification.addLink(mActivity.getString(R.string.learn_more), MIXED_CONTENT_SUPPORT_URL, "\n\n");
>  
> +        int dividerColor = mResources.getColor(R.color.mixed_content_divider);

Make this final.

::: mobile/android/base/resources/layout/doorhanger.xml
@@ +4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <merge xmlns:android="http://schemas.android.com/apk/res/android">
>  
> +    <LinearLayout android:id="@+id/doorhanger_inputs"

There is another "doorhanger_inputs" id. Why there are two?

@@ +24,5 @@
> +                  android:textAppearance="?android:attr/textAppearanceMedium"
> +                  android:textColor="@color/doorhanger_text"
> +                  android:textColorLink="@color/doorhanger_link"/>
> +
> +    </LinearLayout>

The entire block can be a TextView with icon set as a compound drawable.
Do we have this icon always? Can Gecko add this icon? Or, is it just for the SiteIdentity stuff?
Attachment #768679 - Flags: review?(sriram) → review-
(In reply to Sriram Ramasubramanian [:sriram] from comment #21)
> Comment on attachment 768679 [details] [diff] [review]
> patch v2
> 
> Review of attachment 768679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am still not sure if this is a right approach. Ideally DoorHanger should
> expose methods that others should use. The way this patch does is, "hacking"
> DoorHanger -- which doesn't feel good.

Other than the way this patch changes the text size (which I mention below that I'll change), I consider the changes in this patch to be making the DoorHanger view more customizable, which is a form of "hacking" that I find acceptable. I think that as long as the behavior of the DoorHanger view remains predictable, these changes are acceptable.

> My approach would be:
> 1. Add a theme to DoorHanger. setTheme(DoorHanger.LIGHT) or
> setTheme(DoorHanger.DARK);
> Where LIGHT is the default.

Is this really a blocker for landing this patch, or could this be done in a follow-up bug? Since we're currently only using this dark theme here in the site identity popup, it kinda feels like overkill to make a whole new theme for it.

> 2. When the theme is Dark, automatically switch the text size of the
> message, and flip the colors of the buttons.
> These should all be done by DoorHanger and not be exposed by it to others.

This doesn't feel very robust either, since coupling the dark theme with the text size seems unexpected.

> Other than that, I don't see any major changes in existing doorhangers.

There aren't any changes here that should affect exiting doorhangers, so that's good to hear :)

> ::: mobile/android/base/SiteIdentityPopup.java
> @@ +145,5 @@
> >              message = mActivity.getString(R.string.loaded_mixed_content_message);
> >          }
> > +        TextView messageView = mMixedContentNotification.setMessage(message);
> > +        messageView.setTextSize(mResources.getDimension(R.dimen.mixed_content_textsize));
> > +
> 
> This is not a good approach. I don't like sending back the TextView so
> someone else can play with it. It's better to expose another method
> "setTextSize()" on the DoorHanger, that takes care of this.
> 
> ---
> mMixedContentNotification.setMessage(message);
> mMixedContentNotification.setTextSize(...);
> ---

I concede that passing the TextView back is bad, so I'll update my patch to do this.

> ::: mobile/android/base/resources/layout/doorhanger.xml
> @@ +4,5 @@
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >  
> >  <merge xmlns:android="http://schemas.android.com/apk/res/android">
> >  
> > +    <LinearLayout android:id="@+id/doorhanger_inputs"
> 
> There is another "doorhanger_inputs" id. Why there are two?

Good catch, that must have been a copy/paste error. That LinearLayout doesn't need an id.

> @@ +24,5 @@
> > +                  android:textAppearance="?android:attr/textAppearanceMedium"
> > +                  android:textColor="@color/doorhanger_text"
> > +                  android:textColorLink="@color/doorhanger_link"/>
> > +
> > +    </LinearLayout>
> 
> The entire block can be a TextView with icon set as a compound drawable.
> Do we have this icon always? Can Gecko add this icon? Or, is it just for the
> SiteIdentity stuff?

I tried with the compound drawable, but that aligns the icon at the top, not centered vertically like we want. We currently don't have any other DoorHangers that use this icon, but it makes it an option for the future (we could even file a pretty easy follow-up to add a JS API to let add-ons put their own icons in doorhanger notifications).
(In reply to :Margaret Leibovic from comment #22)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #21)
> > Comment on attachment 768679 [details] [diff] [review]
> > patch v2
> > 
> > Review of attachment 768679 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I am still not sure if this is a right approach. Ideally DoorHanger should
> > expose methods that others should use. The way this patch does is, "hacking"
> > DoorHanger -- which doesn't feel good.
> 
> Other than the way this patch changes the text size (which I mention below
> that I'll change), I consider the changes in this patch to be making the
> DoorHanger view more customizable, which is a form of "hacking" that I find
> acceptable. I think that as long as the behavior of the DoorHanger view
> remains predictable, these changes are acceptable.
> 
> > My approach would be:
> > 1. Add a theme to DoorHanger. setTheme(DoorHanger.LIGHT) or
> > setTheme(DoorHanger.DARK);
> > Where LIGHT is the default.
> 
> Is this really a blocker for landing this patch, or could this be done in a
> follow-up bug? Since we're currently only using this dark theme here in the
> site identity popup, it kinda feels like overkill to make a whole new theme
> for it.

I would like to have this approach. This clearly demarcates the different themes we use on a doorhanger. Blocking on this depends on when the feature needs to be landed. I am open to make that a follow up, if this feature has to be landed pretty soon. :mfinkle can comment on that.

> 
> > 2. When the theme is Dark, automatically switch the text size of the
> > message, and flip the colors of the buttons.
> > These should all be done by DoorHanger and not be exposed by it to others.
> 
> This doesn't feel very robust either, since coupling the dark theme with the
> text size seems unexpected.

It's a theme thing. If its a light theme -- emphasis on text. If its a dark theme -- lesser emphasis.
Flags: needinfo?(mark.finkle)
Attached patch patch v3Splinter Review
What do you think of this?
Attachment #768679 - Attachment is obsolete: true
Attachment #777240 - Flags: review?(sriram)
Comment on attachment 777240 [details] [diff] [review]
patch v3

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

Looks pretty good. r+ with few cosmetic changes.

::: mobile/android/base/DoorHanger.java
@@ +77,5 @@
>      public interface OnButtonClickListener {
>          public void onButtonClick(DoorHanger dh, String tag);
>      }
>  
> +    DoorHanger(Context context, Theme theme) {

public DoorHanger(..) { .. }
If you don't want DoorHanger to be initialized/accesible outside this package, give the class a package access and not public access.

@@ +83,3 @@
>      }
>  
>      DoorHanger(Context context, int tabId, String value) {

public DoorHanger(..) { .. }

@@ +85,5 @@
>      DoorHanger(Context context, int tabId, String value) {
> +        this(context, tabId, value, Theme.LIGHT);
> +    }
> +
> +    DoorHanger(Context context, int tabId, String value, Theme theme) {

private, unless you want to make this public.

@@ +116,5 @@
> +    private void setTheme(Theme theme) {
> +        if (theme == Theme.LIGHT) {
> +            // The default styles declared in doorhanger.xml are light-themed, so we just
> +            // need to set the divider color that we'll use in addButton.
> +            mDividerColor = getResources().getColor(R.color.doorhanger_divider_light);

Save the getResources() to a local variable. It's being used quite a few times.

@@ +118,5 @@
> +            // The default styles declared in doorhanger.xml are light-themed, so we just
> +            // need to set the divider color that we'll use in addButton.
> +            mDividerColor = getResources().getColor(R.color.doorhanger_divider_light);
> +        } else if (theme == Theme.DARK) {
> +            mDividerColor = getResources().getColor(R.color.doorhanger_divider_dark);

New line after this.

@@ +192,5 @@
>  
>          if (mChoicesLayout.getChildCount() == 0) {
>              // If this is the first button we're adding, make the choices layout visible.
>              mChoicesLayout.setVisibility(View.VISIBLE);
> +            // Make the divider above the buttons visible.

New line before this.

::: mobile/android/base/resources/values/dimens.xml
@@ +39,5 @@
>  
>      <dimen name="doorhanger_input_width">250dp</dimen>
>      <dimen name="doorhanger_spinner_textsize">9sp</dimen>
>      <dimen name="doorhanger_padding">15dp</dimen>
> +    <dimen name="doorhanger_textsize_small">8sp</dimen>

8sp???? :O :O :O That's going to be way too small.
Attachment #777240 - Flags: review?(sriram) → review+
(In reply to Sriram Ramasubramanian [:sriram] from comment #25)

> ::: mobile/android/base/resources/values/dimens.xml
> @@ +39,5 @@
> >  
> >      <dimen name="doorhanger_input_width">250dp</dimen>
> >      <dimen name="doorhanger_spinner_textsize">9sp</dimen>
> >      <dimen name="doorhanger_padding">15dp</dimen>
> > +    <dimen name="doorhanger_textsize_small">8sp</dimen>
> 
> 8sp???? :O :O :O That's going to be way too small.

That's the text size that results in this screenshot:
https://bug885962.bugzilla.mozilla.org/attachment.cgi?id=767486

Which looks fine. Do you think there's something weird going on that might be causing the text to be bigger than expected?
Pushed with the comments addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/111b3d84c479

We can investigate the textsize in a follow-up if it poses a problem, but from what I can tell, it's doing what we want (tested on a Nexus 4 and Nexus S).
Flags: needinfo?(mark.finkle)
(In reply to Wes Kocher (:KWierso) from comment #28)
> Backed out for breaking Android 2.2:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c3061aed82c9
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=25469511&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=25469542&tree=Mozilla-
> Inbound

It looks like this just needed a clobber build (it built for me locally)... I feel like in the past we would just kick those off manually, but now do I need to always update the CLOBBER file?
(In reply to :Margaret Leibovic from comment #29)
> It looks like this just needed a clobber build (it built for me locally)...
> I feel like in the past we would just kick those off manually, but now do I
> need to always update the CLOBBER file?

I think so, so people building locally will automatically clobber when they rebuild.
(In reply to Wes Kocher (:KWierso) from comment #30)
> (In reply to :Margaret Leibovic from comment #29)
> > It looks like this just needed a clobber build (it built for me locally)...
> > I feel like in the past we would just kick those off manually, but now do I
> > need to always update the CLOBBER file?
> 
> I think so, so people building locally will automatically clobber when they
> rebuild.

In the past I've done that and updated the comment to mention it's Android-only, but I still feel hesitant since people might not read that and end up clobbering when they don't need to (their fault I guess?). This error actually only needs you to clobber objdir/mobile/android/base/res. I guess I'll just update the CLOBBER file to say that and hope people read it.
Pushed again, this time with an update to the CLOBBER file:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36c92f3483e5
https://hg.mozilla.org/mozilla-central/rev/36c92f3483e5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
I should have commented in here a while ago... this shouldn't track 24 anymore, we decided to push the mixed content blocking release to 25:
https://wiki.mozilla.org/Mobile/Roadmap12-13#Firefox_25:_Something_for_Everyone_.28Nightly.29

And someone give me some tracking-fennec permissions! (please :)
tracking-fennec: 24+ → ?
tracking-fennec: ? → 25+
Status: RESOLVED → VERIFIED
Depends on: 917970
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.