Closed
Bug 716095
Opened 13 years ago
Closed 12 years ago
[tablet] Add a max width to door hangers
Categories
(Firefox for Android Graveyard :: General, defect, P3)
Tracking
(firefox15 verified, firefox16 verified, firefox17 verified, fennec15+)
VERIFIED
FIXED
Firefox 16
People
(Reporter: mfinkle, Assigned: Margaret)
References
Details
(Whiteboard: [tablet])
Attachments
(2 files, 2 obsolete files)
83.75 KB,
image/jpeg
|
Details | |
5.59 KB,
patch
|
sriram
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•13 years ago
|
Whiteboard: [tablet]
Updated•13 years ago
|
tracking-fennec: --- → 12+
Updated•13 years ago
|
Priority: -- → P3
Updated•13 years ago
|
Assignee: nobody → dpetters
Comment 1•13 years ago
|
||
Updated•13 years ago
|
Attachment #587815 -
Attachment description: Not being sure how to apply max_width, I first tried setting the width layoutparm to wrap_content. This seems to have given the des have given the desired behaviour on an Asus Tablet and Galaxy Nexus. → 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.
Updated•13 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•13 years ago
|
||
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?
Attachment #587815 -
Flags: feedback?(sriram)
Comment 3•13 years ago
|
||
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 :)
Attachment #587815 -
Flags: feedback?(sriram) → feedback-
Updated•13 years ago
|
Assignee: dpetters → nobody
Updated•12 years ago
|
tracking-fennec: 12+ → 15+
Reporter | ||
Comment 4•12 years ago
|
||
Margaret - Can you take this one?
Assignee: nobody → margaret.leibovic
Assignee | ||
Comment 6•12 years ago
|
||
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.
Attachment #587815 -
Attachment is obsolete: true
Attachment #636442 -
Flags: feedback?(sriram)
Assignee | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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) :)
Attachment #636442 -
Flags: feedback?(sriram) → feedback+
Assignee | ||
Comment 9•12 years ago
|
||
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.
Attachment #636442 -
Attachment is obsolete: true
Attachment #636869 -
Flags: review?(sriram)
Comment 10•12 years ago
|
||
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.
Attachment #636869 -
Flags: review?(sriram) → review+
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3101aa0360
Target Milestone: --- → Firefox 16
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cd3101aa0360
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•12 years ago
|
||
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
Attachment #636869 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Attachment #636869 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 14•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/73be8c95dd66
status-firefox15:
--- → fixed
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
status-firefox16:
--- → verified
status-firefox17:
--- → verified
OS: Linux → Android
Hardware: x86 → ARM
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•