1361195 - Regression from bug 1358382 - Missing separators and mispositioned search engines tiles on the searchbar bar if DPI > 100%

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Itiel]

Attachment: Screenshot

This is a regression from bug 1323001.
Windows 10, 24" monitor, screen resolution set to 1920x1080, DPI set to 150%.

See attached screenshot- some separators are missing and search engines tiles are mispositioned on the location bar.
The search bar doesn't seem to be affected.

Bad:
2016-12-13
changeset=0c650d7f26091598dc2aa96088b9756914a63ff1
buildid: 20161213122801

Good: any build before that

Can't reproduce with DPI set to 100%, 125% or even 175%. Higher DPI (200%+) might be affected, though I can't test it on my system.

Comment 1

[roxleitan]

Hi,

I reproduced the issue on Firefox Release 53.0 and Firefox Beta 54.0b4.

I cannot reproduce on latest Nightly 55.0a1(Build ID:20170502030211).

Comment 2

[Itiel]

(In reply to [e-mail] from comment #1)
> Hi,
> 
> I reproduced the issue on Firefox Release 53.0 and Firefox Beta 54.0b4.
> 
> I cannot reproduce on latest Nightly 55.0a1(Build ID:20170502030211).

I was suprised by these findings because this issue can be reproduced also on latest Nightly, so I investigated further.
Retested using Mozregression with setting Firefox to have RTL UI, and now the regression is from bug 1358382 (hopefully I'm right this time). bug 1323001 probably has nothing to do with it.
So it seems the issue affects RTL Firefox users that also have 150% DPI set on their Windows. At least on Windows 10.

Roxana, can you please try to reproduce the issue again with RTL enabled & 150% DPI?

Comment 3

[roxleitan]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170502030211

I still cannot reproduce the issue on the latest Nightly US build with RTL enabled (Force RTL add-on enabled) & 150% DPI neither with Firefox Nightly Ar build & 150% DPI.

I set 150% DPI both from Windows settings and by "layout.css.devPixelsPerPx" preference set to 1.5.

Comment 4

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

Does the version of Windows 10 make a difference? 

Dão, are you able to reproduce this or can you find someone to investigate? We still have a chance to fix this in 54. Too late to fix in 53 though.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #4)
> Does the version of Windows 10 make a difference? 
> 
> Dão, are you able to reproduce this or can you find someone to investigate?
> We still have a chance to fix this in 54. Too late to fix in 53 though.

Well, according to comment 2, Firefox 53 and 54 aren't affected.

Florian, can you take a look please?

Comment 6

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [::dao] from comment #5)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #4)
> > Does the version of Windows 10 make a difference? 
> > 
> > Dão, are you able to reproduce this or can you find someone to investigate?
> > We still have a chance to fix this in 54. Too late to fix in 53 though.
> 
> Well, according to comment 2, Firefox 53 and 54 aren't affected.
> 
> Florian, can you take a look please?

I saw the issue 2 or 3 times (at least twice on Mac), but it's difficult to do something about it without steps to reproduce.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> (In reply to Dão Gottwald [::dao] from comment #5)
> > (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #4)
> > > Does the version of Windows 10 make a difference? 
> > > 
> > > Dão, are you able to reproduce this or can you find someone to investigate?
> > > We still have a chance to fix this in 54. Too late to fix in 53 though.
> > 
> > Well, according to comment 2, Firefox 53 and 54 aren't affected.
> > 
> > Florian, can you take a look please?
> 
> I saw the issue 2 or 3 times (at least twice on Mac), but it's difficult to
> do something about it without steps to reproduce.

Do you recall if this was before or after your changes from bug 1358382? I assume it wasn't an RTL build? What about your DPI settings?

Comment 8

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [::dao] from comment #7)
> (In reply to Florian Quèze [:florian] [:flo] from comment #6)
> > (In reply to Dão Gottwald [::dao] from comment #5)
> > > (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #4)
> > > > Does the version of Windows 10 make a difference? 
> > > > 
> > > > Dão, are you able to reproduce this or can you find someone to investigate?
> > > > We still have a chance to fix this in 54. Too late to fix in 53 though.
> > > 
> > > Well, according to comment 2, Firefox 53 and 54 aren't affected.
> > > 
> > > Florian, can you take a look please?
> > 
> > I saw the issue 2 or 3 times (at least twice on Mac), but it's difficult to
> > do something about it without steps to reproduce.
> 
> Do you recall if this was before or after your changes from bug 1358382? I
> assume it wasn't an RTL build? What about your DPI settings?

After. I assume the RTL issue is separate/older. We've also had a Windows-specific HiDPI-only issue for a while, see bug 1104325. (Drew recently fixed a similar problem for the awesomebar, see bug 1343507).

What I saw on mac is almost definitely a regression from bug 1358382, but I haven't figured out how to reproduce it. I suspect it was related to me I unplugging an external screen and the Firefox window getting moved automatically to the internal screen of my macbook by the OS.

Comment 9

[Dão Gottwald [:dao]]

[Tracking Requested - why for this release]:

Since there's already an older bug on file for the Windows + HiDPI issue, let's keep this bug focused on the recent regression that Florian has also seen on Mac (and in an LTR build).

Comment 10

[Itiel]

It seems this issue is very similiar to bug 1104325 (or is a dupe of it, after all).
Through resizing the location bar and the Firefox window itself, I've managed to reproduce the issue also on 125% and 175% DPI (even on LTR Nightlies), though for some reason reproducing it on 150% is much easier.

A few days ago I've seen this issue fixed, but that was probably due to one of my addons (whose icon was on the left of the location bar) being disabled because it's not a multiprocess compatible addon, thus the location bar got wider a bit and the bug was not apparent. But the bug was reproduced once I resized it.
At this point I'm not even sure if this is a regression or not..

Florian / Dão, feel free to reach out to me via mail if you'd like to test things out on my system for debugging purposes, through remote control.

Comment 11

[Marcia Knous [:marcia]]

Tracking 55+ based on Comment 9.

Comment 12

[Mike Taylor [:miketaylr]]

Dao, given Comment #10, do we still want to track for 55? (Assuming it's caused by an addon...)

Comment 13

[Dão Gottwald [:dao]]

I think you misread comment 10. It wasn't saying that this was caused by an add-on, but that the issue temporarily disappeared due to an add-on button being removed from the toolbar, because apparently this bug depends on the width of the location bar.

Comment 14

[Mike Taylor [:miketaylr]]

Oh yeah, I did. Sorry about that.

Comment 15

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Any updates? Have you got luck to figure out the STR, Florian?

Comment 17

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

I tried and failed to reproduce on Win10, 1920x1080, 150% and other DPI values. I think this should be fix-optional.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #17)
> I tried and failed to reproduce on Win10, 1920x1080, 150% and other DPI
> values. I think this should be fix-optional.

Window size, DPI settings, and what else you have on the toolbar that might change the width of the location bar are likely relevant factors here. You may never see it and other users may see it all the time.

Comment 19

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

The difficulty in reproducing the issue is precisely why I think this qualifies for fix-optional. Especially if unplugging an external screen is necessary (per comment 8). Unless someone can come up with a solid path towards reproducing the issue, we will be having the same conversation on a weekly basis.

Comment 20

[Itiel]

(In reply to Dão Gottwald [::dao] from comment #18)
> (In reply to Panos Astithas [:past] (please needinfo?) from comment #17)
> > I tried and failed to reproduce on Win10, 1920x1080, 150% and other DPI
> > values. I think this should be fix-optional.
> 
> Window size, DPI settings, and what else you have on the toolbar that might
> change the width of the location bar are likely relevant factors here. You
> may never see it and other users may see it all the time.

I second that.
As I've said before, I'm willing to offer my PC for testing and debugging if it'd help. Anyone who thinks this would help I can allow remote control to my Windows.

Comment 21

[Itiel]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #19)
> The difficulty in reproducing the issue is precisely why I think this
> qualifies for fix-optional. Especially if unplugging an external screen is
> necessary (per comment 8). Unless someone can come up with a solid path
> towards reproducing the issue, we will be having the same conversation on a
> weekly basis.

In my specific case, there are no additional monitors connected to my PC except for the one I'm using (24 inch, 1920x1080, DPI set to 150%, full-screened Firefox)

Comment 22

[Florian Quèze [:florian]]

(In reply to Hsin-Yi Tsai (55 Regression Engineering support) [:hsinyi] from comment #15)
> Any updates? Have you got luck to figure out the STR, Florian?

No, I haven't seen it again on Mac.

Before making efforts here, I think we should start by fixing bug 1104325 (which I could reproduce intentionally on Windows HiDPI last time I tried), and then see if anybody can still reproduce here.

Comment 23

[Simona Badau, Desktop QA]

I can easily reproduce this issue with the recommended settings (DPI- 100%, Resolution 1920 x 1080) on Windows 10. I'm using the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1372846#c0.

I have also reproduced this issue on Windows 8.1 laptop (Resolution: 1366x768) on a clean profile by adding an extra icon to the URL Bar (added the Sidebars). After restarting Nightly the issue was not reproducible anymore.

I could not reproduce this issue on Mac OS X.

Comment 24

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

Florian: are either/any of these a priority? Bug 1104325, bug 1372846 look like duplicates but no one is assigned. It sounds like a noticeable regression in 55 since we have several different reports of the issue.

Comment 25

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

I also notice bug 1371697 and a duplicate, bug 1370401 which are related to resizing and the search/url bar.

Comment 26

[Mike Taylor [:miketaylr]]

Attachment: search-dropdown.png

I'm able to reproduce this on my ASUS ZenBook/Win 10 machine, without an external monitor. 

Note, Resolution is set to 1920 x 1080 (Recommended) and scaling is set to 125% (Recommended). I didn't change these.

Like SimonaB said, STR are pretty simple:

1) open Firefox Nightly in a new profile (C:\Program Files\Nightly\firefox.exe -P)
2) without changing window size, search for something, or just click on the search hourglass icon

If you move the window just a few pixels, it fails to reproduce.

Comment 27

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

Per comment 22, marking bug 1104325 as blocking this one, so let's focus our efforts on working around this issue there. jimm suggested that this might actually be a graphics bug, so I'm CCing Milan in case someone can confirm or refute this hypothesis.

Comment 28

[Dão Gottwald [:dao]]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #27)
> jimm suggested that this might
> actually be a graphics bug, so I'm CCing Milan in case someone can confirm
> or refute this hypothesis.

The screenshots don't show search engines missing, i.e. the empty slots are due to engines moving to the next row rather than engines not getting rendered. I don't see how this could be a graphics bug.

Comment 29

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

Sorry, I should have clarified that the potential graphics issue is the different thickness in the line separators as can bee seen in the screenshot from comment 26. Maybe the gfx layer rounds incorrectly the width to 0, 1 or 2 pixels in some configurations. Of course it could also be an error in one of our stylesheets.

Comment 30

[Itiel]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #29)
> Sorry, I should have clarified that the potential graphics issue is the
> different thickness in the line separators as can bee seen in the screenshot
> from comment 26. Maybe the gfx layer rounds incorrectly the width to 0, 1 or
> 2 pixels in some configurations. Of course it could also be an error in one
> of our stylesheets.

In that regard I've filed bug 1373990, and it applies also here.

Comment 31

[Florian Quèze [:florian]]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #29)
> Sorry, I should have clarified that the potential graphics issue is the
> different thickness in the line separators as can bee seen in the screenshot
> from comment 26. Maybe the gfx layer rounds incorrectly the width to 0, 1 or
> 2 pixels in some configurations. Of course it could also be an error in one
> of our stylesheets.

I also think there's likely an underlying graphics or layout bug, bug 1343507 already sounded like we were working around things rather than fixing them.

Comment 32

[Dão Gottwald [:dao]]

A graphics bug wouldn't affect layout, i.e. it wouldn't push engines to the next row. So if anything it would be a layout bug. It's more likely a bug in the script though. The inconsistent separator widths are likely due to us using CSS background for the separators and the way backgrounds are snapped to device pixels.

Comment 33

[Florian Quèze [:florian]]

I was lucky today, and saw the bug again on my macbook on a nightly. And my local build with a window at apparently the exact same size couldn't reproduce, so I could compare the two using the browser console.

Here is the data I extracted:

On the Firefox window where the searchbar panel was broken, document.getElementById("PopupSearchAutoComplete").style returns:

CSS2Properties { min-width: "317.8px", margin-left: "-25px", margin-right: "-21px" }

On the Firefox window where the searchbar panel was displayed correctly, it gave:

CSS2Properties { min-width: "318.067px", margin-left: "-25px", margin-right: "-21px" }


On both, document.getElementById("PopupSearchAutoComplete").clientWidth returns 318


document.getElementById("PopupSearchAutoComplete").style.minWidth is how we set the size of the panel at http://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/browser/components/search/content/search.xml#1096
I think we really need to ensure this value is always an integer (not sure if it should be Math.round or Math.ceil though).

document.getElementById("PopupSearchAutoComplete").clientWidth is how we get the available size of the one-off buttons at
http://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/browser/components/search/content/search.xml#1590
I wonder if we should switch that to using .style.minWidth instead of .clientWidth, to avoid any possible discrepancy between the 2 values. That would also have a side benefit of avoid a sync reflow (is this the last one we have here?). I'm not sure it would work for the awesomebar though.

Comment 34

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

(In reply to Florian Quèze [:florian] [:flo] from comment #33)
> I think we really need to ensure this value is always an integer (not sure
> if it should be Math.round or Math.ceil though).

Judging by this specific instance that you managed to inspect (hooray!) Math.ceil(318.067) != Math.ceil(317.8), but Math.round(318.067) == Math.round(317.8).

Comment 35

[Florian Quèze [:florian]]

The screenshots attached here, and all the comments here seem to be about the searchbar, and bug 1358382 only touched the searchbar. Adjusting summary to match reality.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24)
> Florian: are either/any of these a priority? Bug 1104325, bug 1372846 look
> like duplicates but no one is assigned. It sounds like a noticeable
> regression in 55 since we have several different reports of the issue.

It was very hard to make this a priority given that we had no steps to reproduce and no way to debug, but after comment 33, I'm now able to fix this, and will attach a patch that should be simple enough to uplift.

Comment 36

[Florian Quèze [:florian]]

Attachment: Patch

The added Math.round() call is probably all it takes to make the issue disappear.

The additional changes are an easy optimization for the searchbar case. It doesn't work (but doesn't hurt) for the awesomebar case.
Instead of setting .style.minWidth, the awesomebar code does .setAttribute("width", ... at http://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/browser/base/content/urlbarBindings.xml#1823
I guess I could try to read that attribute, but given that the _openAutocompletePopup method already does 4 things that trigger sync layout flushes, we wouldn't win anything in the short term, and it would just needlessly increase the risk of this patch causing regressions.

Comment 37

[Florian Quèze [:florian]]

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

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

::: browser/components/search/content/search.xml
@@ +1588,5 @@
>              return;
>  
> +          // Using minWidth from the style object should save us a sync reflow
> +          // for the searchbar case.
> +          let panelWidth = parseInt(this.popup.style.minWidth ||

This is wrong on Windows where the border (or padding) also plays a role. This would probably be fixed/handled, but I'm no longer confident in this optimization for a patch we'll want to uplift, so I'll attach a newer version of the patch with only the Math.round addition.

Comment 38

[Florian Quèze [:florian]]

Attachment: Patch with only the rounding

Patch with only the Math.round() addition. This is safer :-).

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/252f0529666b
round the searchbar min-width to ensure one-off buttons are correctly sized on HiDPI, r=past.

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/252f0529666b

Comment 41

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

Want to uplift this fix to 55? It would likely make it into beta 6 later this week.

Comment 42

[Itiel]

Attachment: Screenshot - Nightly 27-06-27

Unfortunately in the latest Nightly (2017-06-27) the issue remains.
See updated screenshots.

Comment 43

[Itiel]

Attachment: Screenshot - Nightly 27-06-27 #2.png

Comment 44

[Florian Quèze [:florian]]

(In reply to ItielMaN from comment #42)
> Created attachment 8881391 [details]
> Screenshot - Nightly 27-06-27
> 
> Unfortunately in the latest Nightly (2017-06-27) the issue remains.
> See updated screenshots.

Your 2 latest screenshots are RTL, if you can now only reproduce with RTL, please file a new bug.

Comment 45

[Florian Quèze [:florian]]

Comment on attachment 8880815 [details] [diff] [review]
Patch with only the rounding

Approval Request Comment
[Feature/Bug causing the regression]: bug 1358382
[User impact if declined]: The layout of the one-off buttons at the bottom of the sidebar panel will sometimes be broken for users with HiDPI displays.
[Is this code covered by automated tests?]: no.
[Has the fix been verified in Nightly?]: no; we don't have steps to reproduce.
[Needs manual test from QE? If yes, steps to reproduce]: Would be nice if we had steps to reproduce, but we don't :-(.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a one-line change just rounding a value that should always be an integer.
[String changes made/needed]: none.

Comment 46

[Itiel]

(In reply to Florian Quèze [:florian] [:flo] from comment #44)
> Your 2 latest screenshots are RTL, if you can now only reproduce with RTL,
> please file a new bug.

Sorry to disappoint, this happens also on latest LTR Nightly :(
See screenshot.

Comment 47

[Itiel]

Attachment: Screenshot on LTR Nightly 29.6

Comment 48

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

Florian, can you take another look? Should we reopen this bug?

Comment 49

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

Andrei, can your team try to verify the fix (or let us know if you confirm it isn't fixed)? Thanks!

Comment 50

[Itiel]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #49)
> Andrei, can your team try to verify the fix (or let us know if you confirm
> it isn't fixed)? Thanks!

A bit hard to verify for other users, considering almost no one can reproduce the bug in the first place..

Comment 51

[Gabi Cheta [:Gabi] Desktop QA]

I reproduced this issue on the latest nightly 56.0a1 BUILD ID:20170704030203 with Win 10 x64.

Comment 52

[Florian Quèze [:florian]]

(In reply to Gabi Cheta from comment #51)
> I reproduced this issue on the latest nightly 56.0a1 BUILD ID:20170704030203
> with Win 10 x64.

Do you have specific advice about how to reproduce?

Comment 53

[Florian Quèze [:florian]]

My current guess is that there's another rounding issue at http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/browser/components/search/content/search.xml#1590, as the clientWidth getter rounds the value, but we would want a floored value.

After the patch I already landed, the panel width is now always an integer, but I suspect on Windows HiDPI the panel has a border with a non-integer width.

I still believe the patch landed here fixes the regression on Mac and would make sense to uplift, but I'm hoping to find a fix for Windows too. If we can't find a real fix, we'll likely want to put in place a hack like what we did in bug 1343507.

If at this point the bug can only be reproduced on Windows, I think we are now talking about bug 1104325, and not a regression from bug 1358382. Given that my patch here changed the size of the panel by rounding it, it's possible that the window sizes that reproduce bug 1104325 have changed.

Comment 54

[Gabi Cheta [:Gabi] Desktop QA]

(In reply to Florian Quèze [:florian] [:flo] from comment #52)
> (In reply to Gabi Cheta from comment #51)
> > I reproduced this issue on the latest nightly 56.0a1 BUILD ID:20170704030203
> > with Win 10 x64.
> 
> Do you have specific advice about how to reproduce?

Hi Florian, retested this and found the following STR:

1. Launch nightly with a clean profile
2. Go to Search bar and start typing something
3. Remove the bookmarks icon from the toolbar
4. Go to search bar again and start typing

Actual Results: 
Search bar re-positions when the icon next to it is removed and the missing separators can be observed.

Comment 55

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

Bug 1373990 comment 8 might be of interest in this case.

Comment 56

[Florian Quèze [:florian]]

I'm still not able to reproduce unfortunately.

On my Windows machine, document.getElementById("PopupSearchAutoComplete").style returns:

CSS2Properties { min-width: "248px", margin-left: "-24.8px", margin-right: "-18.8px" }

I guess we could round these 2 margin values too, but I'm not sure that'll win us anything (especially as the .8 here compensates perfectly the panel's border width).

document.getElementById("PopupSearchAutoComplete").clientWidth returns 246

getComputedStyle(document.getElementById("PopupSearchAutoComplete")).borderLeftWidth gives me 0.8px (same value for borderRightWidth)

Would be useful to know what these values look like just after reproducing the bug.

Here are the steps to get them:
1. Tools -> Web Developer -> Toggle Tools (or Ctrl+shift+I)
2. Click on the "Toolbar Options" icon (a gear icon 4th from the right for me).
3. Check "Enable browser chrome and add-on debugging toolboxes" (from the 'Advanced settings' section)
4. Open the searchbar panel and verify that the bug is reproduced
5. Open the browser console (Tools -> Web Developer -> Browser Console, or Ctrl+shift+J)
6. Paste the following pieces of code and press enter after each:
 document.getElementById("PopupSearchAutoComplete").style
 document.getElementById("PopupSearchAutoComplete").clientWidth
 getComputedStyle(document.getElementById("PopupSearchAutoComplete")).borderLeftWidth
 getComputedStyle(document.getElementById("PopupSearchAutoComplete")).borderRightWidth

ItielMaN and/or Gabi, could you please do this for me? Thanks!

Comment 57

[Itiel]

document.getElementById("PopupSearchAutoComplete").style
CSS2Properties { min-width: "221px", margin-left: "-4.66666px", margin-right: "-24.6667px" }

document.getElementById("PopupSearchAutoComplete").clientWidth
220

getComputedStyle(document.getElementById("PopupSearchAutoComplete")).borderLeftWidth
"0.666667px"

getComputedStyle(document.getElementById("PopupSearchAutoComplete")).borderRightWidth
"0.666667px"

These results were taken from the latest Nightly, of course. Full screened Nightly, DPI set to 150%, screen resolution 1920x1080.

Comment 58

[Florian Quèze [:florian]]

(In reply to ItielMaN from comment #57)
> document.getElementById("PopupSearchAutoComplete").style
> CSS2Properties { min-width: "221px", margin-left: "-4.66666px",
> margin-right: "-24.6667px" }
> 
> document.getElementById("PopupSearchAutoComplete").clientWidth
> 220
> 
> getComputedStyle(document.getElementById("PopupSearchAutoComplete")).
> borderLeftWidth
> "0.666667px"
> 
> getComputedStyle(document.getElementById("PopupSearchAutoComplete")).
> borderRightWidth
> "0.666667px"
> 
> These results were taken from the latest Nightly, of course. Full screened
> Nightly, DPI set to 150%, screen resolution 1920x1080.

Thanks!

I've finally managed to reproduce on a 1854px wide window, at 175%.

One difference I observe between the values in comment 56 and the values in comment 57 is that the difference between min-width and clientWidth is 2px when things work, and 1px when thinks look broken.

My new hypothesis is that we are in trouble when round(borderWidth) * 2 != round(borderWidth * 2), so when the border width is > 0.5 and < 0.75.
That's 0.666667px in your example, and 0.566667px in the case where I reproduced myself.

I still have some doubts about this, because while trying to reproduce at some point I had a 0.666667px border width, and the layout wasn't broken. So there may be yet another issue somewhere (I vaguely suspect a problem between 0.6667px, 0.66666px and 0.666667px, because when adding/subtracting them, the result would not be an integer).

fwiw, rounding the margin-left and margin-right values doesn't help; at least in the case I reproduced.

Comment 59

[Florian Quèze [:florian]]

Comment on attachment 8880815 [details] [diff] [review]
Patch with only the rounding

The case I managed to reproduce in comment 58 isn't broken if I revert this patch, so I'm not sure anymore that we should uplift.

Comment 60

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Florian, is this a wontfix for 55 then? Based on comment 58 I gathered that the fix in 56 works for most cases except one edge case. If that is the case, shouldn't we uplift this to 55? Perhaps I misunderstand.

Comment 61

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #58)

> My new hypothesis is that we are in trouble when round(borderWidth) * 2 !=
> round(borderWidth * 2), so when the border width is > 0.5 and < 0.75.
> That's 0.666667px in your example, and 0.566667px in the case where I
> reproduced myself.

Following this hypothesis, I pushed to try a patch that rounds to the closest half pixel (this fixes the bug for the case I could locally reproduce) : https://treeherder.mozilla.org/#/jobs?repo=try&revision=dedd07a96aa41fa8de652066bb4697204d11af6b

The reason why I think '2' (or half pixel) matters here is that the clientWidth is rounded, and has a value that seems to be the min-width we set, minus twice the border width (once for each side).

ItielMaN and/or Gabi, could you please try a build from that try push, and see if you can still reproduce?

Here is a link to the Win64 build: https://queue.taskcluster.net/v1/task/ZwinlVOlSiysUUyl4WHMbg/runs/0/artifacts/public/build/target.zip

Comment 62

[Florian Quèze [:florian]]

(In reply to Ritu Kothari (:ritu) from comment #60)
> Hi Florian, is this a wontfix for 55 then?

I'm still trying to fix it for real this time. How much time do we have left to uplift a simple patch to 55?

> Based on comment 58 I gathered
> that the fix in 56 works for most cases except one edge case. If that is the
> case, shouldn't we uplift this to 55? Perhaps I misunderstand.

The patch that landed here fixes an edge case... at the cost of causing the same regression for another similar edge case. I have no data to say which of the 2 edge cases is most frequent, so I don't know if uplifting the existing patch would be overall an improvement or a regression. This is for Windows HiDPI. For the Mac case, I think the existing patch fixed it.

Comment 63

[Itiel]

(In reply to Florian Quèze [:florian] [:flo] from comment #61)
> Here is a link to the Win64 build:
> https://queue.taskcluster.net/v1/task/ZwinlVOlSiysUUyl4WHMbg/runs/0/
> artifacts/public/build/target.zip

Any chace for a 32 bit build? I'm running Windows 10 x86.

Comment 64

[Florian Quèze [:florian]]

(In reply to ItielMaN from comment #63)
> (In reply to Florian Quèze [:florian] [:flo] from comment #61)
> > Here is a link to the Win64 build:
> > https://queue.taskcluster.net/v1/task/ZwinlVOlSiysUUyl4WHMbg/runs/0/
> > artifacts/public/build/target.zip
> 
> Any chace for a 32 bit build? I'm running Windows 10 x86.

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-dedd07a96aa41fa8de652066bb4697204d11af6b/try-win32/

Comment 65

[Itiel]

(In reply to Florian Quèze [:florian] [:flo] from comment #64)
> (In reply to ItielMaN from comment #63)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #61)
> > > Here is a link to the Win64 build:
> > > https://queue.taskcluster.net/v1/task/ZwinlVOlSiysUUyl4WHMbg/runs/0/
> > > artifacts/public/build/target.zip
> > 
> > Any chace for a 32 bit build? I'm running Windows 10 x86.
> 
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> dedd07a96aa41fa8de652066bb4697204d11af6b/try-win32/

I think you've done it!
When running the build you've sent with the same profile I'm always using (without changing any of Nightly's environment), I can't see the issue anymore, for both RTL+LTR.

Comment 66

[Itiel]

False alarm, the issue is reproducible after resizing the search bar :(

Comment 67

[Itiel]

Details from the Browser Console running your build after resizing the search bar and re-reproduing the issue:

document.getElementById("PopupSearchAutoComplete").style
CSS2Properties { min-width: "169px", margin-left: "-4.66666px", margin-right: "-24.6667px" }

document.getElementById("PopupSearchAutoComplete").clientWidth
168

getComputedStyle(document.getElementById("PopupSearchAutoComplete")).borderLeftWidth
"0.666667px"

getComputedStyle(document.getElementById("PopupSearchAutoComplete")).borderRightWidth
"0.666667px"

Comment 68

[Florian Quèze [:florian]]

Here is another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=685d4f3bc948997b1c13d3b72c3200f8cd3e8552

I'm giving up on finding an actual fix, this is just making the workaround from bug 1343507 also apply to the searchbar, it should hopefully hide the problem and let us move on. If it works as expected, I'll attach it in bug 1104325 and request review there.

Here is a direct link to the Win32 build: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-685d4f3bc948997b1c13d3b72c3200f8cd3e8552/try-win32/

Comment 69

[Itiel]

(In reply to Florian Quèze [:florian] [:flo] from comment #68)
> Here is another try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=685d4f3bc948997b1c13d3b72c3200f8cd3e8552
> 
> I'm giving up on finding an actual fix, this is just making the workaround
> from bug 1343507 also apply to the searchbar, it should hopefully hide the
> problem and let us move on. If it works as expected, I'll attach it in bug
> 1104325 and request review there.
> 
> Here is a direct link to the Win32 build:
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 685d4f3bc948997b1c13d3b72c3200f8cd3e8552/try-win32/

I've tried reproducing running this build with resizing the search bar so that the output values from comment 57 and comment 67 would match my current values (for both RTL+LTR) and it seems okay. Of course I've tried resizing the search bar randomly multiple times and the issue doesn't reproduce anymore.

Note that bug 1373990 can still be reproduced. Could fixing bug 1373990 affect this bug in the future?

Comment 70

[Florian Quèze [:florian]]

(In reply to ItielMaN from comment #69)
> (In reply to Florian Quèze [:florian] [:flo] from comment #68)
> > Here is another try push:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=685d4f3bc948997b1c13d3b72c3200f8cd3e8552
> > 
> > I'm giving up on finding an actual fix, this is just making the workaround
> > from bug 1343507 also apply to the searchbar, it should hopefully hide the
> > problem and let us move on. If it works as expected, I'll attach it in bug
> > 1104325 and request review there.
> > 
> > Here is a direct link to the Win32 build:
> > https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> > 685d4f3bc948997b1c13d3b72c3200f8cd3e8552/try-win32/
> 
> I've tried reproducing running this build with resizing the search bar so
> that the output values from comment 57 and comment 67 would match my current
> values (for both RTL+LTR) and it seems okay. Of course I've tried resizing
> the search bar randomly multiple times and the issue doesn't reproduce
> anymore.

:-). Thanks!

> Note that bug 1373990 can still be reproduced. Could fixing bug 1373990
> affect this bug in the future?

Hopefully it won't, but yes it can. We'll need to be careful and re-test this when a fix for bug 1373990 is ready.

Comment 71

[Gabi Cheta [:Gabi] Desktop QA]

Issue is still reproducible on the try push build, on Windows 10 x64.
We'll re-verify when Bug 1373990 gets fixed.

Comment 72

[Florian Quèze [:florian]]

(In reply to Gabi Cheta from comment #71)
> Issue is still reproducible on the try push build, on Windows 10 x64.

Are you talking about the build in comment 61 or the one in comment 68?

Comment 73

[Gabi Cheta [:Gabi] Desktop QA]

(In reply to Florian Quèze [:florian] [:flo] from comment #72)
> (In reply to Gabi Cheta from comment #71)
> > Issue is still reproducible on the try push build, on Windows 10 x64.
> 
> Are you talking about the build in comment 61 or the one in comment 68?

Push build from comment 61.

Comment 74

[Florian Quèze [:florian]]

Comment on attachment 8880815 [details] [diff] [review]
Patch with only the rounding

Approval Request Comment
[Feature/Bug causing the regression]: bug 1358382
[User impact if declined]: broken searchbar one-off buttons layout for some window sizes on HiDPI screens.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not really
[Needs manual test from QE? If yes, steps to reproduce]: same as bug 1104325, but also applies at least on Mac (not sure about Linux).
[List of other uplifts needed for the feature/fix]: bug 1104325
[Is the change risky?]: not really.
[Why is the change risky/not risky?]: The change in itself is trivial, but any change we make to this code changes the window sizes that reproduce bug 1104325, so I'm not confident uplifting this simple patch without also uplifting bug 1104325.
[String changes made/needed]: none

Comment 75

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

Comment on attachment 8880815 [details] [diff] [review]
Patch with only the rounding

Let's try landing this for beta 9. Plus the patch in bug 1104325. 
And then we'll verify it in beta.  Thanks!

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2a6eab645431

Comment 77

[Gabi Cheta [:Gabi] Desktop QA]

Issue verified on Fx 56.0a1 and Fx 55.0b12-build 1.
Tests were performed on Win 10x64, Win 7x64 and macOS 10.12.5