1107591 - Show site identity popup when clicking the favicon on phones

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

If we want to put site settings in the site identity popup (as per bug 1107590), we should have the ability to show the site identity popup all the time on phones.

This is slightly unintuitive, but better than nothing. Perhaps the option to show site identity information should be duplicated in the Settings menu (which would just hide the menu and open the site identity doorhanger).

Comment 1

[Allison Naaktgeboren :ally]

yoink!

Comment 2

[Allison Naaktgeboren :ally]

So far this comes down to mSiteSecurity.getVisibility() returns 8 (GONE) on phones instead of 0 (VISIBLE) on tablets(desired behavior).

The debugger keeps trying to take me a decompiled class file of View, whose getVisibility() function is to throw an exception. So clearly that's not actually the function being run on the call to getVisiblity.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

A search for mSiteSecurity and site_security help give us some clues:

1. The widget is default to visibility=gone in the XML layout used by all devices:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/toolbar_display_layout.xml#26

2. When initializing mSiteSecurity, we make it visible for tablets:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarDisplayLayout.java#160

3. We ignore attempts to change the visibility for tablets:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarDisplayLayout.java#505

4. That means the widget is always visible for tablets, and this check will always succeed:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarDisplayLayout.java#206

Comment 4

[Allison Naaktgeboren :ally]

Attachment: firstpatch

Since we want to ignore the check to change the visibility on phones like we do on tablets, I took that function out as it had no more purpose. Likewise the line in the UI block setting it to visible. Now everyone starts 'visible' and stays that way producing the desired behavior that the larry popup is available at all times.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Nice.

Do we still need the mSiteSecurityVisible too?

Comment 6

[:Margaret Leibovic]

Comment on attachment 8549958 [details] [diff] [review]
firstpatch

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

I'm not sure this patch is correct... We still want to hide the site security icon on phones in the non-SSL case, right? Won't this add an extra blank space next to the favicon if we never hide the ImageButton view?

To fix this bug, I was thinking we could just get rid of the visibility check in the favicon click handler here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarDisplayLayout.java#206

::: mobile/android/base/resources/layout/toolbar_display_layout.xml
@@ +22,5 @@
>                   android:layout_marginRight="4dip"
>                   android:layout_marginBottom="@dimen/site_security_bottom_margin"
>                   android:src="@drawable/site_security_level"
>                   android:contentDescription="@string/site_security"
> +                 android:visibility="visible"/>

Visible is the default case, so you could just remove this attribute declaration altogether.

Comment 7

[Allison Naaktgeboren :ally]

Attachment: v2- margaret/surgical version.

Comment 8

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

Comment on attachment 8552013 [details] [diff] [review]
v2- margaret/surgical version.

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

^-^

Comment 9

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/fx-team/rev/9a389eb9609f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a389eb9609f