684278 - Consolidate arrowbox methods

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

I added a parameter to our "anchorPopup" method for arrowboxes in bug 684277 that deprecates some existing methods we already had (pointLeftAt and pointRightAt). We should remove those methods or have them use the new code, and update any callers.

Comment 1

[Lucas Rocha (:lucasr)]

Fixing.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: (1/4) Add some extra centered positions for arrowbox

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: (2/4) Correctly calculate popup rect on arrowbox anchorTo()

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: (3/4) Reuse same position when re-anchoring popup on resize event

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: (4/4) Use anchorTo() to force arrow direction instead of separate methods

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 558290 [details] [diff] [review]
(2/4) Correctly calculate popup rect on arrowbox anchorTo()

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

This breaks positioning of the bookmarks and menu(?) popup I think. We seem to have some issues with the width of the arrowbox vs the width of its contents (rather than this hack I threw together). We need to figure out some better way to reliably find the width/height of the contents before the box is shown.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 558291 [details] [diff] [review]
(3/4) Reuse same position when re-anchoring popup on resize event

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

Whoops. Coming back to this patch, let's make sure that we've defined a position field in here (to blank) so that we don't run into weird position=undefined situations.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 558292 [details] [diff] [review]
(4/4) Use anchorTo() to force arrow direction instead of separate methods

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

::: mobile/chrome/content/common-ui.js
@@ +457,1 @@
>      }).bind(this), 0);

Double quotes. Also, this reminds me what I was originally writing this to get around, rtl problems. We should be handling those in the anchorTo stuff. If we do that, do we still need this if at all?

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: (1/4) Add some extra centered positions for arrowbox

Support RTL cases.

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: (2/4) Correctly calculate popup rect on arrowbox anchorTo()

This doesn't break the download popup. But yes, we should find a non-hacky way to do this...

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: (3/4) Reuse same position when re-anchoring popup on resize event

Just rebased, no code changes. Given that the position arg in anchorTo method is already optional, we don't need to care about this.position being undefined here, do we?

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: (4/4) Use anchorTo() to force arrow direction instead of separate methods

Simpler patch now that anchorTo's centered positions handle RTL.

Comment 13

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> Created attachment 564786 [details] [diff] [review] [diff] [details] [review]
> (2/4) Correctly calculate popup rect on arrowbox anchorTo()
> 
> This doesn't break the download popup. But yes, we should find a non-hacky
> way to do this...

Talked on IRC some. This causes problems on tablets. Not having it causes problems on phones with the "New tab" popup. You're right. We need a good solution here....

Comment 18

[Lucas Rocha (:lucasr)]

Attachment: (3/4) Reuse same position when re-anchoring popup on resize event

Fixed reference to 'this' in the resize handler.

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 564788 [details] [diff] [review]
(4/4) Use anchorTo() to force arrow direction instead of separate methods

Landed 3/4 as a fix for bug 691338.

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 564788 [details] [diff] [review]
(4/4) Use anchorTo() to force arrow direction instead of separate methods

Oops, not this one.

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 565939 [details] [diff] [review]
(3/4) Reuse same position when re-anchoring popup on resize event

This one.

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 564785 [details] [diff] [review]
(1/4) Add some extra centered positions for arrowbox

Clearing my queue. This is Fennec-xul. Probably not going to move forward

Comment 24

[Wesley Johnston (:wesj)]

Comment on attachment 564786 [details] [diff] [review]
(2/4) Correctly calculate popup rect on arrowbox anchorTo()

Clearing my queue. This is Fennec-xul. Probably not going to move forward

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 564788 [details] [diff] [review]
(4/4) Use anchorTo() to force arrow direction instead of separate methods

Clearing my queue. This is Fennec-xul. Probably not going to move forward

Comment 26

[BMO Automation]

Closing all opened bug in a graveyard component