Closed
Bug 1316566
Opened 8 years ago
Closed 8 years ago
Search settings button moves to its own line with 150% zoom
Categories
(Firefox :: Address Bar, defect, P3)
Tracking
()
VERIFIED
FIXED
Firefox 54
People
(Reporter: quicksaver, Assigned: adw)
References
Details
Attachments
(3 files)
(deleted),
image/png
|
Details | |
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
patch
|
gchang
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
See screenshot.
STR: change "layout.css.devPixelsPerPx" to anything between 1.325 and 1.965 (Windows can set 150% zoom factor easily == 1.5)
Updated•8 years ago
|
Component: Search → Location Bar
Priority: -- → P3
Comment 1•8 years ago
|
||
Hi,
I can reproduce this on Windows 7 x64 with the latest Nightly 52.0a1(2016-11-13). I tried to reproduce this on Mac OS X 10.10 but I can't.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → adw
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
To recap, the width of the one-off buttons is integral, which means that when the popup width is not a multiple of the button width, there's some remainder that's left over at the right edge of the popup. When there's a single row of one-offs, we make the final dummy button wider than all the other buttons by this remainder so that the settings button hugs the right edge of the popup.
I can't explain what's going on in this case, but the width of that final dummy button is somehow ~1px too big, which pushes the settings button onto a new row. It must have something to do with layout pixels not being an integral multiple of the device pixels -- clearly. But I don't know what.
Since I don't exactly understand the problem, I'm not sure what the right fix is. Seems like a layout bug to me. This patch just unscales the remainder, floors it, scales it back, and then floors that. :-/
Assignee | ||
Comment 4•8 years ago
|
||
(which works BTW)
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8833578 [details]
Last Comment Bug 1316566 - Search settings button moves to its own line with 150% zoom.
https://reviewboard.mozilla.org/r/109802/#review111958
That seems reasonable; and thanks for including a comment that's detailed enough to ensure the next person looking at this will understand what we are doing :-).
I guess we'll need to do something similar for bug 1104325.
Attachment #8833578 -
Flags: review?(florian) → review+
Updated•8 years ago
|
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ef05c211734
Last Comment Bug 1316566 - Search settings button moves to its own line with 150% zoom. r=florian
Comment 7•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 8•8 years ago
|
||
Looks like this may have fixed bug 1325816 too?
Blocks: 1325816
Flags: needinfo?(adw)
(In reply to Marco Bonardo [::mak] from comment #8)
> Looks like this may have fixed bug 1325816 too?
Yep, I can confirm this is fixed on latest Nightly.
Comment 10•8 years ago
|
||
It would be nice to uplift the fix, it's more common nowadays on Win10 to use non default dpi settings.
Assignee | ||
Comment 11•8 years ago
|
||
urlbar one-offs are disabled on non-Nightly though, so I'm not sure it's worth uplifting. But I'll request it if you still think so.
Flags: needinfo?(adw)
Comment 12•8 years ago
|
||
I think it's worth to fix the searchbar, plus if we run another Shield on the new urlbar it would be better to not get a broken experience.
Assignee | ||
Comment 13•8 years ago
|
||
OK. This isn't a problem in the searchbar though because only the urlbar uses the "compact" settings button with the gear icon.
Assignee | ||
Comment 14•8 years ago
|
||
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1180944
[User impact if declined]: This bug
[Is this code covered by automated tests?]: This code, yes, this bug, no
[Has the fix been verified in Nightly?]: Not "officially" but commenters here have verified it
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Tiny patch specific to a tiny part of the code
[String changes made/needed]: None
Attachment #8836144 -
Flags: approval-mozilla-beta?
Attachment #8836144 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
status-firefox52:
--- → affected
status-firefox53:
--- → affected
Comment 15•8 years ago
|
||
Comment on attachment 8836144 [details] [diff] [review]
Aurora and Beta patch
Fix a UI issue related to search. Aurora53+.
Attachment #8836144 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 16•8 years ago
|
||
Comment 17•8 years ago
|
||
Comment on attachment 8836144 [details] [diff] [review]
Aurora and Beta patch
ui fix for search in compact mode, beta52+
Attachment #8836144 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 18•8 years ago
|
||
bugherder uplift |
Updated•8 years ago
|
Flags: qe-verify+
Comment 19•8 years ago
|
||
Verified as fixed using the latest Nightly 54.0a1(Build ID:20170216030210) on Windows 10 x64, with "layout.css.devPixelsPerPx" set to 1.5
The issue is still reproducible on Windows 7 x86 with latest Nightly 54.0a1(Build ID:20170216030210) and Ubuntu 16.04 with Nightly 54.0a1 (Build ID:20170217110156) with "layout.css.devPixelsPerPx" set to 1.5
Comment 20•8 years ago
|
||
bugherder uplift |
status-firefox-esr52:
--- → fixed
Comment 21•8 years ago
|
||
(In reply to roxana.leitan@softvision.ro from comment #19)
> The issue is still reproducible on Windows 7 x86 with latest Nightly
> 54.0a1(Build ID:20170216030210) and Ubuntu 16.04 with Nightly 54.0a1 (Build
> ID:20170217110156) with "layout.css.devPixelsPerPx" set to 1.5
Drew, any thoughts on this?
Flags: needinfo?(adw)
Assignee | ||
Comment 22•8 years ago
|
||
Strange, I can reproduce the problem too on Nightly but not on my own build. I'll try to find out what's going on.
Flags: needinfo?(adw)
Assignee | ||
Comment 23•8 years ago
|
||
Oh great, it seems to depend on the window width. :-P I'll clone a new bug.
Comment 25•8 years ago
|
||
Removing the qe-verify flag since there's nothing else to be done here and a follow up bug (Bug 1343507) has been filed separately for Roxana's concerns (see Comment 19).
Flags: qe-verify+
Comment 26•8 years ago
|
||
Based on comment 19 from this bug and comment 8 from bug 1343507 I will mark this as Verified Fixed.
Status: RESOLVED → VERIFIED
Comment 27•8 years ago
|
||
[Bugday-20170405]
OS : Windows 8.1
Browser : Firefox 54.0b3 (64-bit)
I tried to reproduce the bug and the bug has been fixed and verified.
You need to log in
before you can comment on or make changes to this bug.
Description
•