1071267 - Revise door hanger behavior in new tablet toolbar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The v1 solution, via IRC: <yuan> I think the solution makes sense to me: show a globe icon and replace with a security icon when it's secured.

See [1] for the current undesired behavior.

[1]: https://bug1066253.bugzilla.mozilla.org/attachment.cgi?id=8488379

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Remove padding inside site security assets

I removed the padding from the site security assets because it gets in the way
when using the globe asset (which has no padding in the asset). To do this, I
ran the following command:

for DPI in {mdpi,hdpi,xhdpi}; do
  for file in {lock_verified,lock_identified,shield,warning}; do
    pushd drawable-${DPI}/ && convert ${file}.png -trim tmp && mv tmp ${file}.png; popd;
  done;
done

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8494198 [details] [diff] [review]
Part 1: Remove padding inside site security assets

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

YES.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Always display site security icon on new tablet

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note that the patch in comment 3 needs stubbing but because bug 1073474 hasn't landed yet, I think that bug will take care of this.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Toolbar, no doorhanger, not secure

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Toolbar, no doorhanger, secure

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Toolbar, doorhanger, not secure

Anthony, what do you think?

One issue is that the hit area of the security icon is reduced because the favicon can no longer add to that hit area. Any ideas on what to do there?

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

An APK if you'd like to try it out: https://people.mozilla.com/~mcomella/apks/mcomella-1071267_01.apk

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8497875 [details] [diff] [review]
Part 2: Always display site security icon on new tablet

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

Let's simply get images that work without the margin hacks.

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +433,5 @@
>  
> +    /**
> +     * Updates the margins of the site security image to properly align it with other toolbar
> +     * elements. This method is necessary because the unknown security icon and identified icons
> +     * have a different shape.

This looks a bit too fragile. I'd prefer to have image that always look good with margin hacks -- even if this involves adding a bottom margin to one of the images.

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8497878 [details]
Toolbar, doorhanger, not secure

+ for V1. 

We'll have to iterate on this for sure. But I think we should spend some more time on this rather than coming up with an interim solution for V1. :)

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Always display site security icon on new tablet

New solution without dynamically changing and editing the images!

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8498487 [details] [diff] [review]
Part 2: Always display site security icon on new tablet

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

Fair enough, but I still think the assets should align properly without any inset hacks. Have you talked to antlam about this?

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Lucas Rocha (:lucasr) from comment #12)
> Comment on attachment 8498487 [details] [diff] [review]
> Part 2: Always display site security icon on new tablet
> 
> Review of attachment 8498487 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fair enough, but I still think the assets should align properly without any
> inset hacks. Have you talked to antlam about this?

We chatted about this on IRC just now. The globe in the URL bar is a bit small.

But there seems to be some weird resizing going on here..

Comment 14

[Lucas Rocha (:lucasr)]

Maybe this is just about ensuring the image is centered in the image view?

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #13)
> We chatted about this on IRC just now. The globe in the URL bar is a bit
> small.

I made a mistake - I told Anthony the width was 20dp when it is actually 12dp - he requested 18dp x 18dp. I'm not sure what this means for the lock icon, which is 12dp x 15dp (assuming we're keeping the proportions, should it be 18dp width or 18dp height?). Since we're scaling up, we likely need new resources. I'll put together a temporary scaled version in the meantime as Anthony is on PTO.

> But there seems to be some weird resizing going on here..

Likely explained above.

(In reply to Lucas Rocha (:lucasr) from comment #14)
> Maybe this is just about ensuring the image is centered in the image view?

Appears to be centered ("fitCenter").

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #15)
> (In reply to Anthony Lam (:antlam) from comment #13)
> > We chatted about this on IRC just now. The globe in the URL bar is a bit
> > small.
> 
> I made a mistake - I told Anthony the width was 20dp when it is actually
> 12dp - he requested 18dp x 18dp. I'm not sure what this means for the lock
> icon, which is 12dp x 15dp (assuming we're keeping the proportions, should
> it be 18dp width or 18dp height?). Since we're scaling up, we likely need
> new resources. I'll put together a temporary scaled version in the meantime
> as Anthony is on PTO.

Sounds good Michael. NI me on that new bug for the assets and I can get those to you later. I thought it was scaling 'up'! But it's ok, we'll figure something out :)

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Doorhanger + lock, 18dp x 18dp

The doorhanger got pushed down a little bit, so we'll have to re-adjust that position, but here's the 18dp x 18dp I think you wanted, Anthony.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Globe, 18dp x 18dp

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (WIP, fold part 2) 18dp icons

I wrote the 18dp patch, like I think Anthony wanted. This should fold into part 2.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note that the patch in comment 19 still includes the inset drawable to compensate for padding changes and scales up.

Do you think we should fold and land the 18dp version, Lucas? Or should we wait for :antlam to return?

Comment 21

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #20)
> Note that the patch in comment 19 still includes the inset drawable to
> compensate for padding changes and scales up.
> 
> Do you think we should fold and land the 18dp version, Lucas? Or should we
> wait for :antlam to return?

Let's land and file a follow-up to fix this once antlam is back. No need to block on this.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8500746 [details] [diff] [review]
(WIP, fold part 2) 18dp icons

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

Looks like antlam likes it! :P

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I'll fold it into part 2, unless you disagree.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Create large tappable margin for site security

Larger padding/margin, to spec (8dp left side, 9dp right), which creates a larger tapable area. Waiting for antlam feedback before review.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Lock, margins to spec

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Globe, margins to spec

Comment 27

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8501325 [details]
Lock, margins to spec

nice!

Comment 28

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #26)
> Created attachment 8501326 [details]
> Globe, margins to spec

looks good too!

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 8500746 [details] [diff] [review]
(WIP, fold part 2) 18dp icons

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

Not looking super-clean but this is the price you pay for keeping a common ToolbarDisplayLayout implementation for both phones, and new/old tablets.

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +158,5 @@
> +            // TODO: This can likely be set statically in resources when new tablet is default.
> +            // Dynamically update parameters for new tablet.
> +            final LinearLayout.LayoutParams lp =
> +                    (LinearLayout.LayoutParams) mSiteSecurity.getLayoutParams();
> +            lp.height = res.getDimensionPixelSize(R.dimen.new_tablet_site_security_height);

You shouldn't really need to explicitly set the height here. You can probably just rely on match_parent.

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 8501323 [details] [diff] [review]
Part 3: Create large tappable margin for site security

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

Ok.

::: mobile/android/base/newtablet/res/layout-large-v11/new_tablet_browser_toolbar.xml
@@ +43,5 @@
>                    android:layout_toRightOf="@id/back"
>                    android:layout_toLeftOf="@id/menu_items"/>
>  
> +    <!-- Note: * Values of marginLeft are used to animate the forward button so don't change its value.
> +               * We set the padding on the site security icon to increase it's -->

to increase its tappable area?

@@ +48,5 @@
>      <org.mozilla.gecko.toolbar.ToolbarDisplayLayout android:id="@+id/display_layout"
>                    style="@style/UrlBar.Button.Container"
>                    android:layout_toRightOf="@id/back"
>                    android:layout_toLeftOf="@id/menu_items"
> +                  android:paddingLeft="0dip"

No need to set paddingLeft then.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: match_parent on site security

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #29)
> ::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
> > +            lp.height = res.getDimensionPixelSize(R.dimen.new_tablet_site_security_height);
> 
> You shouldn't really need to explicitly set the height here. You can
> probably just rely on match_parent.

The icon is really small when I remove the line - see comment 31.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (Folded) Part 2: Always display site security icon on new tablet

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Create large tappable margin for site security

Addressed nits (comment 30).

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/2de38d704855
https://hg.mozilla.org/integration/fx-team/rev/6e02d0f5d642
https://hg.mozilla.org/integration/fx-team/rev/700ef0cb9a6c

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2de38d704855
https://hg.mozilla.org/mozilla-central/rev/6e02d0f5d642
https://hg.mozilla.org/mozilla-central/rev/700ef0cb9a6c