Show site identity popup when clicking the favicon on phones

RESOLVED FIXED in Firefox 38



Firefox for Android
3 years ago
2 years ago


(Reporter: mcomella, Assigned: ally)


(Blocks: 1 bug, {feature})

Firefox 38

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

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).
No longer blocks: 1107590


3 years ago
Assignee: nobody → margaret.leibovic


3 years ago
Keywords: feature
Depends on: 1111818
See Also: → bug 1111818

Comment 1

3 years ago
Assignee: margaret.leibovic → ally
No longer depends on: 1111818

Comment 2

3 years ago
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.
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:

2. When initializing mSiteSecurity, we make it visible for tablets:

3. We ignore attempts to change the visibility for tablets:

4. That means the widget is always visible for tablets, and this check will always succeed:

Comment 4

3 years ago
Created attachment 8549958 [details] [diff] [review]

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.
Attachment #8549958 - Flags: review?(michael.l.comella)

Do we still need the mSiteSecurityVisible too?

Comment 6

3 years ago
Comment on attachment 8549958 [details] [diff] [review]

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:

::: 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

3 years ago
Created attachment 8552013 [details] [diff] [review]
v2- margaret/surgical version.
Attachment #8549958 - Attachment is obsolete: true
Attachment #8549958 - Flags: review?(michael.l.comella)
Attachment #8552013 - Flags: review?(michael.l.comella)
Comment on attachment 8552013 [details] [diff] [review]
v2- margaret/surgical version.

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

Attachment #8552013 - Flags: review?(michael.l.comella) → review+
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.