Closed Bug 768073 Opened 9 years ago Closed 9 years ago

[tablet] Doorhanger arrow is misaligned

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox15 verified, firefox16 verified, firefox17 verified)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified
firefox17 --- verified

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
This issue still exists, but it's different now that bug 716095 landed.
Blocks: 716095
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)
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 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 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-
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.
(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 ;)
> > 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! ;)
Attached patch smarter patch (obsolete) — Splinter Review
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 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-
Attached patch patch v3Splinter Review
(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 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+
(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
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?
https://hg.mozilla.org/mozilla-central/rev/4cc05ceaa476
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #640272 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.