11.05 KB, image/gif
1.47 KB, patch
Neil Deakin: review+
|Details | Diff | Splinter Review|
16.36 KB, patch
|Details | Diff | Splinter Review|
(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.
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.
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.
(but personally, fwiw, I don't think it should block 1.9)
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.
Created attachment 288921 [details] [diff] [review] fixes autocomplete width This fixes the width of the autocomplete popup for me.
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.
Created attachment 288986 [details] [diff] [review] fixes autocomplete width, v2 Ok, thanks for the info. This is another approach.
Created attachment 289009 [details] [diff] [review] account for scaling when opening a popup, with test
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?
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
Comment on attachment 288986 [details] [diff] [review] fixes autocomplete width, v2 Neil, do you think this is the correct way to solve this?
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 on attachment 288986 [details] [diff] [review] fixes autocomplete width, v2 Because this is toolkit, I assume this doesn't need sr.
I filed bug 407912 for the width patch, since approval1.9 drivers don't 'see' bugs that are already fixed.