1104325 - [Windows HiDPI] search engines appearing in the incorrect row when resizing

Status: VERIFIED FIXED

(Firefox :: Search, defect, P3)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: missingBorders.gif

Sometimes the borders between the search engines are missing after re-sizing fx several times. I've only managed to reproduce this on my Win 7 x64 VM machine. However, I can reproduce the issue consistently after re-sizing fx a few times.

Maybe someone with a dedicated Win 7 machine (not a VM) can try and see if it's only related to the VM? (don't see how though)

STR:

1) Type something into the search toolbar
2) Once the search panel appears, click somewhere else to dismiss the panel
3) Re-size fx
4) Press down to receive the search toolbar panel once again
5) Just keep repeating #1 - #4 a few times and you'll eventually run into the issue

Attached a .gif and a .png

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Attachment: missingBordersIssue.png

Comment 2

[Florian Quèze [:florian]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #1)
> Created attachment 8527957 [details]
> missingBordersIssue.png

The problem here isn't missing borders: you have the last item of the first row displayed on the second row, and the last item of the second row displayed on a third row that shouldn't exist.

The problem likely happens for some specific sizes of the search box; there's likely a margin or a border that isn't correctly taken into account in the width computations.

Comment 3

[Kamil Jozwiak [:kjozwiak]]

Reproduced using a newer build (used 34.0b11 originally):
- http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-win32/1416560414/

Comment 4

[Florian Quèze [:florian]]

The screenshot looks like it was made on a HiDPI screen. I just tried to reproduce on my Windows 7 thinkpad, but couldn't. Could this bug be HiDPI-only?

Comment 5

[Kamil Jozwiak [:kjozwiak]]

It looks like Andrei ran into the same problem but using different STR in bug # 1104748 without a HiDPI screen. I'll try disabling "Use full resolution for Retina Display" on the VM and see if I can reproduce the problem.

Comment 6

[Florian Quèze [:florian]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #5)
> It looks like Andrei ran into the same problem but using different STR in
> bug # 1104748

His screenshot looks like a different bug to me. On your screenshot we have the correct count of items per row but the last one wraps to the next line, creating a large amount of empty space on the right side. On his screenshot there's no large whitespace on the right side.

Comment 7

[Kamil Jozwiak [:kjozwiak]]

I couldn't reproduce the problem on the same machine once I turned off "Use full resolution for Retina Display". So it's probably related to HiDPI.

I was thinking it was a similar issue because it has some similarities when it comes to the missing borders between the search engines. Perhaps it's hitting the same margin or a border computation error but looks different as there's more search engines installed/being listed. Could be unrelated though.

Comment 8

[Florian Quèze [:florian]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #7)

> I was thinking it was a similar issue because it has some similarities when
> it comes to the missing borders between the search engines.

The bug here seems like we don't detect correctly the width of the panel, and are wrong by only a pixel or two. I fixed similar bugs before while coding this; it was typically a problem with detecting the border or margin sizes of the panel.

The problem in bug # 1104748 is that we don't support showing a large engine name at the top of a small panel, and that messes up the layout.

Comment 10

[Florian Quèze [:florian]]

There's more details in bug 1187620 about which DPI sizes show the problem.

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> There's more details in bug 1187620 about which DPI sizes show the problem.

Right. You don't need particular hardware - you can just set the DPI setting in Windows' control panel.

Comment 13

[:shell escalante]

Florian has thoughts on what might be broken...

Comment 14

[Marco Bonardo [:mak]]

So, I tried reproducing this on Windows 10 with all DPI settings (125, 150, 175), insisting a little bit more on 150%, that is what most people had problems with.
After a good 40 minutes of resizing the window (large and small amounts) and testing the panel, I was still unable to reproduce any kind of rounding problem.
So, either my system is not adapt to reproduce the bug, or something else fixed some related hi-dpi stuff so now it's no more a bug.

Regardless doesn't look like I may be useful here :/

Comment 15

[:Gijs (out for now; he/him)]

Attachment: Current nightly

STR:

1. Use 150% DPI on current win10 insider preview (win10 regular release probably works too)
2. new profile on current nightly
3. resize window so it has outerWidth 1080 (you can check in the browser console, or use a screen that renders at 1440px width with this DPI setting (in which case that window size will be the default on Windows).

AR:
the screenshot attached

ER: correctly wrapping items. :-)

I'm bringing this machine to Orlando if that helps.

Comment 16

[Kamil Jozwiak [:kjozwiak]]

Reproduced it as well using Win 7 x64 VM + retina following the steps from comment #15.

Comment 17

[Marco Bonardo [:mak]]

On my box it doesn't reproduce at all :(

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

I can't reproduce with the STR in comment 15 :/

Comment 19

[Florian Quèze [:florian]]

Attachment: Patch

This expands the workaround added in bug 1343507 for the awesomebar panel, so that it also applies to the searchbar. I tried to find a real fix in bug 1361195 but gave up.

Comment 20

[Drew Willcoxon :adw]

Comment on attachment 8884784 [details] [diff] [review]
Patch

Review of attachment 8884784 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f7e63a714f
[Windows HiDPI] expand the awesomebar panel workaround to also apply to the searchbar so that one-off buttons are laid out correctly even when there are rounding issues, r=adw.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3f7e63a714f

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8884784 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: AFAIK this bug has always been there since we landed one-off buttons in the searchbar. We didn't worry much about it because back in 2014 Windows HiDPI wasn't a common configuration.
[User impact if declined]: Broken searchbar one-off buttons layout for some window sizes with some DPI settings on Windows.
[Is this code covered by automated tests?]: no.
[Has the fix been verified in Nightly?]: no.
[Needs manual test from QE? If yes, steps to reproduce]: Try to resize the window, add/remove toolbar buttons, resize the searchbar, change the Windows DPI settings, and check that the searchbar one-off buttons don't break. Also check that we didn't regress bug 1343507.
[List of other uplifts needed for the feature/fix]: Bug 1361195 (which is a regression in 55 causing the same issue but on all platforms) should be uplifted at the same time.
[Is the change risky?]: not really
[Why is the change risky/not risky?]: it's only moving an existing workaround to apply more broadly so should not be risky... but this code has been a bit fragile in the past, so I would be more confident if QE could verify.
[String changes made/needed]: none.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8884784 [details] [diff] [review]
Patch

Let's uplift the fix and verify in beta. 
Kamil, maybe you can take a look, or andrei's team may already have a high dpi test ongoing for beta.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fdedc974cf3b

Comment 26

[Kamil Jozwiak [:kjozwiak]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24)
> Kamil, maybe you can take a look, or andrei's team may already have a high
> dpi test ongoing for beta.

I can take a look once I get back from PTO on the 16th.

Comment 27

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue with Firefox 45.0a1 (2015-12-04), under Windows 10x64, using the STR from Comment 15.
The issue is no longer reproducible on Firefox 56.0a1 (2017-07-13), or on Firefox 55.0b9-build 2. 
Tests were performed under Windows 10x64.