Closed Bug 1124711 Opened 5 years ago Closed 5 years ago

Site identity popup overlaps the URL Bar on phone

Categories

(Firefox for Android :: General, defect)

38 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1136469
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: TeoVermesan, Assigned: ally)

References

Details

(Whiteboard: [wip])

Attachments

(2 files, 2 obsolete files)

Tested with:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 38.0a1 (2015-01-22)
Steps to reproduce:
1. Open a page (for example http://popuptest.com/popup1.html)
2. Tap on the favicon

Expected results:
- Site identity popup is displayed correctly.

Actual results:
- Site identity popup is displayed over the URL Bar

Note:
- not reproducible on Nexus 7(Android 5.0.2)  or Nexus 5 5.0.1
Is this a regression from bug 1107591? I suspect what's happening here is that the site identity popup is anchored to a hidden view, so it doesn't know how to position itself.

Maybe we should optionally pass in a view to the site identity popup as an anchor? If we do that, we could pass in the favicon.
Flags: needinfo?(ally)
Assignee: nobody → ally
Flags: needinfo?(ally)
Sounds like a regression, and if its not, its close enough that I should take it.
Attached patch regressionSiteIDOverlapsToolbar (obsolete) — Splinter Review
this works on both my phones. I haven't had a chance to try it with a tablet yet
Attachment #8554890 - Flags: review?(michael.l.comella)
Comment on attachment 8554890 [details] [diff] [review]
regressionSiteIDOverlapsToolbar

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

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +189,5 @@
>  
>          mSiteSecurityVisible = (mSiteSecurity.getVisibility() == View.VISIBLE);
>  
>          mSiteIdentityPopup = new SiteIdentityPopup(mActivity);
> +        mSiteIdentityPopup.setAnchor(mFavicon);

Will this make the arrow point at the favicon instead of at the lock?

Also, tablets don't have an mFavicon view, so I don't think this will work:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarDisplayLayout.java#182
Comment on attachment 8554890 [details] [diff] [review]
regressionSiteIDOverlapsToolbar

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

(In reply to :Margaret Leibovic from comment #4)
> Will this make the arrow point at the favicon instead of at the lock?

I'd think so - but I don't think I mind where it points.

> Also, tablets don't have an mFavicon view, so I don't think this will work:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/
> ToolbarDisplayLayout.java#182

I agree.

Sounds like a case where this is different on each device so you should consider using HardwareUtils.
Attachment #8554890 - Flags: review?(michael.l.comella) → feedback+
(In reply to Michael Comella (:mcomella) from comment #5)
> I'd think so - but I don't think I mind where it points.

Maybe check with :antlam?
please respond so I can move this forward. Thanks!
Flags: needinfo?(alam)
We're doing a lot of updates to these doorhangers so I'm not that picky about where it points right now. But it'd be nice if we could keep it pointing to the same place (before we land that other work) in addition to moving this back to the original position.
Flags: needinfo?(alam)
Status: NEW → ASSIGNED
Whiteboard: [shovel-ready]
Whiteboard: [shovel-ready] → [wip]
Whiteboard: [wip] → [shovel-ready]
Whiteboard: [shovel-ready] → [wip]
Attached patch regressionSiteIDOverlapsToolbar2 (obsolete) — Splinter Review
per antlam's request, the door hanger now rests in a static position below the url bar.

Appears to work on tablet, large phone, small phone.
Attachment #8554890 - Attachment is obsolete: true
Attachment #8561740 - Flags: review?(margaret.leibovic)
Comment on attachment 8561740 [details] [diff] [review]
regressionSiteIDOverlapsToolbar2

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

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +191,5 @@
>  
>          mSiteSecurityVisible = (mSiteSecurity.getVisibility() == View.VISIBLE);
>  
>          mSiteIdentityPopup = new SiteIdentityPopup(mActivity);
> +        ImageView SiteIdentityPopupAnchor = (ImageView)findViewById(R.id.url_bar_title);

We generally make local variables final where possible, and use lowercase for the first letter of local variable names.

@@ +192,5 @@
>          mSiteSecurityVisible = (mSiteSecurity.getVisibility() == View.VISIBLE);
>  
>          mSiteIdentityPopup = new SiteIdentityPopup(mActivity);
> +        ImageView SiteIdentityPopupAnchor = (ImageView)findViewById(R.id.url_bar_title);
> +        mSiteIdentityPopup.setAnchor(SiteIdentityPopupAnchor);

setAnchor doesn't require an ImageView, so no need to import ImageView and cast this (you can just use a View).

In fact, this cast isn't even safe, since url_bar_title is a TextView. We already have it stored as a member variable (mTitle), so you can just use that.
Attachment #8561740 - Flags: review?(margaret.leibovic) → review-
I didn't know java would let me *do* unmarked unsafe casts! Thanks for pointing that out.

Patch keeps getting smaller.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aff688934a2a
Attachment #8561740 - Attachment is obsolete: true
Attachment #8562334 - Flags: review?(margaret.leibovic)
Comment on attachment 8562334 [details] [diff] [review]
regressionSiteIDOverlapsToolbar4

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

Looks good, thanks.
Attachment #8562334 - Flags: review?(margaret.leibovic) → review+
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #11)
> I didn't know java would let me *do* unmarked unsafe casts! Thanks for
> pointing that out.

In general, if you're casting in Java, you should double-check that there isn't a better way (e.g. interfaces) - all casts in Java are unsafe (e.g. [1]) and may throw at runtime. However, there are a few places where exceptions are made such as dealing with Views in the Android framework, where having a findViewById for each potential View type is unreasonable and breaks when creating View subclasses (i.e. you'd have to cast anyway or somehow extend the framework).

[1]: http://stackoverflow.com/a/8649996
https://hg.mozilla.org/mozilla-central/rev/9e88eb0cb0d2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Depends on: 1133445
Requesting backout.

due to https://bugzilla.mozilla.org/show_bug.cgi?id=1133445#c8: Reviewer believes this should be backouted and reopened.
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---
I'm gone for the weekend. Please find someone on the mobile team to take care of the backout.
Flags: needinfo?(ryanvm)
Ok, sorry about that. I was told that we're no longer to do backouts, but to request the sheriffs do them. Sounds like that's not the case here.
 
https://hg.mozilla.org/integration/fx-team/rev/a652101ef37a
Thanks Wes.

favicon + lock -> align with lock
favicon  -> align with favicon
lock  -> align with lock
nothing (should not be possible; there'd be nothing to tap on to get the popup)

So if I were to put the two image buttons in a div together, we would want the arrow to point to the center of rightmost child.
liuche is actually working on a patch right now to get rid of this arrow altogether, so once that lands, we won't need to worry about the arrow position anymore. However, we'll still need to make sure the popup itself ends up in the correct place, so this bug will probably still be valid.

In any case, I think we should hold off on a fix here until we land the patch to remove the arrow.

liuche, can you link that bug here as a dependency?
Flags: needinfo?(liuche)
Depends on: 1136469
Flags: needinfo?(liuche)
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1136469
You need to log in before you can comment on or make changes to this bug.