Closed Bug 1115307 Opened 5 years ago Closed 5 years ago

Search bar alignment fixes and cleanup

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.3 - 12 Jan
Tracking Status
firefox35 --- wontfix
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: polish, rtl)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
followup from bug 1101996
Attachment #8541152 - Flags: review?(florian)
Flags: qe-verify-
Flags: firefox-backlog+
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?
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.
Keywords: polish, rtl
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.
Attachment #8541152 - Flags: feedback+
(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.
(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?
(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.
Ok, it looks like the width being set on .searchbar-search-button for high-DPI is off because of the padding I added.
Attached patch patch v2Splinter Review
This should fix that
Attachment #8541152 - Attachment is obsolete: true
Attachment #8541152 - Flags: review?(florian)
Attachment #8546689 - Flags: review?(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!
Attachment #8546689 - Flags: review?(florian) → review+
https://hg.mozilla.org/mozilla-central/rev/05511d05f1e0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
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
Attachment #8546689 - Flags: approval-mozilla-aurora?
(In reply to Dão Gottwald [:dao] from comment #12)

I think you meant to request beta approval for 36 rather than aurora.
Attachment #8546689 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8546689 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.