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

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

(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?

The way Chenxia currently has it in Nightly is correct. But, we still need to horizontally center the entire doorhanger.

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.

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

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).

Centering on Tablets was awkward from my mock but I could be convinced :) Can I see a screenshot?

...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. ?!?! :/

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

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.

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

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.

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

Comment on attachment 8592396 [details] Screenshot: Phone centered this looks good!

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.

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

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

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.

(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.

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

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

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 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+