Closed Bug 976772 Opened 7 years ago Closed 7 years ago

[e10s] Widget can appear at the wrong place with iframes

Categories

(Firefox :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch coordinatesSplinter Review
For bug 938359, I was messing around with positioning and I discovered that our existing code for the context menu and autocomplete doesn't handle frames correctly.

This first patch addresses the issue with the context menu. The main problem is that event.client{X,Y} returns coordinates relative to the innermost frame. I decided to use event.screen{X,Y} instead and to pass those values to openPopupAtScreen. Doing it this way seems more compatible with our API than trying to anchor everything to the <browser> element. It also deals correctly with chrome zooming.

I also discovered that event.screen{X,Y} gives different values depending on whether we're in-process or out-of-process, so I added a new method to browser.xml to normalize out the difference. The comment explains the issue.
Attachment #8381703 - Flags: review?(felipc)
Attached patch autocomplete (obsolete) — Splinter Review
There's a related problem with autocomplete. It uses getBoundingClientRect(), which also returns coordinates relative to the innermost frame. These coordinates also aren't adjusted to account for zooming.

This patch just does a little arithmetic to adjust everything. I've tested it with both chrome and content zooming as well as frames and it seems to work.
Attachment #8381706 - Flags: review?(felipc)
Attachment #8381703 - Flags: review?(felipc) → review+
Comment on attachment 8381706 [details] [diff] [review]
autocomplete

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

I had done something similar for the <select> popups code, in a slightly different manner (based on some Metro code IIRC). 
https://hg.mozilla.org/mozilla-central/rev/56c09de8d0f6#l8.49
But it still doesn't account for zoom level, and your implementation is better (so with window.mozInnerScreenX we don't need to iterate through nested frames?)
It'd be nice to unify the two and have your impl somewhere that the <select> code can access, and presumably other features too which will need it.
Attachment #8381706 - Flags: review?(felipc) → review+
Attached patch autocomplete v2Splinter Review
This version is more comprehensive. It moves the getBoundingClientRect stuff to a JSM. I also switched a few things to use openPopupAtScreen, since I now remember from the tooltip bug that that function works with RTL text and openPopup doesn't.
Attachment #8381706 - Attachment is obsolete: true
Attachment #8383343 - Flags: review?(felipc)
Attachment #8383343 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/e4139eb421ff
https://hg.mozilla.org/mozilla-central/rev/9a822b2b8810
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Keywords: verifyme
Hi, do you think you can provide us with a test page in order to manually verify this bug?
Flags: needinfo?(wmccloskey)
That doesn't seem worth the effort.
Flags: needinfo?(wmccloskey)
Depends on: 1262332
You need to log in before you can comment on or make changes to this bug.