716095 - [tablet] Add a max width to door hangers

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

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

When displaying on tablets, doorhanger notifications span the entire width of the screen. This looks a bit silly. We could easily add a max width.

Comment 1

[Dmitrij Petters]

Attachment: Not being sure how to apply max_width, I first tried setting the width LayoutParam to WRAP_CONTENT. This seems to have given the desired behavior on an Asus Tablet and Galaxy Nexus.

Comment 2

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

Comment on attachment 587815 [details] [diff] [review]
Not being sure how to apply max_width, I first tried setting the width LayoutParam to WRAP_CONTENT. This seems to have given the desired behavior on an Asus Tablet and Galaxy Nexus.

Sriram - Can you look this patch over and give some feedback?

Comment 3

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 587815 [details] [diff] [review]
Not being sure how to apply max_width, I first tried setting the width LayoutParam to WRAP_CONTENT. This seems to have given the desired behavior on an Asus Tablet and Galaxy Nexus.

Could you please post a screenshot with this patch?
Changing the FILL_PARENT to WRAP_CONTENT can cause issues if the text we should is very small (like 2 words). 
I would probably do a WRAP_CONTENT and set a minWidth for DoorHanger to make sure the width is atleast 200 dip or something. [http://developer.android.com/reference/android/view/View.html#setMinimumWidth%28int%29]
If that doesn't work, we might need to override onSizeChanged().

Marking it feedback-. If the screenshots are fine for very small text doorhangers, I am happy with this patch :)

Comment 4

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

Margaret - Can you take this one?

Comment 6

[:Margaret Leibovic]

Attachment: patch with min/max width

This patch correctly limits the width of the doorhanger to be within a certain range, but we may want to adjust what that range is. Using 200dip makes it so that the doorhanger may not take up the entire width on phones, and I'm not sure that we want that.

When the doorhanger doesn't take up the whole width, the arrow is misaligned. This is already a problem on tablets (see bug 768073), but I'm thinking we could pass an offset to showAsDropDown to fix it.

Also, the sides of the doorhanger look really sharp when they're not against the edge of the screen, so we may want new assets for that.

Comment 7

[:Margaret Leibovic]

Attachment: screenshot

Here's a screenshot. You can see that our current use of showAsDropDown is just aligning the doorhanger with the left side of the favicon.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 636442 [details] [diff] [review]
patch with min/max width

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

This looks good for me.
But since phones should have full screen width, please add the layout file to layout-xlarge/ and layout-sw600dp/
and leave the phone related ones like how it was early. It should work fine (with a "if" clause for windowlayout mode) :)

Comment 9

[:Margaret Leibovic]

Attachment: patch with different layouts for tablet

I checked that this maintains the old appearance on phones, but uses the correct restricted width for tablets.

I can file a follow-up to update the resources to make the edges look better.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 636869 [details] [diff] [review]
patch with different layouts for tablet

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

This looks good to me.
I kind of don't like the "widthSpec" variable. If you could bring the tertiary operator into the setWindowLayoutMode(), that would be nice.

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3101aa0360

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

[:Margaret Leibovic]

Comment on attachment 636869 [details] [diff] [review]
patch with different layouts for tablet

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this is an issue with the new tablet UI, which is tracking 15
User impact if declined: doorhangers stretch all the way across the screen
Testing completed (on m-c, etc.): just landed on m-c
Risk to taking this patch (and alternatives if risky): low-risk, adds some new tablet-specific layouts
String or UUID changes made by this patch: n/a

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/73be8c95dd66