Show site identity popup when clicking the favicon on phones

RESOLVED FIXED in Firefox 38

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mcomella, Assigned: ally)

Tracking

(Blocks: 1 bug, {feature})

unspecified
Firefox 38
All
Android
feature
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(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

Updated

3 years ago
Assignee: nobody → margaret.leibovic

Updated

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

Comment 1

3 years ago
yoink!
Assignee: margaret.leibovic → ally
No longer depends on: 1111818
(Assignee)

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:
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
(Assignee)

Comment 4

3 years ago
Created attachment 8549958 [details] [diff] [review]
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.
Attachment #8549958 - Flags: review?(michael.l.comella)
Nice.

Do we still need the mSiteSecurityVisible too?

Comment 6

3 years ago
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.
(Assignee)

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+
https://hg.mozilla.org/mozilla-central/rev/9a389eb9609f
Status: NEW → RESOLVED
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.