Closed Bug 1071267 Opened 5 years ago Closed 5 years ago

Revise door hanger behavior in new tablet toolbar

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(13 files, 2 obsolete files)

13.43 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
294.72 KB, image/png
Details
569.13 KB, image/png
Details
946.28 KB, image/png
antlam
: feedback+
Details
8.48 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
277.53 KB, image/png
antlam
: feedback+
Details
353.24 KB, image/png
Details
9.24 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
571.43 KB, image/png
antlam
: feedback+
Details
416.99 KB, image/png
Details
358.02 KB, image/png
Details
12.85 KB, patch
Details | Diff | Splinter Review
5.74 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
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
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
Attachment #8494198 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8494198 [details] [diff] [review]
Part 1: Remove padding inside site security assets

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

YES.
Attachment #8494198 - Flags: review?(lucasr.at.mozilla) → review+
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.
Status: NEW → ASSIGNED
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?
Attachment #8497878 - Flags: feedback?(alam)
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.
Attachment #8497875 - Flags: review?(lucasr.at.mozilla) → feedback+
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. :)
Attachment #8497878 - Flags: feedback?(alam) → feedback+
New solution without dynamically changing and editing the images!
Attachment #8498487 - Flags: review?(lucasr.at.mozilla)
Attachment #8497875 - Attachment is obsolete: true
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?
Attachment #8498487 - Flags: review?(lucasr.at.mozilla) → review+
(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..
Maybe this is just about ensuring the image is centered in the image view?
(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").
(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 :)
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.
Attachment #8500740 - Flags: feedback?(alam)
I wrote the 18dp patch, like I think Anthony wanted. This should fold into part 2.
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?
Flags: needinfo?(lucasr.at.mozilla)
Attachment #8500740 - Flags: feedback?(alam) → feedback+
(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.
Flags: needinfo?(lucasr.at.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
Attachment #8500746 - Flags: review?(lucasr.at.mozilla)
I'll fold it into part 2, unless you disagree.
Larger padding/margin, to spec (8dp left side, 9dp right), which creates a larger tapable area. Waiting for antlam feedback before review.
Comment on attachment 8501325 [details]
Lock, margins to spec

nice!
Attachment #8501325 - Flags: feedback?(alam) → feedback+
(In reply to Michael Comella (:mcomella) from comment #26)
> Created attachment 8501326 [details]
> Globe, margins to spec

looks good too!
Attachment #8501323 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8500746 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8501323 - Flags: review?(lucasr.at.mozilla) → review+
(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.
Attachment #8498487 - Attachment is obsolete: true
Attachment #8501323 - Attachment is obsolete: true
Attachment #8498487 - Attachment is obsolete: false
QA Contact: ioana.chiorean
You need to log in before you can comment on or make changes to this bug.