1115307 - Search bar alignment fixes and cleanup

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

followup from bug 1101996

Comment 1

[Florian Quèze [:florian]]

Looks like a nice cleanup! :-)

I'm probably not going to fully review/test this before January. Am I correct in assuming that you are only targeting 37?

Comment 2

[Dão Gottwald [:dao]]

I'd like to uplift to 36 at least, and 35 if time allows. Do we already use this code with rtl locales? It's kind of broken there, right? This patch should help with that.

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8541152 [details] [diff] [review]
patch

The code looks good. I haven't fully understood (yet) the platform specific padding/margin changes.

On Mac retina, the panel appears a few pixels to the left of the textfield: http://i.imgur.com/cmbidiY.png
On the non-retina external screen, it looks good (when zooming it seems to be one pixel to the right; but that's not noticeable without zooming and I won't claim the alignment was perfect before either).

I don't see anything obviously causing this difference

I haven't been able to test on Windows yet, as the patch doesn't apply cleanly above my current checkout, and I need to update Visual Studio before I can build newer code.

Comment 4

[Florian Quèze [:florian]]

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

> On Mac retina, the panel appears a few pixels to the left of the textfield:
> http://i.imgur.com/cmbidiY.png
> On the non-retina external screen, it looks good (when zooming it seems to
> be one pixel to the right; but that's not noticeable without zooming and I
> won't claim the alignment was perfect before either).
> 
> I don't see anything obviously causing this difference

What happens is that the search glass icon shrinks when moving the window to the retina screen.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> What happens is that the search glass icon shrinks when moving the window to
> the retina screen.

This doesn't have anything to do with my patch, does it?

Comment 6

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to Florian Quèze [:florian] [:flo] from comment #4)
> > What happens is that the search glass icon shrinks when moving the window to
> > the retina screen.
> 
> This doesn't have anything to do with my patch, does it?

This doesn't happen without applying your patch.

Comment 7

[Dão Gottwald [:dao]]

Ok, it looks like the width being set on .searchbar-search-button for high-DPI is off because of the padding I added.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v2

This should fix that

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8546689 [details] [diff] [review]
patch v2

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

Now looks as good or better than before the patch on all the platforms I tested (OS X 10.10 both retina and non-retina, Windows 7, Ubuntu), thanks!

Comment 10

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/05511d05f1e0

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05511d05f1e0

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8546689 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1088660
[User impact if declined]: some minor misalignment, likely worse glitches in right-to-left locales
[Describe test coverage new/current, TBPL]: no test coverage for this kind of stuff
[Risks and why]: somewhere between low and medium. not a very complicated patch, but not the simplest either. however, any fallout (not that I anticipate it) would likely be minor
[String/UUID change made/needed]: none

Comment 13

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [:dao] from comment #12)

I think you meant to request beta approval for 36 rather than aurora.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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