1203481 - All dividers in the URL bar should have the same style

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect, P5)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: screen shot

There are currently two permanent dividers in the URL bar: the one between URL and identity block and the one between the stop/go/reload button and the dropmarker.

They have slightly different colors and sizes. One way or another they should look the same.

Comment 1

[Ray Lin[:ralin]]

Hi!!

I've made a patch, but have no idea whom to contact.  

Philipp, should I contact to you? Thanks.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

How about upload the patch to the bug here and provide a screenshot :) ?

Comment 3

[Ray Lin[:ralin]]

Attachment: Bug-1203481-All-dividers-in-the-URL-bar-to-same-styl.patch

In this patch, I adjusted the height of .urlbar-*-buttons to the same as #urlbar's. 

Screenshot:
https://www.dropbox.com/s/m7my0mjfshmre8e/Screenshot%202016-03-29%2017.39.34.png?dl=0

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8735787 [details] [diff] [review]
Bug-1203481-All-dividers-in-the-URL-bar-to-same-styl.patch

Looks good! Thanks!
You'll also need to request a code review from an engineer before landing it.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8735787 [details] [diff] [review]
Bug-1203481-All-dividers-in-the-URL-bar-to-same-styl.patch

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

I don't think this is right on Windows. Here is how it looks on Windows, which shows that the separator next to the stop/go/reload button is now a few pixels taller than the identity-block separator: http://screencast.com/t/R0Sf4TRrYU3X

Comment 6

[Ray Lin[:ralin]]

That's my fault that I didn't verify it again on Windows. I don't have handy Windows environment now, I'll fix it this weekend.

Thanks for your reviewing.

Comment 7

[Ray Lin[:ralin]]

Attachment: MozReview Request: Bug 1203481 - All dividers in the URL bar to same style; r?jaws

Review commit: https://reviewboard.mozilla.org/r/46293/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46293/

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8741214 [details]
MozReview Request: Bug 1203481 - All dividers in the URL bar to same style; r?jaws

https://reviewboard.mozilla.org/r/46293/#review43775

Unfortunately these are still not the same on my machine, which is running Windows 10 in HiDPI. Zoom in on this screenshot to see that the stop-go-reload separator is about 2 pixels shorter (1 on the top and 1 on the bottom) than the site-identity separator.

http://content.screencast.com/users/j.wein/folders/Jing/media/4fe5a065-75c4-4b9c-99f8-e61482beff8f/2016-04-18_1335.png

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

In practice, it is really hard to tell that these two are different, so I am willing to grant r+ on this if you'd like to land what you have in the as-is.

Comment 10

[Ray Lin[:ralin]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> In practice, it is really hard to tell that these two are different, so I am
> willing to grant r+ on this if you'd like to land what you have in the as-is.

Thank you Jared! 

If this bug is not urgent, I'd like to fix it after Bug 887934. I think I'll have more comprehensive environment to work on UI tweaks on Windows then.

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8741214 [details]
MozReview Request: Bug 1203481 - All dividers in the URL bar to same style; r?jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46293/diff/1-2/

Comment 12

[Ray Lin[:ralin]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> In practice, it is really hard to tell that these two are different, so I am
> willing to grant r+ on this if you'd like to land what you have in the as-is.

Sorry for sooooo late update. Little tweak to Windows theme, now the dividers' height should be close to be the same.

Thank you so much :)

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8741214 [details]
MozReview Request: Bug 1203481 - All dividers in the URL bar to same style; r?jaws

https://reviewboard.mozilla.org/r/46293/#review54860

On my HiDPI resolution the url-stop-go splitter actually has one extra pixel on the top and bottom, though this difference is not noticeable unless you place them very close together and zoom in using a graphics program. The patch as-is is a net improvement, so let's take this and move on from here :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/5662da82f125
All dividers in the URL bar to same style; r=jaws

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5662da82f125

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5662da82f125

Comment 17

[Dão Gottwald [:dao]]

Has anyone tested this on Linux? Also, what's the point of the negative top and bottom margins that are then seemingly neutralized by the top and bottom padding?

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8741214 [details]
MozReview Request: Bug 1203481 - All dividers in the URL bar to same style; r?jaws

I've backed this out: https://hg.mozilla.org/integration/fx-team/rev/846fcdf259a5

This patch is definitely wrong on Linux, may need another look on other platforms too. Comment 13 isn't really reassuring. We should aim for pixel-perfection here unless there's some technical reason why we can't.

Comment 19

[David Critchley (:dcritch)]

Would it would be a better fix to make the divider it's own element and styles, as opposed to using borders on existing elements which may vary in height? This way the styling of the divider would be consistent for this and future applications.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

I'll defer to Dao on this.

Comment 21

[Dão Gottwald [:dao]]

I think if we can fix this with simple CSS adjustments, we should just do that. If there are reason why that would be particularly hard, we can consider adding a dedicated element for the separator.

Comment 22

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1203481 - All dividers in the URL bar should have the same style

Fix firefox dividers to have same style

Comment 23

[David Critchley (:dcritch)]

Comment on attachment 8771119 [details] [diff] [review]
Attachment to Bug 1203481 - All dividers in the URL bar should have the same style

styled the top/bottom padding on the elements the same so hopefully this fixes the issue you were seeing. Tested on OS X, Linux and Windows, divider was the same height and color on all 3.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8771119 [details] [diff] [review]
Attachment to Bug 1203481 - All dividers in the URL bar should have the same style

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

Looks good to me but please wait for Dao to review this as well.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8771119 [details] [diff] [review]
Attachment to Bug 1203481 - All dividers in the URL bar should have the same style

couple of points:

- does this actually change anything? Could you attach before/after screenshots?

- https://hg.mozilla.org/mozilla-central/rev/3705eb1dc2cc moved the separator from the end of #identity-box and #urlbar-display-box to the start of #urlbar-display-box and .urlbar-input-box. This is confusing and seems unnecessary; I think we should go back to setting the border on #identity-box and #urlbar-display-box.

- on Linux there's also this rule that should be removed for that separator to have the expected height: https://dxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#966

Comment 26

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #25)
> - on Linux there's also this rule that should be removed for that separator
> to have the expected height:
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.
> css#966

permalink: https://hg.mozilla.org/mozilla-central/annotate/091b06284ffe/browser/themes/linux/browser.css#l966

Comment 27

[Itiel]

I think this can be closed now with Photon in place.