989701 - Location bar text sits one pixel too high unless the forward button is enabled

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Attachment: Screenshot from 2014-03-30 08:20:15.png

I suspect bug 477948 caused this.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: fix urlbar alignment

This probably also fixes bug 989466. I still need to verify this.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8399482 [details] [diff] [review]
Patch v1: fix urlbar alignment

Why is this negative margin there at all? We don't have it on Windows, as far as I know.

Comment 3

[Dão Gottwald [:dao]]

I mean only the negative top margin, of course.

Comment 4

[Benjamin Kerensa [:bkerensa]]

This will need uplifting if 477948 lands

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #2)
> Why is this negative margin there at all? We don't have it on Windows, as
> far as I know.

Wow, f-, you must really be sure this is fundamentally invalid.

To align the `urlbar-back-button-clip-path` correctly, I made sure that the `#urlbarwrapper` element moves down 5px - see http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#930 - and this is different than Windows, even though the clip-path is shared between both OSes.
I needed to do this, because the clip-path changed since bug 893661.

To keep the `#urlbar` element at the correct position, I applied a negative margin of 4px to move it back. This is wrong, because the original margin defines 2px on top and bottom, so it needed to be 5px - 2px = 3px to compensate, not 4px. That's what this patch does.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > Why is this negative margin there at all? We don't have it on Windows, as
> > far as I know.
> 
> Wow, f-, you must really be sure this is fundamentally invalid.

Pretty sure it's the wrong approach, yes.

Windows uses -moz-box-align:center for #urlbar-container, Linux uses -moz-box-align:stretch. If you want to share the clip path, you need to be consistent there. If the clip path still doesn't fit after making this consistent, then it probably shouldn't be shared between Windows and Linux.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: fix urlbar alignment

You were spot on, re. the box-align discrepancy!

So changing that yields a tad less undesired behavior, but I still had to correct the `#urlbar-wrapper` and `#urlbar` upward by one pixel to correct the clip-path.
I'm kinda hoping you might find this acceptable or have an idea how to correct this last bit?

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v3

Stealing; this completely gets rid of the transient margin-top and reduces the default margin for the url and search bars to be in line with the Windows styling.

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8399564 [details] [diff] [review]
patch v3

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

<hugs> this works :)

Thanks Dão!

Comment 10

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/08a9cf1f40b5

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08a9cf1f40b5

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8399564 [details] [diff] [review]
patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 477948
User impact if declined: This needs to be uplifted together with bug 477948, since it's a regression.
Testing completed (on m-c, etc.): landed on m-c.
Risk to taking this patch (and alternatives if risky): minor.
String or IDL/UUID changes made by this patch: n/a.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Mike, Firefox 30 was marked as unaffected and you requested an uplift. Mistake in the tracking flags usage?

Comment 14

[Mike de Boer [:mikedeboer]]

Sylvestre, I requested uplift for this one too, because it technically belongs together with the patches in bug 477948.

But since these haven't been uplifted yet, I'm not sure how to set the tacking flags for this one... maybe better to use tracking-firefoxX+ ?

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Affected for both should should be fine.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/5f955bd6ff35


Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/4b355a2745cd

Comment 17

[Petruta Horea [:phorea], PTO until 8/22/25]

Verified that the url is correctly aligned on Firefox 29 beta 8 (20140414143035), latest Aurora 30.0a2 and latest Nightly 31.0a1 2014-04-15.