Closed Bug 989701 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Theme, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- verified
firefox30 --- verified
firefox31 + verified

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P3])

Attachments

(2 files, 2 obsolete files)

I suspect bug 477948 caused this.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Attached patch Patch v1: fix urlbar alignment (obsolete) — Splinter Review
This probably also fixes bug 989466. I still need to verify this.
Attachment #8399482 - Flags: review?(jaws)
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.
Attachment #8399482 - Flags: feedback-
I mean only the negative top margin, of course.
This will need uplifting if 477948 lands
(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.
(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.
Blocks: 989466
Attached patch Patch v2: fix urlbar alignment (obsolete) — Splinter Review
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?
Attachment #8399482 - Attachment is obsolete: true
Attachment #8399553 - Flags: feedback?(dao)
Attached patch patch v3Splinter Review
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.
Assignee: mdeboer → dao
Attachment #8399553 - Attachment is obsolete: true
Attachment #8399553 - Flags: feedback?(dao)
Attachment #8399564 - Flags: review?(mdeboer)
Comment on attachment 8399564 [details] [diff] [review]
patch v3

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

<hugs> this works :)

Thanks Dão!
Attachment #8399564 - Flags: review?(mdeboer) → review+
Whiteboard: [Australis:P3]
https://hg.mozilla.org/mozilla-central/rev/08a9cf1f40b5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
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.
Attachment #8399564 - Flags: approval-mozilla-beta?
Attachment #8399564 - Flags: approval-mozilla-aurora?
Attachment #8399564 - Flags: approval-mozilla-beta?
Attachment #8399564 - Flags: approval-mozilla-beta+
Attachment #8399564 - Flags: approval-mozilla-aurora?
Attachment #8399564 - Flags: approval-mozilla-aurora+
Mike, Firefox 30 was marked as unaffected and you requested an uplift. Mistake in the tracking flags usage?
Flags: needinfo?(mdeboer)
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+ ?
Flags: needinfo?(mdeboer) → needinfo?(sledru)
Affected for both should should be fine.
Flags: needinfo?(sledru)
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa
You need to log in before you can comment on or make changes to this bug.