Closed
Bug 1343507
Opened 8 years ago
Closed 8 years ago
Search settings button moves to its own line with 150% zoom follow-up
Categories
(Firefox :: Address Bar, defect, P3)
Tracking
()
VERIFIED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | verified |
People
(Reporter: adw, Assigned: adw)
References
Details
(Whiteboard: [fxsearch])
Attachments
(1 file)
+++ This bug was initially created as a clone of Bug #1316566 +++
(In reply to Drew Willcoxon :adw from comment #23)
> Oh great, it seems to depend on the window width. :-P I'll clone a new bug.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
I give up, this just decrements the remainder if screenPixelsPerCSSPixel isn't an integer. It works on the various window widths and devPixelsPerPx values I tried, and it looks fine.
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8842410 [details]
Bug 1343507 - Search settings button moves to its own line with 150% zoom follow-up.
https://reviewboard.mozilla.org/r/116274/#review117830
Quick r+ by code inspection because I don't want to block this and the code change seems harmless, but it indeed doesn't seem to be a very satisfying fix. Feel free to re-request review if you would like me to spend some time digging into it too.
::: browser/components/search/content/search.xml:1572
(Diff revision 1)
> //
> // There's one weird thing to guard against. When layout pixels
> // aren't an integral multiple of device pixels, the calculated
> // remainder can end up being ~1px too big, at least on Windows,
> // which pushes the settings button to a new row. The remainder
> // is integral, not a fraction, so that's not the problem. To
I'm slightly confused by "the remainder can end up being ~1px too big" and "The remainder is integral" in the same comment. When you say ~1px, I assumed it meant something like 0.8. Can it sometimes be 2?
Attachment #8842410 -
Flags: review?(florian) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> but it indeed doesn't seem to be a very satisfying fix. Feel free to
> re-request review if you would like me to spend some time digging into
> it too.
I would be interested to know what exactly is going on here, but to be honest I don't think it's worth our time, unless maybe this new patch still doesn't fix the problem in some case.
> I'm slightly confused by "the remainder can end up being ~1px too big" and
> "The remainder is integral" in the same comment. When you say ~1px, I
> assumed it meant something like 0.8. Can it sometimes be 2?
Yeah, that wasn't clear, you're right. I reworded it:
// There's one weird thing to guard against: when layout pixels
// aren't an integral multiple of device pixels, the settings
// button sometimes gets pushed to a new row, depending on the
// panel and button widths. It's as if `remainder` is somehow
// too big, even though it's an integer. To work around that,
// decrement the remainder if the scale is not an integer.
Hopefully that plus these two bugs (via blame) will be enough to help people looking at it in the future.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99aabb33581a
Search settings button moves to its own line with 150% zoom follow-up. r=florian
Updated•8 years ago
|
Whiteboard: [fxsearch]
Comment 7•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Verified fixed using the latest Nightly 54.0a1 (2017-03-05) on Ubuntu 16.04, Mac OS X 10.10, Windows 7 x86 and Windows 10 x64 with "layout.css.devPixelsPerPx" set to 1.5
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•