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

RESOLVED FIXED

Status

()

Core
XUL
P3
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Neil Deakin (mostly unavailable until September))

Tracking

Trunk
x86
Windows XP
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

10 years ago
(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.

Updated

10 years ago
Blocks: 4821

Updated

10 years ago
No longer blocks: 389628

Updated

10 years ago
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
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.
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.
(Reporter)

Comment 3

10 years ago
Created attachment 288684 [details]
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.
(Reporter)

Comment 4

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

Comment 5

10 years ago
Well, it certainly does look broken and like a properly blocking bug.
Created attachment 288881 [details] [diff] [review]
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.
Assignee: jag → enndeakin
Status: NEW → ASSIGNED
(Reporter)

Comment 7

10 years ago
Created attachment 288921 [details] [diff] [review]
fixes autocomplete width

This fixes the width of the autocomplete popup for me.

Comment 8

10 years ago
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.
(Reporter)

Comment 9

10 years ago
Created attachment 288986 [details] [diff] [review]
fixes autocomplete width, v2

Ok, thanks for the info.
This is another approach.
Attachment #288921 - Attachment is obsolete: true

Updated

10 years ago
Attachment #288986 - Attachment is patch: true
Attachment #288986 - Attachment mime type: application/octet-stream → text/plain
Created attachment 289009 [details] [diff] [review]
account for scaling when opening a popup, with test
Attachment #288881 - Attachment is obsolete: true
Attachment #289009 - Flags: superreview?(roc)
Attachment #289009 - Flags: review?(roc)
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.
(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?
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.
Created attachment 289511 [details] [diff] [review]
something like this?
Attachment #289009 - Attachment is obsolete: true
Attachment #289511 - Flags: superreview?(roc)
Attachment #289511 - Flags: review?(roc)
Attachment #289009 - Flags: superreview?(roc)
Attachment #289009 - Flags: review?(roc)
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.
Created attachment 289673 [details] [diff] [review]
add more comments, use size instead of rect
Attachment #289511 - Attachment is obsolete: true
Attachment #289673 - Flags: superreview?(roc)
Attachment #289673 - Flags: review?(roc)
Attachment #289511 - Flags: superreview?(roc)
Attachment #289511 - Flags: review?(roc)
Attachment #289673 - Flags: superreview?(roc)
Attachment #289673 - Flags: superreview+
Attachment #289673 - Flags: review?(roc)
Attachment #289673 - Flags: review+
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 17

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

Neil, do you think this is the correct way to solve this?
Attachment #288986 - Flags: review?(enndeakin)
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.
Attachment #288986 - Flags: review?(enndeakin) → review+
(Reporter)

Comment 19

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

Because this is toolkit, I assume this doesn't need sr.
Attachment #288986 - Flags: approval1.9?
(Reporter)

Comment 20

10 years ago
I filed bug 407912 for the width patch, since approval1.9 drivers don't 'see' bugs that are already fixed.
(Reporter)

Updated

10 years ago
Attachment #288986 - Flags: approval1.9?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.