1139551 - Doorhanger anchor position

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Doorhanger positioning

Doorhanger should be centered horizontally on phones, and anchored beneath the back arrow on tablets. The width should be 300dp.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Not sure how we're doing this, bit i've also overlapped the toolbar at 12dp (as measured from the bottom of the toolbar).

Comment 2

[Allison Naaktgeboren :ally]

(In reply to Anthony Lam (:antlam) from comment #1)
> Not sure how we're doing this, bit i've also overlapped the toolbar at 12dp
> (as measured from the bottom of the toolbar).

What do you mean by overlapped?

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

The way Chenxia currently has it in Nightly is correct.

But, we still need to horizontally center the entire doorhanger.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Should we center the doorhanger, or just make it wider? I'd rather make it a bit wider, and only center it once we hit a max width like we do for tablets.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Should we center the doorhanger, or just make it wider? I'd rather make it a
> bit wider, and only center it once we hit a max width like we do for tablets.

Centering it is the current plan. It's designed to be the same width as the Share overlay (300dp) and Chenxia said it was that width currently so centering it should be good.

Yes, for Tablets it would keep at this width, but left align (with specific margins on the left, 15dp I think is the spec)

Comment 6

[Chenxia Liu [:liuche]]

This should be two patches, one that centers the doorhanger, and one that handles the special casing on tablets. Ally, I can review those separately, so you should feel free to upload the first part and flag me for review.

Antlam, Ally was asking if you would be okay with just the centering on both phone and tablets (versus anchoring below the back button on tablets).

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Centering on Tablets was awkward from my mock but I could be convinced :)

Can I see a screenshot?

Comment 8

[Allison Naaktgeboren :ally]

Attachment: justcenterdoorhangeronphones

...the showasdropdown() w/ -1 for x offset (which is supposed to center on the x-axis) is no longer working on phones. It was last week before I left on pto. ?!?! :/

Comment 9

[Allison Naaktgeboren :ally]

Attachment: centerDoorhanger_originalPatch

Comment 10

[Allison Naaktgeboren :ally]

since _that_ has stopped working, I don't even have that to offer :/

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Patch: Center doorhanger

This should have the dimensions correct for phones and tablets. I didn't check on tiny devices.

This doesn't work on GB devices (though I started making some changes), and doesn't do the tablet back-button anchor.

Comment 12

[Allison Naaktgeboren :ally]

Attachment: centerdoorhanger + GB

liuche: I was wondering why you changed the y offset. It broke the original gb code.

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8592055 [details] [diff] [review]
centerdoorhanger + GB

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

::: mobile/android/base/resources/values/dimens.xml
@@ +103,5 @@
>      <dimen name="doorhanger_input_width">250dp</dimen>
>      <dimen name="doorhanger_spinner_textsize">9sp</dimen>
>      <dimen name="doorhanger_padding">15dp</dimen>
>      <dimen name="doorhanger_offsetX">10dp</dimen>
> +    <dimen name="doorhanger_offsetY">66dp</dimen>

Based on feedback from antlam, can you make this 68dp instead?

@@ +104,5 @@
>      <dimen name="doorhanger_spinner_textsize">9sp</dimen>
>      <dimen name="doorhanger_padding">15dp</dimen>
>      <dimen name="doorhanger_offsetX">10dp</dimen>
> +    <dimen name="doorhanger_offsetY">66dp</dimen>
> +    <dimen name="doorhanger_GB_offsetY">7dp</dimen>

This is not the offset for centering, this is the offset for showing it from an anchor, and that's why it's so different from the other values.

If you don't get showAtLocation + centering working (from the comment in AnchoredPopup), keep this value here, and we'll use it as a hack.

If you do get showAtLocation + centering working, the GB value should be here in values/dimens.xml as doorhanger_offsetY, and the "most phones" value should be in values-v11/dimens.xml. But be sure to document that this value is for GB, and to see values-v11/dimens.xml for the value that applies for most phones.

::: mobile/android/base/widget/AnchoredPopup.java
@@ +102,4 @@
>              return;
>          }
>  
> +        if (HardwareUtils.isTablet()) {

This should definitely be a different patch, don't include it here.
The tablet case isn't an "edge" case because we intentionally want it to have different behavior, so when you do add it, use an if/else, and make sure to document the differences in a comment (back button vs centering).

@@ +110,3 @@
>          }
> +
> +        if (Versions.preHC) {

Explain what you're doing here and why (GB hack), in a concise, sentence-format comment.

@@ +110,4 @@
>          }
> +
> +        if (Versions.preHC) {
> +            setWidth(decorView.getWidth());

Why do you need to set the width? From the comment above, isn't that only necessary if you use showAtLocation?

@@ +114,5 @@
> +            int offsetGBY = mContext.getResources().getDimensionPixelOffset(R.dimen.doorhanger_GB_offsetY);
> +            if (isShowing()) {
> +                update(mAnchor, 0, -offsetGBY, -1, -1);
> +            } else {
> +                showAsDropDown(mAnchor, 0, -offsetGBY);

Does this look centered? I don't think this code actually centers on GB devices - there is no gravity. Shouldn't this match the code from the (mAnchor == null) case? And if that case doesn't work here, then the code up there won't work either for GB devices.

...but if you can't get it working with showAtLocation, I'm okay with using the dropdown method (but you should make sure it's consistent in the mAnchor == null case too).

Don't spend too much time on this, it's not the important part.

Comment 14

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Tablet centering

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Phone centered

This one will be moved slightly lower to match conversation I had with antlam.

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8592396 [details]
Screenshot: Phone centered

this looks good!

Comment 17

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8592395 [details]
Screenshot: Tablet centering

This looks like it could move up 1 dp to be just a little bit closer to the URL bar.

Otherwise I'm ok with centering this for now.

Comment 18

[Allison Naaktgeboren :ally]

Attachment: centeronphonesReviewCopy

If you would like the popup not to overlap the urlbar, I will need to to switch back to the showasdropdown() code path

Comment 19

[:Margaret Leibovic]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #18)
> Created attachment 8592614 [details] [diff] [review]
> centeronphonesReviewCopy
> 
> If you would like the popup not to overlap the urlbar, I will need to to
> switch back to the showasdropdown() code path

For spoofing protection, this popup should always overlap with the urlbar (you two can fix bug 1151505 next :).

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8592614 [details] [diff] [review]
centeronphonesReviewCopy

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

Let's go back to the showAsDropdown code, using the GB_offsetY value. When building this, I see the doorhanger completely overlapping the urlbar, and that's not something we can ship.

::: mobile/android/base/widget/AnchoredPopup.java
@@ +86,5 @@
>  
> +        // The doorhanger should overlap the bottom of the urlbar.
> +        int offsetY = mContext.getResources().getDimensionPixelOffset(R.dimen.doorhanger_offsetY);
> +        final View decorView = ((Activity) mContext).getWindow().getDecorView();
> +

Yeah, I guess that experiment didn't work.

I'm pretty tired of GB (and I imagine you are too) so let's just move all the GB code up here, and label it as a hack - that will keep the rest of this code a little more clean. Add comment explaining why need this GB hack, because window layout parameters are ignored so the view has 0 height and width, and also that showAtLocation doesn't respect offsets (right? and also adds a big black non-tap-dismissable shadow to everything, but these are details and we just need the big picture, which is that there are some bugs in GB).

So up here, there should be an
if (Versions.preHC) {
// Check for null or out of window bounds anchor, and use decorView if so (or mAnchor if that's fine)
// Call the showAsDropdown code that you had in the previous iteration of the patch
}

@@ +91,2 @@
>          // If the anchor is null or out of the window bounds, just show the popup at the top of the
>          // root view, keeping the correct X coordinate.

Remove the "x coordinate" part of this comment because it's no longer correct with these changes.

Comment 21

[Allison Naaktgeboren :ally]

Attachment: centeronphonesReviewCopy

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 8593084 [details] [diff] [review]
centeronphonesReviewCopy

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

::: mobile/android/base/widget/AnchoredPopup.java
@@ +95,5 @@
> +            offsetY = mContext.getResources().getDimensionPixelOffset(R.dimen.doorhanger_GB_offsetY);
> +            if (mAnchor != null) {
> +              showAsDropDown(mAnchor, 0, -offsetY);
> +            } else {
> +              showAtLocation(decorView, Gravity.NO_GRAVITY, 0, offsetY);

Can we not use showAtLocation at all? Is it possible to only use showAsDropDown only? (as in, if there is no anchor, use decorView as the anchor with the default offsetY) Does that make sense?

final View doorhangerAnchor = mAnchor;
if (doorhangerAnchor == null) {
  doorhangerAnchor = decorView;
  offsetY = mContext...(R.dimen.doorhanger_offsetY);
}

showAsDropDown(...)

Is there a reason why this wouldn't work?

@@ +99,5 @@
> +              showAtLocation(decorView, Gravity.NO_GRAVITY, 0, offsetY);
> +            }
> +            return;
> +        }
> +        

ws

Comment 23

[Allison Naaktgeboren :ally]

Attachment: centeronphonesReviewCopy

Comment 24

[Chenxia Liu [:liuche]]

Comment on attachment 8593191 [details] [diff] [review]
centeronphonesReviewCopy

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

YESSSSSS! This looks great, Ally :) It's much cleaner than it was before, because we're isolating the gingerbread bug on its own so it's not cluttering up (and confusing) the rest of the code. Fix the comments (so no one else has to go through this kind of hell again) and then we're good to land!

::: mobile/android/base/widget/AnchoredPopup.java
@@ +86,5 @@
> +        // The doorhanger should overlap the bottom of the urlbar.
> +        int offsetY = mContext.getResources().getDimensionPixelOffset(R.dimen.doorhanger_offsetY);
> +        final View decorView = ((Activity) mContext).getWindow().getDecorView();
> +
> +        // Bug in gingerbread os in showAtLocation ignores window layout parameters.

Nit: fix the capitalization of Gingerbread (you can take OS out, it's implied).

This comment should definitely mention that we can't use showAtLocation because of the window layout bug, *and therefore* we're using showAsDropDown instead. Basically, it would have been helpful to know this was a problem when we first looked at this code (I remember confusion when we were trying to figure out why *both* showAtLocation and showAsDropDown were used), so we should be explicit and provide the necessary context in this comment.

You can optionally label this very clearly as a hack, like "HACK for Gingerbread: showAtLocation..." because honestly, this isn't nice, and this isn't something people should be changing, but up to you.

@@ +87,5 @@
> +        int offsetY = mContext.getResources().getDimensionPixelOffset(R.dimen.doorhanger_offsetY);
> +        final View decorView = ((Activity) mContext).getWindow().getDecorView();
> +
> +        // Bug in gingerbread os in showAtLocation ignores window layout parameters.
> +        // They are incorrectly ignored, dimensions default to 0x0 dp.

Nit: "Height and width are set to 0dp." That way it isn't redundant with the previous sentence. I like reading clear, concise comments, so we should make sure they get written as such :)

@@ +97,5 @@
> +            }
> +            showAsDropDown(mAnchor, 0, -offsetY);
> +            return;
> +        }
> +        

trailing whitespace (ws)

Comment 25

[Chenxia Liu [:liuche]]

Let's land this first part, so it can bake on Nightly and then get it uplifted. I'll mark this [leave-open] so you can finish the tablet back button anchoring part.

Comment 26

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #25)
> Let's land this first part, so it can bake on Nightly and then get it
> uplifted. I'll mark this [leave-open] so you can finish the tablet back
> button anchoring part.

If we're okay with shipping the doorhanger centered on tablets for Fx39, I would move this to a follow-up bug.

Comment 27

[Chenxia Liu [:liuche]]

Anthony was okay with it - Ally, can you file the follow-up and block it on this bug?

Comment 28

[Allison Naaktgeboren :ally]

   https://hg.mozilla.org/integration/fx-team/rev/bb032c07da15

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb032c07da15

Comment 30

[Chenxia Liu [:liuche]]

Comment on attachment 8593191 [details] [diff] [review]
centeronphonesReviewCopy

Approval Request Comment
[Feature/regressing bug #]: Doorhanger redesign
[User impact if declined]: UX of doorhangers will be degraded
[Describe test coverage new/current, TreeHerder]: local, Nightly
[Risks and why]: low, simplifies code path
[String/UUID change made/needed]: none

Comment 31

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8593191 [details] [diff] [review]
centeronphonesReviewCopy

My understanding is that this work is shipping in 39 and 38 is unaffected. We're early enough in the Aurora cycle to take this change. Aurora+

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fe48f12288a9