1185960 - Add a separator between urlbar dropmarker and stop/reload/go button

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P4)

Description

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

There should be a thin line between the urlbar dropmarker and the stop/reload icon in the url bar.
This code did the trick for me:

> @media (-moz-os-version: windows-win10) {
>   .urlbar-history-dropmarker:after {
>     content: " ";
>     height: 10px;
>     border-right: 1px solid hsla(0, 0%, 0%, 0.1);
>     margin-left: 5px;
>   }
> }

when adding it here:
https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#1499

Comment 1

[Dão Gottwald [:dao]]

Why would this be Windows-10-specific?

Comment 2

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

At this point it's mostly that I'm not sure that code would look good on other versions and I was prioritizing fixing it for Windows 10 in time over fixing it in general.

I do agree however that it should become a general thing at least on Windows.

Comment 3

[Justin Dolske [:Dolske]]

Does seem like this should be a cross-platform thing, although I wouldn't really object to a Windows-only fix in the short-term (unless we have to do more work/hacks to make it Windows-only than just fixing everywhere ;).

Comment 4

[:Gijs (he/him)]

If we're going to do this I almost wonder if it would make more sense for the button to live outside of the inputbox itself so we could also fix bug 941667. Of course we'd need to make sure the size of it always matches, but we have to do that with the forward button too, so that by itself shouldn't be a show-stopper.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

This implements the change across platforms.

Here's a try build for Windows:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d70bf3c28b7b/try-win32/

Comment 6

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #5)
> Created attachment 8655964 [details] [diff] [review]
> patch
> 
> This implements the change across platforms.
> 
> Here's a try build for Windows:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgottwald@mozilla.
> com-d70bf3c28b7b/try-win32/

Thank you for the try-build. Will test it today.

Comment 7

[Stephen Horlander [:shorlander]]

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

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

Can you please make it wider (32px) and add some more spacing between the dropdown and the separator (8px)? This will make it a bigger more obvious hit target. Will attach a screenshot.

Comment 8

[Stephen Horlander [:shorlander]]

Attachment: Wider Stop/Go/Reload Button

Comment 9

[Dão Gottwald [:dao]]

(In reply to Stephen Horlander [:shorlander] from comment #7)
> Comment on attachment 8655964 [details] [diff] [review]
> patch
> 
> Review of attachment 8655964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you please make it wider (32px) and add some more spacing between the
> dropdown and the separator (8px)? This will make it a bigger more obvious
> hit target. Will attach a screenshot.

Since we made the URL bar taller only on Windows 10, this won't look like attachment 8656566 [details] on any OS except Windows 10.

Comment 10

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Stephen Horlander [:shorlander] from comment #7)
> > Comment on attachment 8655964 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8655964 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can you please make it wider (32px) and add some more spacing between the
> > dropdown and the separator (8px)? This will make it a bigger more obvious
> > hit target. Will attach a screenshot.
> 
> Since we made the URL bar taller only on Windows 10, this won't look like
> attachment 8656566 [details] on any OS except Windows 10.

I think it should be ok if it is wider but not as tall on other OSes. Although I think we should look into making it taller on other platforms too (outside of this bug).

Comment 11

[Dão Gottwald [:dao]]

Attachment: wider buttons on Linux

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 13

[Stephen Horlander [:shorlander]]

Comment on attachment 8656623 [details]
wider buttons on Linux

Looks good to me. I will file a follow-up about the height of the location bar… or try to find the one I thought I saw at some point.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/45b16903a80a

Comment 15

[Dão Gottwald [:dao]]

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

Approval Request Comment
[Feature/regressing bug #]: Windows 10 release
[User impact if declined]: Smaller and less clear hit target for the reload/stop/go button. This is particularly relevant for Windows 10 because touch screens are more common there.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: pretty straightforward styling change, low risk
[String/UUID change made/needed]:

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45b16903a80a

Comment 17

[Edo Karabas]

(In reply to Stephen Horlander [:shorlander] from comment #13)
> Comment on attachment 8656623 [details]
> wider buttons on Linux
> 
> Looks good to me. I will file a follow-up about the height of the location
> bar… or try to find the one I thought I saw at some point.

maybe this Bug 1186562

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

This is a low risk but pretty big patch. The sooner we land this in Beta, the sooner we can find out and fix any unexpected fall outs before 41 goes live. Aurora42+, Beta41+

Comment 19

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/272572ec32e4

Comment 20

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

Trying to uplift this to beta, I'm hitting a conflict because identity-block.inc.css doesn't exist on beta.

Can we either get a beta version of the patch posted, confirmation that it's okay to land it without that file, or abandon the uplift request to beta?

Comment 21

[Dão Gottwald [:dao]]

(In reply to Wes Kocher (:KWierso) from comment #20)
> Trying to uplift this to beta, I'm hitting a conflict because
> identity-block.inc.css doesn't exist on beta.
> 
> Can we either get a beta version of the patch posted, confirmation that it's
> okay to land it without that file, or abandon the uplift request to beta?

It's fine without that file. Thanks!

Comment 22

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-beta/rev/54a42cdca2cb

Comment 23

[Sören Hentzschel]

Attachment: screenshot

On OS X (HiDPI) there is no seperator, just a big gap…

Comment 24

[:Gijs (he/him)]

(In reply to Sören Hentzschel from comment #23)
> Created attachment 8657888 [details]
> screenshot
> 
> On OS X (HiDPI) there is no seperator, just a big gap…

OS X misses border-image-slice: 1. The padding is just weird. Did anyone test this on OS X before it landed?

Comment 25

[Pulsebot]

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

Comment 26

[Tim Nguyen :ntim]

(In reply to Pulsebot from comment #25)
> https://hg.mozilla.org/integration/fx-team/rev/ec99f1dfd668

In my testing using developer tools, I think the patch also needs border-style: none;/border-width: 0; otherwise an end border will also appear.

Comment 27

[Tim Nguyen :ntim]

I can post a patch this evening (UTC+1) if I have the time.

Comment 28

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen [:ntim] from comment #26)
> (In reply to Pulsebot from comment #25)
> > https://hg.mozilla.org/integration/fx-team/rev/ec99f1dfd668
> 
> In my testing using developer tools, I think the patch also needs
> border-style: none;/border-width: 0; otherwise an end border will also
> appear.

Oh wow, I didn't know we set transparent left and right borders for toolbar buttons on OS X by default. Seems really weird.

Comment 29

[Dão Gottwald [:dao]]

I filed bug 1202618, but that's not suitable for uplifting.

Comment 30

[Dão Gottwald [:dao]]

Attachment: workaround for bug 1202618

Comment 31

[Dão Gottwald [:dao]]

I also pushed the accidentally missing border-image-slice lines to aurora and beta.

Comment 32

[:Gijs (he/him)]

Comment on attachment 8658124 [details] [diff] [review]
workaround for bug 1202618

We're really going to either need both this and bug 1202287 on beta, or back all of this stuff out of beta. :-\

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 8658124 [details] [diff] [review]
workaround for bug 1202618

Approval Request Comment
[Feature/regressing bug #]: this bug
[User impact if declined]: unintended border on the right side of the stop/reload/go buttons
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: mostly risk-free, there's hardly a way this could break anything further :)
[String/UUID change made/needed]:

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Please create a new bug for the followup. It might be missed by sheriff and relman queries otherwise.
This bug is marked fixed, so are the tracking flags...

Comment 35

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/90199e99d855
https://hg.mozilla.org/releases/mozilla-beta/rev/efcaea2e63d9

Comment 36

[Pulsebot]

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

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec99f1dfd668

Comment 38

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

https://hg.mozilla.org/mozilla-central/rev/d11e3bc39d96

Comment 39

[Sören Hentzschel]

Attachment: screenshot

Now there is a separator on OS X, but with much more padding on the right side than on the left side (left: 11px, right: 20px). It looks a bit strange. Is this intended?

Comment 40

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

(In reply to Sören Hentzschel from comment #39)
> Created attachment 8659396 [details]
> screenshot
> 
> Now there is a separator on OS X, but with much more padding on the right
> side than on the left side (left: 11px, right: 20px). It looks a bit
> strange. Is this intended?

It looks unbalanced to me too. Would you like to file a bug for this and needinfo shorlander?

Comment 41

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40)
> (In reply to Sören Hentzschel from comment #39)
> > Created attachment 8659396 [details]
> > screenshot
> > 
> > Now there is a separator on OS X, but with much more padding on the right
> > side than on the left side (left: 11px, right: 20px). It looks a bit
> > strange. Is this intended?
> 
> It looks unbalanced to me too. Would you like to file a bug for this and
> needinfo shorlander?

That should be bug 1202287.

Comment 42

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #41)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40)
> > (In reply to Sören Hentzschel from comment #39)
> > > Created attachment 8659396 [details]
> > > screenshot
> > > 
> > > Now there is a separator on OS X, but with much more padding on the right
> > > side than on the left side (left: 11px, right: 20px). It looks a bit
> > > strange. Is this intended?
> > 
> > It looks unbalanced to me too. Would you like to file a bug for this and
> > needinfo shorlander?
> 
> That should be bug 1202287.

No, it isn't. The complaint is that the current situation looks like:

-----------------+
\/  |     <>     |
-----------------+

The padding between the stop/go/reload icon and the separator vs. the padding between that icon and the end-edge of the urlbar is bug 1202287. What Sören reported is the inequality in the distance between the separator and the thing to its left (dropdown) and its right (stop/go/reload). bug 1202287 didn't change that, and I agree that it looks bad (though bug 1202287 certainly made it look *less* bad.

We should either make the padding around the stop/go/reload icon smaller, or the padding of the dropdown icon bigger.

Filed bug 1203661.

Comment 43

[Nazir Ahmed Sabbir [:NaSb]]

Reproduced in Nightly 42.0a1 (2015-07-21) on Linux x64.

This Bug is now verified as fixed on Latest Firefox beta 42.0b3 (Build ID: 20151001142456)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0 and

Latest Firefox Developer Edition 43.0a2 (2015-10-01) (Build ID 	20151001004012)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0