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

VERIFIED FIXED in Firefox 41

Status

()

defect
P4
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: phlsa, Assigned: dao)

Tracking

Trunk
Firefox 43
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox41 fixed, firefox42 verified, firefox43 verified)

Details

Attachments

(5 attachments, 2 obsolete attachments)

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
Why would this be Windows-10-specific?
Flags: needinfo?(philipp)
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.
Flags: needinfo?(philipp)
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 ;).
Priority: -- → P4
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.
Assignee: nobody → dao
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Windows 10: Add a separator between urlbar dropmarker and stop/reload button → Add a separator between urlbar dropmarker and stop/reload button
Version: 40 Branch → Trunk
Summary: Add a separator between urlbar dropmarker and stop/reload button → Add a separator between urlbar dropmarker and stop/reload/go button
Posted patch patch (obsolete) — Splinter Review
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/
Attachment #8655964 - Flags: ui-review?(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 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.
Attachment #8655964 - Flags: ui-review?(shorlander) → ui-review-
(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.
Flags: needinfo?(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).
Flags: needinfo?(shorlander)
Attachment #8656623 - Flags: ui-review?(shorlander)
Posted patch patch v2Splinter Review
Attachment #8655964 - Attachment is obsolete: true
Attachment #8656638 - Flags: review?(jaws)
Attachment #8656638 - Flags: review?(jaws) → review+
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.
Attachment #8656623 - Flags: ui-review?(shorlander) → ui-review+
Flags: qe-verify-
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]:
Attachment #8656638 - Flags: approval-mozilla-beta?
Attachment #8656638 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/45b16903a80a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(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 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+
Attachment #8656638 - Flags: approval-mozilla-beta?
Attachment #8656638 - Flags: approval-mozilla-beta+
Attachment #8656638 - Flags: approval-mozilla-aurora?
Attachment #8656638 - Flags: approval-mozilla-aurora+
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?
Flags: needinfo?(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!
Flags: needinfo?(dao)
Depends on: 1202287
Posted image screenshot (obsolete) —
On OS X (HiDPI) there is no seperator, just a big gap…
(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?
Depends on: 1202506
(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.
I can post a patch this evening (UTC+1) if I have the time.
(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.
I filed bug 1202618, but that's not suitable for uplifting.
Attachment #8657888 - Attachment is obsolete: true
Attachment #8658124 - Flags: review?(gijskruitbosch+bugs)
I also pushed the accidentally missing border-image-slice lines to aurora and beta.
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. :-\
Attachment #8658124 - Flags: review?(gijskruitbosch+bugs) → review+
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]:
Attachment #8658124 - Flags: approval-mozilla-beta?
Attachment #8658124 - Flags: approval-mozilla-aurora?
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...
Attachment #8658124 - Flags: approval-mozilla-beta?
Attachment #8658124 - Flags: approval-mozilla-beta+
Attachment #8658124 - Flags: approval-mozilla-aurora?
Attachment #8658124 - Flags: approval-mozilla-aurora+
Depends on: 1203554
Posted image 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?
(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?
(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.
Depends on: 1203661
(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.
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
QA Whiteboard: [testday-20151002]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.