Closed
Bug 768073
Opened 13 years ago
Closed 13 years ago
[tablet] Doorhanger arrow is misaligned
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox15 verified, firefox16 verified, firefox17 verified)
VERIFIED
FIXED
Firefox 16
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
Attachments
(1 file, 3 obsolete files)
6.75 KB,
patch
|
sriram
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This is a similar issue to bug 764405. I think it would be nice to just come up with a general solution to make the arrows on these various panels point to a certain element, but I'm filing this separately to make sure it gets tracked.
Assignee | ||
Comment 1•13 years ago
|
||
This issue still exists, but it's different now that bug 716095 landed.
Blocks: 716095
Assignee | ||
Comment 2•13 years ago
|
||
This patch just adds a hard-coded offset to make the arrow point to the right place. It looks good on my Galaxy Tab, but we probably want to test on other devices. I took the metrics.density trick from FormAssistPopup, so I hope that will work as we'd want.
Assignee: nobody → margaret.leibovic
Attachment #638018 -
Flags: review?(sriram)
Assignee | ||
Comment 3•13 years ago
|
||
I found this while I was working on my previous patch. There doesn't seem to be any reason for setting explicit width/height values on these arrows, and it confused me while working on figuring out what that offset should be.
I tested this on a Galaxy Tab and a Nexus S, and it doesn't change the appearance of the doorhangers.
Attachment #638020 -
Flags: review?(sriram)
Comment 4•13 years ago
|
||
Comment on attachment 638018 [details] [diff] [review]
add offset to showAsDropDown for tablets
Review of attachment 638018 [details] [diff] [review]:
-----------------------------------------------------------------
This is a hacky patch. This will break when there is a conditional forward button. I wouldn't suggest adding an offset and aligning the arrow that way.
Ideally the doorhanger should listen for layout changes of its anchor, and probably just do an update().
The update() should be overridden to take in account the width of the anchor, the width of the arrow, and align it at the center.
Also, if offset is a better way after these, the dpi value is better mentioned in values/dimens.xml
(Or, even if we want the width of arrow -- which is not available until it is shown -- we can use this)
Please define a dimension, say, "<dimen name="doorhanger_arrow_width">16dp</dimen>
and in code, use it as, "getApplicationContext().getResources().getDimension(R.dimen.doorhanger_arrow_width)"
This is cleaner than devicemanager, metrics.
Attachment #638018 -
Flags: review?(sriram) → review-
Comment 5•13 years ago
|
||
Comment on attachment 638020 [details] [diff] [review]
(cleanup) don't use hardcoded width/height for doorhanger arrows
Sorry for r-.
There is a reason for explicitly mentioning the size.
Tomorrow, when the size changes, or as per my comments in previous patch, if they are mentioned in dimens.xml, the need to be specified.
It's better to know the size, than banking on the resource. If anything goes odd with the display, we can catch what's going wrong where easily.
Attachment #638020 -
Flags: review?(sriram) → review-
Comment 6•13 years ago
|
||
I would like to have exact measurements specified for ImageViews and ImageButtons. "wrap_content" doesn't convey the essence. They are better for layouts (like Linear/Relative), but not buttons. In case of browser_toolbar.xml, the sizes of these buttons are so important, as there is a padding involved with most of them.
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to Sriram Ramasubramanian [:sriram] from comment #4)
> This is a hacky patch. This will break when there is a conditional forward
> button. I wouldn't suggest adding an offset and aligning the arrow that way.
> Ideally the doorhanger should listen for layout changes of its anchor, and
> probably just do an update().
> The update() should be overridden to take in account the width of the
> anchor, the width of the arrow, and align it at the center.
This shouldn't break when there is a conditional forward button, since the issue is that calling showAsDropDown with the favicon as the anchor aligns the left side of the doorhanger with the left side of the favicon.
I agree, though, that it would be better to be smart about computing the offset based on the width of the arrow and the width of the anchor. I can work on a new version of the patch to do that.
> Also, if offset is a better way after these, the dpi value is better
> mentioned in values/dimens.xml
> (Or, even if we want the width of arrow -- which is not available until it
> is shown -- we can use this)
> Please define a dimension, say, "<dimen
> name="doorhanger_arrow_width">16dp</dimen>
> and in code, use it as,
> "getApplicationContext().getResources().getDimension(R.dimen.
> doorhanger_arrow_width)"
> This is cleaner than devicemanager, metrics.
Cool, didn't know about this. We should file a follow-up to do the same kind of fix for FormAssistPopup, since I was just copying that ;)
Comment 8•13 years ago
|
||
> > Also, if offset is a better way after these, the dpi value is better
> > mentioned in values/dimens.xml
> > (Or, even if we want the width of arrow -- which is not available until it
> > is shown -- we can use this)
> > Please define a dimension, say, "<dimen
> > name="doorhanger_arrow_width">16dp</dimen>
> > and in code, use it as,
> > "getApplicationContext().getResources().getDimension(R.dimen.
> > doorhanger_arrow_width)"
> > This is cleaner than devicemanager, metrics.
>
> Cool, didn't know about this. We should file a follow-up to do the same kind
> of fix for FormAssistPopup, since I was just copying that ;)
There are many places where we use display metrics. We should revisit and change them all, when we get it for free from resources :)
"#define OLD_C_MACRO" is always awesome! ;)
Assignee | ||
Comment 9•13 years ago
|
||
This patch computes the offset based on the dimensions of the arrow view and the anchor view.
Out of curiosity, do you know why we have that update() call in there? The docs make it sound like it doesn't do anything we care about, so maybe we should just bail if the popup is already showing:
http://developer.android.com/reference/android/widget/PopupWindow.html#update%28%29
Attachment #638018 -
Attachment is obsolete: true
Attachment #638020 -
Attachment is obsolete: true
Attachment #639484 -
Flags: review?(sriram)
Comment 10•13 years ago
|
||
Comment on attachment 639484 [details] [diff] [review]
smarter patch
Review of attachment 639484 [details] [diff] [review]:
-----------------------------------------------------------------
Aaah.. getDimension() is missing.
Basically have the width in XML as "@dimen/doorhanger_arrow_width" and use the same in code.
This is because, when we show the DoorHanger for the first time, the arrow's width will not be available.
mArrow.getWidth() will return 0.
And regarding many XMLs -- they are needed for a constrained height for tablets (I'll kill one of those soon).
And regarding update -- it's basically to update the height and position. An empty update() call updates the popup based on change in anchor.
Ideally that should/can be overriden to calculate the arrow-width and align.
Attachment #639484 -
Flags: review?(sriram) → review-
Assignee | ||
Comment 11•13 years ago
|
||
(This is on top of my patch for bug 769001)
I found that my math was wrong in my last patch (not quite sure how I thought that was working), but this does the trick.
Attachment #639484 -
Attachment is obsolete: true
Attachment #640272 -
Flags: review?(sriram)
Comment 12•13 years ago
|
||
Comment on attachment 640272 [details] [diff] [review]
patch v3
Review of attachment 640272 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good to me. Please save the arrow width in a variable instead of mResources.
Attachment #640272 -
Flags: review?(sriram) → review+
Assignee | ||
Comment 13•13 years ago
|
||
Target Milestone: --- → Firefox 16
Assignee | ||
Comment 14•13 years ago
|
||
(This doesn't actually have to do with the browser toolbar layout)
Summary: [tablet] Doorhanger arrow is misaligned with new browser toolbar layout → [tablet] Doorhanger arrow is misaligned
Assignee | ||
Comment 15•13 years ago
|
||
Comment on attachment 640272 [details] [diff] [review]
patch v3
[Approval Request Comment]
Bug caused by (feature/regressing bug #): polish issue with the tablet UI
User impact if declined: doorhanger notifications are misaligned on tablets
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk style change
String or UUID changes made by this patch: n/a
Attachment #640272 -
Flags: approval-mozilla-aurora?
Comment 16•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Attachment #640272 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 17•13 years ago
|
||
status-firefox15:
--- → fixed
Updated•13 years ago
|
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
•