751205 - Refactor site identity popup and doorhanger popup to use common arrow panel container

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

The site identity popup added in bug 695204 just copied the arrow panel styles from the doorhanger popup. It would be nice if they could use shared XML.

Comment 1

[:Margaret Leibovic]

Looks like Sriram is doing this in bug 850927.

Comment 2

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #1)
> Looks like Sriram is doing this in bug 850927.
> 
> *** This bug has been marked as a duplicate of bug 850927 ***

The other bug is trying to use one XML for phones and tablets. It's not factoring out the common XML stuff between doorhangers and site-identity popups. I feel this is a bug good to be fixed separately. Even the custom menu has a similar container with a pointed arrow.

Comment 3

[:Margaret Leibovic]

Attachment: patch

This issue really started bothering me while working on bug 860581, so I decided to finally fix this bug!

There's definitely more that we can share. I think it would be cool if we could totally combine DoorHangerPopup and SiteIdentityPopup into one thing, especially to do things like add a notification to the site identity popup for mixed content blocking. But that will require more work, including things like dynamically changing where we put the arrow, and changing the logic around creating/showing these things.

However, I decided it's best to just do one small thing at a time and not try to fix the whole world at once :)

(I also want to clean up the names of things like the attribute/dimension names used in the layout files to make them not depend on being a "doorhanger")

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=897fcc35aedb

Comment 4

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 760724 [details] [diff] [review]
patch

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

Drive by comments.

::: mobile/android/base/SiteIdentityPopup.java
@@ +43,3 @@
>  
>      private SiteIdentityPopup() {
>          super(GeckoAppShell.getContext());

I don't find a need for having this a singleton. We could give it a context from an activity.

@@ +64,4 @@
>  
>          // Make the popup focusable so it doesn't inadvertently trigger click events elsewhere
>          // which may reshow the popup (see bug 785156)
>          setFocusable(true);

DoorHangerPopup is also made Focusable. May be this can be factored out.

@@ +65,5 @@
>          // Make the popup focusable so it doesn't inadvertently trigger click events elsewhere
>          // which may reshow the popup (see bug 785156)
>          setFocusable(true);
>  
> +        LinearLayout layout = (LinearLayout) mInflater.inflate(R.layout.site_identity, null);

mInflater seems unnecessary when there is a context.

::: mobile/android/base/resources/layout/arrow_popup.xml
@@ +14,5 @@
> +
> +        <LinearLayout android:id="@+id/content"
> +                      android:layout_width="fill_parent"
> +                      android:layout_height="wrap_content"
> +                      android:orientation="vertical"/>

Is this LinearLayout needed? Can't the content be added directly to the ScrollView?

::: mobile/android/base/widget/ArrowPopup.java
@@ +19,5 @@
> +import android.widget.RelativeLayout;
> +
> +public class ArrowPopup extends PopupWindow {
> +    protected LayoutInflater mInflater;
> +    protected boolean mInflated; 

White space.

@@ +25,5 @@
> +    protected LinearLayout mContent;
> +    protected ImageView mArrow;
> +
> +    protected int mArrowWidth;
> +    protected int mYOffset;

Add methods "addContentView(), setArrowWidth(), setPopupOffset()". That's better than exposing member variables as protected.

@@ +50,5 @@
> +        setContentView(layout);
> +
> +        mArrow = (ImageView) layout.findViewById(R.id.arrow);
> +        mContent = (LinearLayout) layout.findViewById(R.id.content);
> +        

Whitespace.

Comment 5

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #4)

Thanks for the comments!

> ::: mobile/android/base/SiteIdentityPopup.java
> @@ +43,3 @@
> >  
> >      private SiteIdentityPopup() {
> >          super(GeckoAppShell.getContext());
> 
> I don't find a need for having this a singleton. We could give it a context
> from an activity.

Yeah, definitely. I'm not quite sure why we did that in the first place. I plan to file a new bug about that.

> @@ +64,4 @@
> >  
> >          // Make the popup focusable so it doesn't inadvertently trigger click events elsewhere
> >          // which may reshow the popup (see bug 785156)
> >          setFocusable(true);
> 
> DoorHangerPopup is also made Focusable. May be this can be factored out.

There was a weird bug about this that caused a crash, so I wanted to leave the current logic separated, then deal with that in a different bug: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DoorHangerPopup.java#313

> @@ +65,5 @@
> >          // Make the popup focusable so it doesn't inadvertently trigger click events elsewhere
> >          // which may reshow the popup (see bug 785156)
> >          setFocusable(true);
> >  
> > +        LinearLayout layout = (LinearLayout) mInflater.inflate(R.layout.site_identity, null);
> 
> mInflater seems unnecessary when there is a context.

I think I decided to do this because we're not currently saving the context in ArrowPopup/SiteIdentityPopup. The old code in SiteIdentityPopup would actually grab the context from GeckoAppShell when it needed an inflater. Maybe this is something we can deal with in the un-singleton-ify bug.

> ::: mobile/android/base/resources/layout/arrow_popup.xml
> @@ +14,5 @@
> > +
> > +        <LinearLayout android:id="@+id/content"
> > +                      android:layout_width="fill_parent"
> > +                      android:layout_height="wrap_content"
> > +                      android:orientation="vertical"/>
> 
> Is this LinearLayout needed? Can't the content be added directly to the
> ScrollView?

This is just copied over from the existing doorhangerpopup.xml. I was trying to maintain the current state of things as much as possible, but I agree this can be better though, since there's now an extra LinearLayout in site_identity.xml. 

> @@ +25,5 @@
> > +    protected LinearLayout mContent;
> > +    protected ImageView mArrow;
> > +
> > +    protected int mArrowWidth;
> > +    protected int mYOffset;
> 
> Add methods "addContentView(), setArrowWidth(), setPopupOffset()". That's
> better than exposing member variables as protected.

I did this just to pull common shared things out of DoorHangerPopup and SiteIdentityPopup instead of declaring them twice, but I can actually see a future where the logic that uses these variables is pulled out into methods on ArrowPopup. So instead of needing getters/setters, we could just have a method like show() that does the arrow positioning stuff itself (there is definitely some more copypasta around this in DoorHangerPopup/SiteIdentityPopup, but it's slightly different enough that I think that change should just happen in a different bug).

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 760724 [details] [diff] [review]
patch

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

I wonder if we can rename DoorHangerPopup.show to DoorHangerPopup.update and the combine some of the logic there as well. From the int[] anchorLocation = new int[2]; they both look different, but most of those differences shouldn't be there I think.... The only one that we might need is the max doorhanger width stuff? And even that seems like it should actually just be a style property that we read and use....

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 760724 [details] [diff] [review]
patch

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

I like this a lot, but want it to go further. A separate patch for that seems fine to me.

For instance, I wonder if we can combine DoorHangerPopup.update and SiteIdentity.show. But to be honest, I think we'd want to factor out some of the popup specific stuff in show and make it an abstract method, prepareContents or something? That way we could move the:

if (!mInflated)
  init();

as well as all the arrow positioning logic into the parent class. Right now its slightly different between the two, but I'm not sure why that is.

::: mobile/android/base/DoorHangerPopup.java
@@ +27,1 @@
>      private View mAnchor;

I kinda think we should just move these two things to ArrowPopup here. Maybe it would be easy to (for now) override (SiteIdentity?)Popup.show(View v) to do:

mAnchor = v;
show();
mAnchor = null; // ?

::: mobile/android/base/resources/layout/arrow_popup.xml
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +                android:layout_width="?attr/doorhangerWidth"

Rename this attribute and other doorhanger ones in here.

@@ +19,5 @@
> +
> +    </ScrollView>
> +
> +    <ImageView android:id="@+id/arrow"
> +               android:layout_width="@dimen/menu_popup_arrow_width"

Likewise, we should rename this attribute and drawable to something more generic.

Comment 8

[:Margaret Leibovic]

Attachment: (Part 2) Un-singleton-ify SiteIdentityPopup

I decided to just do this sooner rather than later, because it made my next patch easier to write.

We can probably improve upon this, but for right now I just wanted to kill the SiteIdentityPopup singleton, and let GeckoApp forget about SiteIdentityPopup.

Comment 9

[:Margaret Leibovic]

Attachment: (Part 3) Factor out arrow/anchor positioning code to ArrowPopup

I don't have a tablet with me to test this, but on IRC jfkthame told me that this was looking good on his tablet. You should test it to make sure :)

But, if it works well on tablets, I'm pretty proud of this patch. I was able to munge through the DoorHangerPopup and SiteIdentityPopup logic to figure out what they were aiming to do, and I came up with a more robust solution based on that. I tried commenting the logic a lot, hopefully it's understandable.

Basically, before, for the DoorHangerPopup, we were relying on a hardcoded left margin on the arrow to position it, but we were dynamically setting that margin for the SiteIdentityPopup.

Comment 10

[:Margaret Leibovic]

In case you don't feel like applying the patches yourself:
https://dl.dropboxusercontent.com/u/3358452/fennec-24.0a1.en-US.android-arm.apk

Comment 11

[:Margaret Leibovic]

Comment on attachment 762082 [details] [diff] [review]
(Part 3) Factor out arrow/anchor positioning code to ArrowPopup

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

::: mobile/android/base/widget/ArrowPopup.java
@@ +25,5 @@
> +    private View mAnchor;
> +    private ImageView mArrow;
> +
> +    private int mArrowWidth;
> +    private int mPopupWidth;

Oops, I didn't end up needing this. I'm getting rid of it locally.

Comment 12

[:Margaret Leibovic]

Attachment: (Part 4) Rename resources to be more generic for arrow popups

We can leave the renaming of menu_popup_arrow_ stuff for another bug, I've had enough hg rename for one week :)

Comment 13

[:Margaret Leibovic]

Attachment: (Part 4) Rename resources to be more generic for arrow popups

Oops, forgot to update dimens.xml

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 762076 [details] [diff] [review]
(Part 2) Un-singleton-ify SiteIdentityPopup

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

::: mobile/android/base/BrowserApp.java
@@ +1751,5 @@
>  
>      @Override
> +    public void onResume() {
> +        if (mSiteIdentityPopup != null)
> +            mSiteIdentityPopup.dismiss();

I think we can just put this in refreshChrome(). Its called for both onResume and onConfigurationChanged to handle things we do in both cases.

::: mobile/android/base/BrowserToolbar.java
@@ +290,5 @@
>              public void onClick(View view) {
>                  if (mSiteSecurity.getVisibility() != View.VISIBLE)
>                      return;
>  
> +                mActivity.getSiteIdentityPopup().show(mSiteSecurity);

Hmm.. Maybe we should just expose mFavicon and the listener should be in BrowserApp. i.e. BrowserToolbar.addFaviconClickListener(...) ? Follow ups I guess...

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 762082 [details] [diff] [review]
(Part 3) Factor out arrow/anchor positioning code to ArrowPopup

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

There's a little problem with the positioning here that I'd like to get right, but I know that's easier said than done. I wonder if we can steal desktop's arrow popup code for it, although I'd be fine if we only allowed positioning this on the top left side of things for now (i.e. we should be able to do something 1000x simpler than they do).

::: mobile/android/base/SiteIdentityPopup.java
@@ +122,5 @@
>              mHost.setTextColor(mResources.getColor(R.color.identity_identified));
>              mOwner.setTextColor(mResources.getColor(R.color.identity_identified));
>          }
>  
> +        showPopup();

I don't really like that a method named updatePopup also shows the popup. They seem like orthogonal actions.

::: mobile/android/base/widget/ArrowPopup.java
@@ +33,3 @@
>      protected boolean mInflated;
>  
> +    public ArrowPopup(GeckoApp aActivity, View aAnchor) {

I wonder if an overloaded constructor without the anchor would make it clearer to us in the future that having an anchor isn't required.

@@ +79,5 @@
> +
> +        // If there's no anchor or the anchor is out of the window bounds,
> +        // just show the popup at the top of the gecko app view.
> +        if (mAnchor == null || anchorLocation[1] < 0) {
> +            showAtLocation(mActivity.getView(), Gravity.TOP, 0, 0);

At one point I tried hiding the arrow in this case as well, but I think I ran into some issue and never came back to it.

@@ +92,5 @@
> +        int anchorWidth = mAnchor.getWidth() - mAnchor.getPaddingLeft() - mAnchor.getPaddingRight();
> +        int arrowOffset = (anchorWidth - mArrowWidth)/2;
> +
> +        // Account for anchor padding again to make the arrow point to the center of the anchor icon.
> +        int leftMargin = anchorLocation[0] + arrowOffset + mAnchor.getPaddingLeft() - mAnchor.getPaddingRight();

Do you need padding right here? This to me looks like it should be (pardon my lack of ASCII art skillz in bugzilla.

|----------------------------------------|------------ANCHOR------------|
|-anchorLocation-|-paddingLeft-|-arrowOffset-|

@@ +99,5 @@
> +        layoutParams.setMargins(leftMargin, 0, 0, 0);
> +
> +        // On tablets, we need to give the popup itself a horizontal offset because it does not stretch
> +        // across the screen. Remember, this offset is relative to the left side of the anchor view.
> +        int offset = HardwareUtils.isTablet() ? arrowOffset - leftMargin : 0;

We're basically removing the offset for the anchorLocation and paddingLeft (and paddingRight, but I think that's a bug) here. If the favicon was for some reason further from the left edge of the device than the popup's width, we'd hit a weird case where the arrow was not touching the popup.

It might be better to position this popup and then calculate the arrow position based on the popup's left hand side and the anchors left hand side.

Comment 16

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #15)

> There's a little problem with the positioning here that I'd like to get
> right, but I know that's easier said than done. I wonder if we can steal
> desktop's arrow popup code for it, although I'd be fine if we only allowed
> positioning this on the top left side of things for now (i.e. we should be
> able to do something 1000x simpler than they do).

As I mentioned on IRC, the desktop arrows are basically CSS hacks, we don't want to model ourselves after that :)

> ::: mobile/android/base/SiteIdentityPopup.java
> @@ +122,5 @@
> >              mHost.setTextColor(mResources.getColor(R.color.identity_identified));
> >              mOwner.setTextColor(mResources.getColor(R.color.identity_identified));
> >          }
> >  
> > +        showPopup();
> 
> I don't really like that a method named updatePopup also shows the popup.
> They seem like orthogonal actions.

Yeah, I was just following along with what we currently have in DoorHangerPopup. Maybe we should rename that method updateAndShow, although it sometimes doesn't show the popup. What's a good name for a method that updates the contents of the popup and maybe shows it if it's supposed to (called all over the place, like when tabs change or we add a new doorhanger notification)?

I'll hold SiteIdentityPopup to higher standards, and we can look into updating DoorHangerPopup in a separate bug (or maybe here if it ends up not being too much effot).

> ::: mobile/android/base/widget/ArrowPopup.java
> @@ +33,3 @@
> >      protected boolean mInflated;
> >  
> > +    public ArrowPopup(GeckoApp aActivity, View aAnchor) {
> 
> I wonder if an overloaded constructor without the anchor would make it
> clearer to us in the future that having an anchor isn't required.

Sure, that sounds good. I can also add some documentation here about what this is used for.

> @@ +79,5 @@
> > +
> > +        // If there's no anchor or the anchor is out of the window bounds,
> > +        // just show the popup at the top of the gecko app view.
> > +        if (mAnchor == null || anchorLocation[1] < 0) {
> > +            showAtLocation(mActivity.getView(), Gravity.TOP, 0, 0);
> 
> At one point I tried hiding the arrow in this case as well, but I think I
> ran into some issue and never came back to it.

Sounds like follow-up material. Although then it wouldn't be an "arrow" popup anymore ;)

> @@ +92,5 @@
> > +        int anchorWidth = mAnchor.getWidth() - mAnchor.getPaddingLeft() - mAnchor.getPaddingRight();
> > +        int arrowOffset = (anchorWidth - mArrowWidth)/2;
> > +
> > +        // Account for anchor padding again to make the arrow point to the center of the anchor icon.
> > +        int leftMargin = anchorLocation[0] + arrowOffset + mAnchor.getPaddingLeft() - mAnchor.getPaddingRight();
> 
> Do you need padding right here? This to me looks like it should be (pardon
> my lack of ASCII art skillz in bugzilla.
> 
> |----------------------------------------|------------ANCHOR------------|
> |-anchorLocation-|-paddingLeft-|-arrowOffset-|

Yeah, I must have been tired, you're correct that we only care about stuff going on on the left side. But we do still need to take it into account above where we calculate the width.

> @@ +99,5 @@
> > +        layoutParams.setMargins(leftMargin, 0, 0, 0);
> > +
> > +        // On tablets, we need to give the popup itself a horizontal offset because it does not stretch
> > +        // across the screen. Remember, this offset is relative to the left side of the anchor view.
> > +        int offset = HardwareUtils.isTablet() ? arrowOffset - leftMargin : 0;
> 
> We're basically removing the offset for the anchorLocation and paddingLeft
> (and paddingRight, but I think that's a bug) here. If the favicon was for
> some reason further from the left edge of the device than the popup's width,
> we'd hit a weird case where the arrow was not touching the popup.
> 
> It might be better to position this popup and then calculate the arrow
> position based on the popup's left hand side and the anchors left hand side.

My brain always starts to hurt when thinking about these offsets. When passing an offset to showAsDropDown, that offset is relative to the anchor, so this would imply that we would always have constant offset for the tablet case, although we'd want to start to do something creative if the popup is going to go off the right edge of the screen. Actually, are you sure that the issue you mentioned exists? The fact that the offset is relative to the anchor makes me think that it's not. I'm going to think about this more.

Comment 17

[:Margaret Leibovic]

Attachment: (Part 3 v2) Factor out arrow/anchor positioning code to ArrowPopup

Here's a better patch that makes a clearer distinction between what's happening on tablets vs. phones.

Upped the arrow inset for tablets, as per ibarlow's request.

Comment 18

[:Margaret Leibovic]

Attachment: (Part 3 v3) Factor out arrow/anchor positioning code to ArrowPopup

This is better, factors out the anchor padding into arrowOffset.

Comment 19

[:Margaret Leibovic]

Pushed the most recent patches to try:
https://tbpl.mozilla.org/?tree=Try&rev=45233b9f6a0f

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 763762 [details] [diff] [review]
(Part 3 v3) Factor out arrow/anchor positioning code to ArrowPopup

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

Nice!

::: mobile/android/base/resources/layout/arrow_popup.xml
@@ +21,5 @@
>  
>      <ImageView android:id="@+id/arrow"
>                 android:layout_width="@dimen/menu_popup_arrow_width"
>                 android:layout_height="12dip"
> +               android:layout_marginLeft="20dip"

Ideally I think we put these all dimens.xml, but I think sriram's policy is we only do that when the numbers are reused.

Comment 21

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe598caed73
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b17a76cd79a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1433cdfd1a43
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb01c197c682

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bfe598caed73
https://hg.mozilla.org/mozilla-central/rev/6b17a76cd79a
https://hg.mozilla.org/mozilla-central/rev/1433cdfd1a43
https://hg.mozilla.org/mozilla-central/rev/cb01c197c682