402548 - Autocomplete popup is not offset at the right position and width when page is zoomed

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Martijn Wargers (dead)]

(not really a regression from bug 389628, but that bug makes it possible to encounter for users)

To reproduce:
- Go to http://www.google.nl/firefox and zoom in (or out)
- Double-click in the input to open the autocomplete popup.

Expected result:
- Autocomplete popup should be aligned and have the same width as the text input of the page

Actual result:
- Autocomplete popup is a little off. It seems like the code doesn't make a zoom correction for the zoomed height and width of the page input.

Comment 1

[Neil Deakin]

This is because the width of the element is computed using getBoundingClientRect which returns the unscaled width, but we need to use the scaled width.

Comment 2

[Neil Deakin]

Martijn: the position of the popup looks ok to me, but only the width isn't right. If that's what you're seeing, then this is only a very minor bug.

Comment 3

[Martijn Wargers (dead)]

Attachment: screenshot of what I see

This is what I get when I have the page zoomed in a lot.
The popup is overlaying half of the input in that case.

Comment 4

[Martijn Wargers (dead)]

(but personally, fwiw, I don't think it should block 1.9)

Comment 5

[Dão Gottwald [:dao]]

Well, it certainly does look broken and like a properly blocking bug.

Comment 6

[Neil Deakin]

Attachment: account for scale when calculating popup position

This fixes the position of the autocomplete popup and also fixes bug 402551. It does not fix the width of the autocomplete popup.

I will add some tests and then ask for review.

Comment 7

[Martijn Wargers (dead)]

Attachment: fixes autocomplete width

This fixes the width of the autocomplete popup for me.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 288921 [details] [diff] [review]
fixes autocomplete width

Can't use getBrowser() in toolkit. This is also wrong for the Location bar and the Search bar if the content is zoomed, for instance.

Comment 9

[Martijn Wargers (dead)]

Attachment: fixes autocomplete width, v2

Ok, thanks for the info.
This is another approach.

Comment 10

[Neil Deakin]

Attachment: account for scaling when opening a popup, with test

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think a better way to write this would be to convert from appunits to device units via aPresContext->AppUnitsToDevUnits() and then back to CSS pixels or appunits as necessary. It's more clear.

Comment 12

[Neil Deakin]

(In reply to comment #11)
> I think a better way to write this would be to convert from appunits to device
> units via aPresContext->AppUnitsToDevUnits() and then back to CSS pixels or
> appunits as necessary. It's more clear.
> 

Can you explain what you mean here? What purpose would converting to devpixels and then back again serve?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Instead of calling GetPixelScale, which is a confusing API and actually unused currently, it would be more obvious what's going on if you convert parentRect to device pixels and then back to appunits in the appropriate coordinate system.

Comment 14

[Neil Deakin]

Attachment: something like this?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I still find this unclear.

+  // the anchor may be in a different document with a different scale,
+  // so adjust the rectangle to the scale of the popup.
+  parentRect.ScaleRoundOut(float(presContext->AppUnitsPerDevPixel()) /
+                           aAnchorFrame->PresContext()->AppUnitsPerDevPixel());

We don't actually use the top/left which is good because it's kinda meaningless here. So lets make this parentSize and add a comment to make it clear that it's the size in this frame's appunits.

I don't understand this screenpos code. Maybe it would be clearer if you start by converting mScreenPosX to unscaled dev pixels and then proceed from there.

Comment 16

[Neil Deakin]

Attachment: add more comments, use size instead of rect

Comment 17

[Martijn Wargers (dead)]

Comment on attachment 288986 [details] [diff] [review]
fixes autocomplete width, v2

Neil, do you think this is the correct way to solve this?

Comment 18

[Neil Deakin]

Comment on attachment 288986 [details] [diff] [review]
fixes autocomplete width, v2

That's probably the best we can do for now. I wouldn't consider the popup width to be as much of a concern as the position patch in the bug though.

Comment 19

[Martijn Wargers (dead)]

Comment on attachment 288986 [details] [diff] [review]
fixes autocomplete width, v2

Because this is toolkit, I assume this doesn't need sr.

Comment 20

[Martijn Wargers (dead)]

I filed bug 407912 for the width patch, since approval1.9 drivers don't 'see' bugs that are already fixed.