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

VERIFIED FIXED in Firefox 54

Status

()

P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

unspecified
Firefox 54
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
+++ 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

2 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

2 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

2 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.

Comment 6

2 years ago
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]

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99aabb33581a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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
status-firefox54: fixed → verified

Updated

2 years ago
See Also: → bug 1361195
You need to log in before you can comment on or make changes to this bug.