1027139 - Fix ArrowPopup arrow positioning

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

The plan is to extend ArrowPopup to make the hint popup for the reader mode icon (bug 1011712). From [1]: "This assumes that anchor is not too close to the right side of the screen."

Guess which side of the screen the reader mode icon is on!

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ArrowPopup.java#114

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Fix ArrowPopup positioning for anchors on right side of screen

Dynamically sets the arrow position after the popup window is positioned. Wish this didn't require a custom view for such a trivial use, but View.OnLayoutChangeListener wasn't introduced until API 11.

Comment 2

[:Margaret Leibovic]

Comment on attachment 8442551 [details] [diff] [review]
Fix ArrowPopup positioning for anchors on right side of screen

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

This positioning code is always so tricky, but this looks good. Be sure to test on tablets and phones.

::: mobile/android/base/widget/ArrowPopup.java
@@ +63,3 @@
>          setContentView(layout);
>  
> +        layout.mListener = new ArrowPopupLayout.OnSizeChangedListener() {

You can set this even though it's a private member? I guess that works because ArrowPopupLayout is an inner class?

@@ +138,5 @@
>              return;
>          }
>  
> +        if (isShowing()) {
> +            update(mAnchor, -mArrowWidth, -mYOffset, -1, -1);

Add a comment explaining that -mArrowWidth is just being used here to create some space to the left of the arrow (the fact that it's the arrow width confused me, since this doesn't actually have to do with the arrow).

Comment 3

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #2)
> You can set this even though it's a private member? I guess that works
> because ArrowPopupLayout is an inner class?

Yep.

> Add a comment explaining that -mArrowWidth is just being used here to create
> some space to the left of the arrow (the fact that it's the arrow width
> confused me, since this doesn't actually have to do with the arrow).

Done.

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

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/82142792b685