Closed Bug 1343507 Opened 7 years ago Closed 7 years ago

Search settings button moves to its own line with 150% zoom follow-up

Categories

(Firefox :: Address Bar, defect, P3)

Unspecified
Windows
defect

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.
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 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+
(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
Whiteboard: [fxsearch]
https://hg.mozilla.org/mozilla-central/rev/99aabb33581a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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
See Also: → 1361195
You need to log in before you can comment on or make changes to this bug.