If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Site identity popup overlaps the URL Bar on phone

RESOLVED DUPLICATE of bug 1136469

Status

()

Firefox for Android
General
RESOLVED DUPLICATE of bug 1136469
3 years ago
3 years ago

People

(Reporter: TeoVermesan, Assigned: ally)

Tracking

38 Branch
Firefox 38
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [wip])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8553136 [details]
Screenshot_2015-01-22-15-55-04.png

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

Comment 1

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

Updated

3 years ago
Assignee: nobody → ally
Flags: needinfo?(ally)
(Assignee)

Comment 2

3 years ago
Sounds like a regression, and if its not, its close enough that I should take it.
(Assignee)

Comment 3

3 years ago
Created attachment 8554890 [details] [diff] [review]
regressionSiteIDOverlapsToolbar

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 4

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

Comment 7

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

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Whiteboard: [shovel-ready]
(Assignee)

Updated

3 years ago
Whiteboard: [shovel-ready] → [wip]
(Assignee)

Updated

3 years ago
Whiteboard: [wip] → [shovel-ready]
(Assignee)

Updated

3 years ago
Whiteboard: [shovel-ready] → [wip]
(Assignee)

Comment 9

3 years ago
Created attachment 8561740 [details] [diff] [review]
regressionSiteIDOverlapsToolbar2

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 10

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

Comment 11

3 years ago
Created attachment 8562334 [details] [diff] [review]
regressionSiteIDOverlapsToolbar4

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 12

3 years ago
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
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38

Updated

3 years ago
Depends on: 1133445
(Assignee)

Comment 15

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

Comment 17

3 years ago
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
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a652101ef37a
(Assignee)

Comment 19

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

Comment 20

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

Updated

3 years ago
Depends on: 1136469
Flags: needinfo?(liuche)

Updated

3 years ago
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1136469
You need to log in before you can comment on or make changes to this bug.