Closed Bug 684278 Opened 9 years ago Closed 1 year ago

Consolidate arrowbox methods

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: wesj, Unassigned)

References

Details

Attachments

(3 files, 6 obsolete files)

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.
Depends on: 677673
Fixing.
Assignee: nobody → lucasr.at.mozilla
Attachment #558289 - Flags: review?(wjohnston) → review+
Attachment #558291 - Flags: review?(wjohnston) → review+
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.
Attachment #558290 - Flags: review?(wjohnston) → review-
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 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?
Support RTL cases.
Attachment #558289 - Attachment is obsolete: true
Attachment #564785 - Flags: review?(wjohnston)
This doesn't break the download popup. But yes, we should find a non-hacky way to do this...
Attachment #558290 - Attachment is obsolete: true
Attachment #564786 - Flags: review?(wjohnston)
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?
Attachment #558291 - Attachment is obsolete: true
Attachment #564787 - Flags: review?(wjohnston)
Simpler patch now that anchorTo's centered positions handle RTL.
Attachment #558292 - Attachment is obsolete: true
Attachment #558292 - Flags: review?(wjohnston)
Attachment #564788 - Flags: review?(wjohnston)
(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....
Blocks: 692483
Duplicate of this bug: 692494
Duplicate of this bug: 692489
Duplicate of this bug: 692488
Duplicate of this bug: 692494
Fixed reference to 'this' in the resize handler.
Attachment #564787 - Attachment is obsolete: true
Attachment #564787 - Flags: review?(wjohnston)
Attachment #565939 - Flags: review?(wjohnston)
Duplicate of this bug: 692483
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.
Attachment #564788 - Attachment is obsolete: true
Attachment #564788 - Flags: review?(wjohnston)
Comment on attachment 564788 [details] [diff] [review]
(4/4) Use anchorTo() to force arrow direction instead of separate methods

Oops, not this one.
Attachment #564788 - Attachment is obsolete: false
Attachment #564788 - Flags: review?(wjohnston)
Comment on attachment 565939 [details] [diff] [review]
(3/4) Reuse same position when re-anchoring popup on resize event

This one.
Attachment #565939 - Attachment is obsolete: true
Attachment #565939 - Flags: review?(wjohnston)
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
Attachment #564785 - Flags: review?(wjohnston)
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
Attachment #564786 - Flags: review?(wjohnston)
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
Attachment #564788 - Flags: review?(wjohnston)
Assignee: lucasr.at.mozilla → nobody
Closing all opened bug in a graveyard component
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.